Skip to Main Content
  • Questions
  • Regarding Profiling Stored Procedures

Breadcrumb

Question and Answer

Tom Kyte

Thanks for the question, Hariharan.

Asked: October 20, 2009 - 5:49 am UTC

Last updated: October 23, 2009 - 11:53 am UTC

Version: 10.0.2

Viewed 1000+ times

You Asked

Hi Tom,

Good Day.

In my current company, a colleague of mine have written a stored procedure (which is provided below). He asked me to tune. In my current company we do not have access to any of the dynamic performance views or any other tools like TKPROF to profile the stored procedure to find out where it takes time.

At the outset, the logic in the stored procedure is correct and seems to be perfect, but takes very long time to execute.



The logic is provided below:

a) There are 3 tables MS_FO_INVESTMENTVEHICLE, MS_FO_AREANAME, MS_FO_FEESCHEDULE.
b) Exact replica of these 3 tables are MS_FO_INVESTVEH_UPDATES, MS_FO_ANAME_UPDATES, MS_FO_FEESCH_UPDATES. These tables are exposed to other applications accessing this database.
c) Data comes in two modes one is Refresh (R) and other one is Delta (here it is specified as U)
d) Whenever data comes into the system, they are initially written into the tables specified in a) and then the replicated data are updated or inserted in tables provided in b)
e) In case of 'R', the records in the tables provided in b) must be deleted and then fresh data to be inserted. In case of 'U', the records in MS_FO_INVESTVEH_UPDATES must be updated (if already exists) or inserted (new records) and the records must be deleted and inserted in other 2 tables specified in b)
f) Once this is done, there is a column called Processed, which must be updated to Y. Default is N

All the necessary indexes are created.

Please pardon me for the format of the stored procedure provided below

--------------Procedure Starts Here--------------
/* Created for Feed Handle Process */
create or replace procedure db_FeedUpdMorningStarProc(inibdpartyid in number) as
cursor rucur is select distinct refreshupdate from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N' order by refreshupdate;
tempid number;
begin
for ru in rucur
loop
/* R implies Full Refresh. In this case, the records for the particular IBDPARTYID and LEGALTYPE would be deleted and then inserted with the new data*/
if ru.refreshupdate ='R' then
dbms_output.put_line('Calling Refresh Procedure');
db_RefreshDataProc(inibdpartyid);
dbms_output.put_line('Calling Refresh Procedure Completed');
/* U implies Delta. In this case, the records for the particular IBDPARTYID and LEGALTYPE would be deleted from two child tables and then inserted with the new data*/
elsif ru.refreshupdate ='U' then
dbms_output.put_line('Calling Update Procedure');
db_UpdateDataProc(inibdpartyid);
dbms_output.put_line('Calling Update Procedure Completed');
end if;
end loop;
dbms_output.put_line('Calling Update N to Y Procedure');
db_UpdateNToYProc(inibdpartyid);
dbms_output.put_line('Update N to Y Procedure Completed');
exception
When others then
dbms_output.put_line('Error is ' || SQLERRM || ' ----------> ' || tempid);
end;

/* The following procedure takes care of Refresh Data */
create or replace procedure db_RefreshDataProc(inibdpartyid in varchar2) as
cursor refcur is select distinct legaltype from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N';
begin
for rcur in refcur
loop
delete from MS_FO_INVESTVEH_UPDATES where ibdpartyid=inibdpartyid and legaltype=rcur.legaltype;
delete from ms_fo_feeschupdates where ibdpartyid=inibdpartyid and exists (select id__ from ms_fo_investmentvehicle where legaltype=rcur.legaltype and processed='N');
delete from ms_fo_aname_updates where ibdpartyid=inibdpartyid and exists (select id__ from ms_fo_investmentvehicle where legaltype=rcur.legaltype and processed='N');
dbms_output.put_line('Delete Refresh Done');
insert into MS_FO_INVESTVEH_UPDATES select * from ms_fo_investmentvehicle where processed='N' and ibdpartyid=inibdpartyid and legaltype=rcur.legaltype;
insert into ms_fo_feeschupdates select * from ms_fo_feeschedule where exists (select id__ from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N' and legaltype=rcur.legaltype);
insert into ms_fo_aname_updates select * from ms_fo_areaname where exists (select id__ from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N' and legaltype=rcur.legaltype);
dbms_output.put_line('Insert Refresh Done');
end loop;
commit;
dbms_output.put_line('Refresh Done');
exception
When others then
dbms_output.put_line('Error In Refresh Procedure and Error is ' || SQLERRM);
end;

/* The following procedure takes care of Update Processed value from N to Y Data */
create or replace procedure db_UpdateNToYProc(inibdpartyid in varchar2) as
begin
update ms_fo_investmentvehicle set processed='Y' where ibdpartyid=inibdpartyid and processed='N';
dbms_output.put_line('N On Investmentvehicle Done');
update MS_FO_INVESTVEH_UPDATES set processed='Y' where ibdpartyid=inibdpartyid and processed='N';
dbms_output.put_line('N On Investmentvehicle Updates Done');
commit;
exception
When others then
dbms_output.put_line('Error In Update N To Y Procedure and Error is ' || SQLERRM);
end;
/* The following procedure takes care of Update Data */
create or replace procedure db_UpdateDataProc(inibdpartyid in varchar2) as
begin
merge into MS_FO_INVESTVEH_UPDATES a
using (select ID__, INVESTMENTVEHICLENAME, LEGALTYPE, LEGALNAME, SHARECLASSTYPE_ID, CUSIP, ISIN, TRADINGSYMBOL, FUNDFAMILYNAME, FUNDFAMILYNAME_ID, RATINGY3, RISKRATINGY3, RATINGY5, RISKRATINGY5, RATINGY10, RISKRATINGY10, RATINGOVERALL, RISKRATINGOVERALL, FEEDPATHID, IBDPARTYID, CREATEDDATE, PROCESSED, NAME, ID, FRONTLOAD, DEFERLOAD, REFRESHUPDATE from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N' and refreshupdate='U') b
on (a.id__=b.id__)
when MATCHED then update set INVESTMENTVEHICLENAME=b.INVESTMENTVEHICLENAME, LEGALNAME=b.LEGALNAME, SHARECLASSTYPE_ID=b.SHARECLASSTYPE_ID, ISIN=b.ISIN, TRADINGSYMBOL=b.TRADINGSYMBOL, FUNDFAMILYNAME=b.FUNDFAMILYNAME, FUNDFAMILYNAME_ID=b.FUNDFAMILYNAME_ID, RATINGY3=b.RATINGY3, RISKRATINGY3=b.RISKRATINGY3, RATINGY5=b.RATINGY5, RISKRATINGY5=b.RISKRATINGY5, RATINGY10=b.RATINGY10, RISKRATINGY10=b.RISKRATINGY10, RATINGOVERALL=b.RATINGOVERALL, RISKRATINGOVERALL=b.RISKRATINGOVERALL, FEEDPATHID=b.FEEDPATHID, CREATEDDATE=b.CREATEDDATE, NAME=b.NAME, ID=b.ID, FRONTLOAD=b.FRONTLOAD, DEFERLOAD=b.DEFERLOAD where a.cusip=b.cusip and a.processed=b.processed and a.refreshupdate=b.refreshupdate and a.ibdpartyid=b.ibdpartyid and a.legaltype=b.legaltype
when not MATCHED then insert(ID__, INVESTMENTVEHICLENAME, LEGALTYPE, LEGALNAME, SHARECLASSTYPE_ID, CUSIP, ISIN, TRADINGSYMBOL, FUNDFAMILYNAME, FUNDFAMILYNAME_ID, RATINGY3, RISKRATINGY3, RATINGY5, RISKRATINGY5, RATINGY10, RISKRATINGY10, RATINGOVERALL, RISKRATINGOVERALL, FEEDPATHID, IBDPARTYID, CREATEDDATE, PROCESSED, NAME, ID, FRONTLOAD, DEFERLOAD, REFRESHUPDATE) values (b.ID__, b.INVESTMENTVEHICLENAME, b.LEGALTYPE, b.LEGALNAME, b.SHARECLASSTYPE_ID, b.CUSIP, b.ISIN, b.TRADINGSYMBOL, b.FUNDFAMILYNAME, b.FUNDFAMILYNAME_ID, b.RATINGY3, b.RISKRATINGY3, b.RATINGY5, b.RISKRATINGY5, b.RATINGY10, b.RISKRATINGY10, b.RATINGOVERALL, b.RISKRATINGOVERALL, b.FEEDPATHID, b.IBDPARTYID, b.CREATEDDATE, b.PROCESSED, b.NAME, b.ID, b.FRONTLOAD, b.DEFERLOAD, b.REFRESHUPDATE);
dbms_output.put_line('Merge Done');
delete from ms_fo_feeschupdates where ibdpartyid=inibdpartyid and exists (select id__ from ms_fo_investmentvehicle where processed='N');
dbms_output.put_line('Delete 1 Done');
delete from ms_fo_aname_updates where ibdpartyid=inibdpartyid and exists (select id__ from ms_fo_investmentvehicle where processed='N');
dbms_output.put_line('Delete 2 Done');
insert into ms_fo_feeschupdates select * from ms_fo_feeschedule where exists (select id__ from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N');
insert into ms_fo_aname_updates select * from ms_fo_areaname where exists (select id__ from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N');
commit;
dbms_output.put_line('Update Done');
exception
When others then
dbms_output.put_line('Error In Update Procedure and Error is ' || SQLERRM);
end;

--------------Procedure Ends Here--------------

a) I would like to know a way I can profile the stored procedures
b) Is there a way I can use Bulk Insert here? If yes, can you please site some examples or links where it is demonstrated

Thanks a lot

Hari

and Tom said...

... In my current company we do not have access to any of the dynamic performance views or any other tools like TKPROF to profile the stored procedure to find out where it takes time. ...

why not? get it, without it - you are somewhat DOOMED.



exception
When others then
  dbms_output.put_line('Error is ' || SQLERRM || ' ----------> ' || tempid);
end; 


I hate your code. All of it.

http://asktom.oracle.com/pls/asktom/asktom.search?p_string=%22i+hate+your+code%22


I cannot bear to read it with that 'construct'.

Also, without some formatting - it is unreadable.

I don't know your schema
I don't know your data volumes
I don't know anything about your stuff.

and I cannot read the existing code

and it has that horrible "when others then null" bug in it.


... All the necessary indexes are created. ...

either that statement is wrong or it is right. We have no way to judge that from where we sit

for a batch process - probably ZERO indexes would be useful. I see slow by slow processing in there (for loops).

Suggestion: get rid of slow by slow code, turn it into a single set operation.



I see some really "questionable" code there.


Look at this delete:

delete from ms_fo_feeschupdates where ibdpartyid=inibdpartyid and exists (select id__ from ms_fo_investmentvehicle where legaltype=rcur.legaltype and processed='N');


Now, rcur is defined as:

select distinct legaltype from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N';




since you get distinct legaltype from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N';


would that delete not just simply be (outside of a loop - no looping required/desired for this!)

  delete from ms_fo_feeschupdates where ibdpartyid=inibdpartyid
     and exists (select null from ms_fo_investmentvehicle where       
                   ibdpartyid=inibdpartyid and processed='N');


you KNOW that

and exists (select id__ from ms_fo_investmentvehicle where legaltype=rcur.legaltype and processed='N');


must be satisfied since you are looping over

select distinct legaltype from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N';


that legaltype for processed = 'N' must exist, else you would not have retrieved it on the distinct




Further, that code in the loop - why is there a loop at all, why are they not just all SET BASED operations - what point, what purpose, what goal is the loop? (only to slow things down....)


As far as I can tell, the one procedure should be nothing more than:


create or replace procedure db_RefreshDataProc(inibdpartyid in varchar2)
as
begin
  delete from MS_FO_INVESTVEH_UPDATES
   where ibdpartyid=inibdpartyid
     and legaltype in ( select legaltype from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N' );

  delete from ms_fo_feeschupdates where ibdpartyid=inibdpartyid
     and exists (select null from ms_fo_investmentvehicle where       
                   ibdpartyid=inibdpartyid and processed='N');

  delete from ms_fo_aname_updates
   where ibdpartyid=inibdpartyid
     and exists (select null from ms_fo_investmentvehicle where       
                   ibdpartyid=inibdpartyid and processed='N');

  dbms_output.put_line('Delete Refresh Done');
  insert into MS_FO_INVESTVEH_UPDATES
  select *
    from ms_fo_investmentvehicle
   where processed='N' and ibdpartyid=inibdpartyid;

  insert into ms_fo_feeschupdates
  select *
    from ms_fo_feeschedule
   where exists (select null from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N';

  insert into ms_fo_aname_updates
  select *
    from ms_fo_areaname
   where exists (select id__ from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N');
end;





remember that rcur is defined as:

select distinct legaltype from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N';


in the loop therefore:

delete from MS_FO_INVESTVEH_UPDATES where ibdpartyid=inibdpartyid and legaltype=rcur.legaltype;

would just be

delete from MS_FO_INVESTVEH_UPDATES where ibdpartyid=inibdpartyid and legaltype in (that rcur set);


and

delete from ms_fo_feeschupdates where ibdpartyid=inibdpartyid and exists (select id__ from ms_fo_investmentvehicle where legaltype=rcur.legaltype and processed='N');

is just

delete from ms_fo_feeschupdates where ibdpartyid=inibdpartyid
and exists (select null from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N');


and so on.


And then I see that RefreshDataProc is called itself in a loop - meaning, we need to FURTHER decompose all of this and build an even larger, more comprehensive delete statement (the goal being to remove EVERY LOOP in your code and replace with SINGLE SQL STATEMENTS that do all of the work in bulk - avoiding every index possible!!!)



This code needs heavy duty review and refactoring. It needs to literally be ripped apart - and turned into a specification and rewritten from the ground up.




and please, never ever EVER code:

exception
When others then
  dbms_output.put_line('Error In Update Procedure and Error is ' || SQLERRM);
end 



again, it makes you look like a high school programmer at best. It is the single most worst practice I know of.


Rating

  (3 ratings)

Is this answer out of date? If it is, please let us know via a Comment

Comments

Regarding Profiling Stored Procedures

Hariharan Sairam, October 20, 2009 - 7:34 am UTC

Hi Tom,

Thanks for being our eye-openers. In future will follow your nice tips and advise.

Please clarify the following:

a) What is the reason for using "select null" in the statement
delete from ms_fo_aname_updates where ibdpartyid=inibdpartyid and exists (select null from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N');
or
insert into ms_fo_feeschupdates select * from ms_fo_feeschedule where exists (select null from ms_fo_investmentvehicle where ibdpartyid=inibdpartyid and processed='N');

b) What exactly is the meaning of the following statement:
"when others then null" bug in it

Thanks Again

Hari
Tom Kyte
October 22, 2009 - 4:23 pm UTC

a) where exists never should select anything in the exists subquery. It doesn't make sense, you are looking for the existence of a row - you are not looking for data values.

Let me ask you - what was the intent behind:

delete from ms_fo_feeschupdates where ibdpartyid=inibdpartyid and exists (select id__ from ms_fo_investmentvehicle where legaltype=rcur.legaltype and processed='N');


why did you select id__??? what was your intent, what was your meaning?



b) did you read any of the links I linked to?

when others then <no raise>

is a bug, I said that as clearly as I can.

Your code has:


exception
When others then
  dbms_output.put_line('Error is ' || SQLERRM || ' ----------> ' || tempid);
end; 



there is no raise, to me that is identical to having:

when other then null;

therefore, you have littered throughout your code the infamous:

"when others then null" bug in it

Completely Agree

Analyst, October 21, 2009 - 5:05 am UTC

"... In my current company we do not have access to any of the dynamic performance views or any other tools like TKPROF to profile the stored procedure to find out where it takes time. ...

why not? get it, without it - you are somewhat DOOMED. "

I completely agree with Tom. To ask someone to analyse the performance of a stored procedure, but to then not provide them with the access to the required tools, is to make visible a clear case of incompetence. In fact, any "manager", who follows such a policy should be sacked immediately. You will be surprised that are actually people out there who would follow such a policy.

Two Suggestions

Analyst, October 21, 2009 - 6:37 am UTC

Having read only the first few lines of the procedure, I shall offer some advice:

1) All database objects must be assigned a name that does not contain any of the following terms:

tbl, db, proc, procedure, function, sp, fn.

I wish Oracle would raise an exception when a user attempts to name an object in such a way.

I wonder what names such people give to their children?

Child_John, Child_Michael

2) Document all code.
Tom Kyte
October 23, 2009 - 11:53 am UTC

1) the sp_ comes from sqlserver - Sybase first and then when Microsoft bought their code, they kept it.

All stored procedures start with sp_ in sqlserver - they don't have to, but everyone does....

More to Explore

PL/SQL demos

Check out more PL/SQL tutorials on our LiveSQL tool.

PL/SQL docs

PL/SQL reference manual from the Oracle documentation library