Thursday, December 11, 2008

Doing it wrong...

I hate queries of the following form:

select count(*) from <anything else here>

The reason?  The code typically looks something like this around the count(*):

select count(*) into l_cnt from .....;
if ( l_cnt > 0 )
then
process_the_data;
end if;


I've always wondered why that code isn't just:


process_the_data;



Why bother counting - and then processing if that count was greater than zero.  Why not just process_the_data - that routine already knows how to stop when it runs out of data - just let it run out of data naturally on row zero if there is no data.



Many people don't stop to consider that




  • The count can change between the select count(*) and the process_the_data call - there might be nothing by the time you get into the process_some_data


  • The count can change while you are running the process_some_data call itself - you cannot use the count as "this is how many times to iterate" (I've seen it done - it fails spectacularly when there are less rows than you counted, it fails silently when there are suddenly more and you never get to them, it also sometimes works by accident).



I've seen code like:



select count(*) into :cnt from t;
allocate array of :cnt elements
open C for select * from t;
for i in 1 .. :cnt
loop
fetch c into array(i);
end loop;
close c;


You can just imagine the damage that could do in a language like C for example - interesting results when there are less than :cnt rows to get, segmentation fault - core dumped (we HOPE - we hope it crashes) if there are more than :cnt rows to get.



 



Anyway, this isn't a post about "don't count and then process" (well, ok, it is in part) - this is a post about an interesting snippet of code a friend sent me.  They are on site doing some "tuning".  I've modified the variables and such to disguise it - but the "logic" is intact:



FUNCTION count_em_up
( p_input1 in number,
p_input2 in varchar2
)
return number
IS
CURSOR C
IS
SELECT actual_columns
FROM some_table
WHERE a_column = p_input1
AND another_column = p_input2;

l_the_cnt number default 0;
BEGIN
FOR rec IN C
LOOP
l_the_cnt := l_the_cnt+1;
END LOOP;
RETURN l_the_cnt;
EXCEPTION
WHEN OTHERS THEN
RETURN NULL;
END;


That hurts me in so many ways. 




  • The dreaded "when others <no error raised here>"


  • A loop to COUNT THE ROWS RETRIEVED BY A QUERY!!!


  • Because I did not believe it: A loop to COUNT!!! (had to be said twice)


  • A function to count rows - probably used in higher level code like this "if count_em_up(x,y) > 0 then process_some_data; end if;"



Well, at least there is the very real probability of tuning this particular application - there is probably lots and lots of low hanging fruit out there like this!

POST A COMMENT

33 Comments:

Blogger Gary Myers said....

Plus, if it really is actual_columns (as in plural) you may incur extra reads if it requires an INDEX + TABLE lookup over an INDEX only one.

Thu Dec 11, 09:58:00 PM EST  

Blogger Aman Sharma said....

Hi sir,
There is a recent open thread over OTN forums.
http://forums.oracle.com/forums/thread.jspa?forumID=61&threadID=835794
I don't know whether the guy's approach is right or not? Its still open but surely would like to hear your thoughts about it?
Regards
Aman....

Thu Dec 11, 11:29:00 PM EST  

Blogger Toon Koppelaars said....

Now imagine they do this kind of counting in the Java layer...
The only thing you would see is:

SELECT actual_columns
FROM some_table
WHERE a_column = :b1
AND another_column = :b2

Executed over and over, where every time fetching continues till %NOTFOUND. And you are left guessing why they submit these queries to the DBMS.

With the code you were sent, we can at least track down what the reason is, and propose a better way.

Fri Dec 12, 03:31:00 AM EST  

Blogger Marcel-Jan said....

It's a real candidate for the Oracle WTF blog (http://oracle-wtf.blogspot.com).

Fri Dec 12, 04:48:00 AM EST  

Blogger Thomas Kyte said....

@Aman

the poster that wrote I am not getting it. The requirement doesn't make sense. nailed it dead on.

Fri Dec 12, 06:27:00 AM EST  

Anonymous Expert DBA said....

Mr. Kite,

I've been a fan for years. Please tell me why this doesn't work? Oh, and I've actually tested this and NO_GAS is never thrown.

FUNCTION START_CAR();
RETURN NULL;

IS;

BEGIN;

SELECT key;
FROM key_ring;
WHERE key FITS ignition;

INSERT key;
INTO ignition;

EXCEPTION;

WHEN NO_GAS;
THEN RETURN FILE_NOT_FOUND;

WHEN OTHERS;
THEN RETURN NULL;

END PROCEDURE START_CAR();

Fri Dec 12, 08:33:00 AM EST  

Anonymous TakeFlight said....

That hurts me in so many ways.

;) heh heh


The dreaded "when others *no error raised here*"

Perhaps they're catching it upstream if the function returns a NULL? At least it's not

WHEN OTHERS THEN
RETURN 0;

Fri Dec 12, 09:12:00 AM EST  

Blogger Thomas Kyte said....

@TakeFlight

even if they "catch it up stream", that is so "return code" like and return codes are so 1980.

Exceptions - let them propagate up, do not 'hide them'.

I'm a fan of listing worst practices and this "when others" thing is among the WORST and most EVIL of worst practices...

Fri Dec 12, 09:24:00 AM EST  

Blogger Brian Tkatch said....

One case where it may be useful to COUNT(*) before processing, is to know if there is any work to be done. This can be used to avoid unnecessary logging, or even checking as the system still needs to check for work being done and that can take longer than a simple COUNT(*).

In our case, we have a processes that check for available batches every few seconds or so. If something is available, it grabs a process number (from a SEQUENCE) and processes the batches recording the process number on each batch.

If we did not use a COUNT(*) first, the SEQUENCE would be incrementing quite quickly. Although the maximum SEQUENCE number is higher than we will ever reach, it is easier to work with a smaller number, (and technically less space to store it).

Granted, the actually statements that work on the batches do not rely on the COUNT(*) at all, exactly for the reasons you stated, i would say that in some cases, doing a quick COUNT(*) is beneficial even if not ultimately reliable.

Fri Dec 12, 09:29:00 AM EST  

Blogger Thomas Kyte said....

The entire sequence thing would not cause me to pause at all.

A number is a number is a number. Dealing with the number 100 in code is just as easy as dealing with 1543219515123512 in code.

And in your case, the logic should not even grab the sequence UNLESS THERE WAS DATA to process.

So, I'm still of the mindset "do not count, just try to process and if NOTHING EXISTS TO PROCESS - bail out"


No need to get a sequence AND THEN process.

Make getting the sequence part of the process.

Fri Dec 12, 09:38:00 AM EST  

Blogger Brian Tkatch said....

"A number is a number is a number. Dealing with the number 100 in code is just as easy as dealing with 1543219515123512 in code."

I'm thinking of the coder who has to use ad-hoc queries later on when debugging. I figure a smaller number is easier to work with.

"And in your case, the logic should not even grab the sequence UNLESS THERE WAS DATA to process."

Isn't that done with a COUNT(*)? The actual statements deal with all available batches at the same time, which means the SEQUENCE.NextVal has to come first. (So as not to cause one id per batch, we want one id for all batches.)

Fri Dec 12, 09:52:00 AM EST  

Blogger Thomas Kyte said....

@Brian

123,321 is just as hard to cut and past as 4315315314513512 is when debugging. The numbers get large over time regardless. I don't see this as 'valid'

Look at my id's on asktom for example....


Or the post id's here on blogspot.


... Isn't that done with a COUNT(*)? ...



No, it doesn't have to be. If you have something to COUNT(*), you must have something you then loop over.

loop over them
assign sequence inside loop

Fri Dec 12, 09:56:00 AM EST  

Blogger Brian Tkatch said....

Tom, i see your point on the number. My preference is still otherwise, but that is all it is, a preference.

We are actually not looping at all. The process is kicked off by an external processes that runs in its own loop and delays by a configurable time. The DB processes works on all batches at the same time running each statement once.

I must be missing something here. I'll try to give a concrete example, and then you can show me how its done.

CREATE TABLE Batch_Master(Batch INT, Process INT);

CREATE TABLE Batch_Detail(Batch INT, Data INT);

A recurring processes wants to: UPDATE Batch_Master SET Process = Process.NextVal WHERE Process IS NULL;, which tags all current open batches, and retrieve the assigned Process id so the statements that actually work on the data have the process id.

Fri Dec 12, 10:13:00 AM EST  

Blogger Thomas Kyte said....

@Brian

perfect - easy - and efficient:

ops$tkyte%ORA10GR2> drop sequence s;

Sequence dropped.

ops$tkyte%ORA10GR2>
ops$tkyte%ORA10GR2> create sequence s;

Sequence created.

ops$tkyte%ORA10GR2> create table batch_master
2 ( batch int, process int );

Table created.

ops$tkyte%ORA10GR2>
ops$tkyte%ORA10GR2> insert into batch_master values ( 100, null );

1 row created.

ops$tkyte%ORA10GR2> insert into batch_master values ( 200, null );

1 row created.

ops$tkyte%ORA10GR2>
ops$tkyte%ORA10GR2> declare
2 l_batches sys.odcinumberlist;
3 l_processes sys.odcinumberlist;
4 begin
5 update batch_master
6 set process = s.nextval
7 where process IS NULL
8 returning batch, process BULK COLLECT
9 into l_batches, l_processes;
10
11 for i in 1 .. l_processes.count
12 loop
13 dbms_output.put_line
14 ( 'processing (' || l_batches(i) || ', ' ||
15 l_processes(i) || ')' );
16 end loop;
17 dbms_output.put_line( 'processed ' || l_processes.count || ' things' );
18 end;
19 /
processing (100, 1)
processing (200, 2)
processed 2 things

PL/SQL procedure successfully completed.

ops$tkyte%ORA10GR2> /
processed 0 things

PL/SQL procedure successfully completed.

ops$tkyte%ORA10GR2>
ops$tkyte%ORA10GR2> select s.nextval from dual;

NEXTVAL
----------
3


no "waste of a sequence", minimal work and yes, the "WHERE IS NULL" can and will use an index if sensible.


create index i on t(process,0)

Fri Dec 12, 10:40:00 AM EST  

Blogger Brian Tkatch said....

Tom, the only issue here is that the process that actually works on the batches does not use a FOR LOOP, the statement works on all tagged batches at the same time. And all the batches have the same process number.

Thanx for the example. I always learn from those.

Fri Dec 12, 11:05:00 AM EST  

Anonymous JHANSON said....

set serveroutput on size 20000
BEGIN
DECLARE
CURSOR igetit is
select 'idontgetit' huh
from dual;

noclue varchar2(20000);
clueless NUMBER := 1;
getaclue_o NUMBER := 1;
getaclue_e NUMBER := 2;

BEGIN
for ithinkigetit in igetit loop
select length(ithinkigetit.huh)
into noclue
from dual;
for i in 1 .. noclue loop
dbms_output.put_line('doh x'||i);
end loop;
for i in 1 .. noclue loop
if(getaclue_o=i) then
dbms_output.put_line('oh i get it x'||i);
getaclue_o := getaclue_o+2;
end if;
if (getaclue_e=i) then
dbms_output.put_line('doh x'||TO_CHAR(getaclue_o-1+length(ithinkigetit.huh)));
getaclue_e := getaclue_e+2;
end if;
end loop;
end loop;
END;
END;
/

Fri Dec 12, 02:26:00 PM EST  

Blogger Rahul said....

One reason you would want to check the count of the records is to see you if you want to create the file the file or not. now imagine you create the file and there are no rows outputted from the select and then, you will have an incomplete file and that is not acceptable...

Rahul.

Fri Dec 12, 03:42:00 PM EST  

Blogger Thomas Kyte said....

@Rahul

ok, say you count and it is 1 and you create the file and then when you query you get zero (that is my point - who is to say the count you get will be the number of records you get - how many times have I seen that bug.... when you get MORE or LESS than you anticipated - usually explodes in a spectacular fashion).


and come on, you cannot figure out how to open the file after finding data to process if that is your goal?


for x in (select ...)
loop
if file isn't open then open it
write


or,

open c;
fetch c into rec;
if c%found then
open file
end if
while c%found loop
write
fetch c into rec
end loop

Not harder than

select count(*)
if cnt > 0 then
open file
end if
for x in (select * ....)
loop
write
end loop

not only not harder BUT it actually works all of the time - if you have data to write, it opens the file and writes it - else - it doesn't. The count and then open and then fetch by itself - is a BUG, it does not match the requirement.

Fri Dec 12, 03:50:00 PM EST  

Blogger Rahul said....

@Tom,

Hmm...I see your point of this being a bug, yes, but, I guess the reason I did it this way instead of directly creating the file in the loop is because my requirement needs that I have a some header information I need to spew out into the file that DOESN'T repeat in the loop. I guess I can use some kind of boolean variable to check if I already wrote that header information or not....

And now that I think about it, yes, it is a bug..waiting to happen....I was aware of that, but, then, couldn't foresee using this boolean for taking care of the header information in the file and also deciding whether the file is is created....

Fri Dec 12, 05:16:00 PM EST  

Blogger Thomas Kyte said....

@Rahul

but I showed you how to do it outside the loop??

right now, you have something like:

select count
if (cnt>0)
then
open file
write header
end if
for x in (select ... )
loop
write file
end loop


All you need to do is

open cursor
fetch c into rec
if ( c%found )
then
open file
write header
end if;
while (c%found)
loop
write file
fetch c into rec
end loop;


The logic is identical as far as "open file/write header" goes - just one of them works and the other is waiting to break.

Fri Dec 12, 05:19:00 PM EST  

Blogger David Kurtz said....

I have found myself fixing this sort of thing many times. While it would have been better not to write the code that way in the first place, in order to make the most minimal change to production code I will often add:

WHERE ROWNUM<=1

At least that stops any scan on the database as soon as it has found one row.

Fri Dec 12, 05:20:00 PM EST  

Blogger Rahul said....

@Tom,

>>open cursor
>>fetch c into rec
>>if ( c%found )

Whenever I see this kind of code, my mind just shuts off...Maybe because in some cases (of course not all the cases), you have said not to do this. Use the 'for loop' instead of explicitly opening the cursor....because, Oracle takes care of opening the cursor , handles it be itself and closes it itself(I guess less bugs that way)...

But, I guess this is one scenario where you use the explicitly open the cursor, check the %FOUND and then go forward....

Fri Dec 12, 05:27:00 PM EST  

Blogger Thomas Kyte said....

@Rahul

never say never
never say always
I always say


Use implicit cursors when you can, use explicit when it makes sense. I would never use an explicit cursor when an implicit one would work - a boolean inside the loop would work - but I think it would be "uglier" (and it would have to do the check each iteration)

Fri Dec 12, 05:34:00 PM EST  

Blogger Rahul said....

@Tom,

>>never say never
>>never say always
>>I always say.

You couldn't be more right, and it is now time to refactor (although technically it is bug fixing) that part of my code :).

Fri Dec 12, 05:54:00 PM EST  

Blogger Thomas Kyte said....

@Rahul

one thing I forgot to say

consider BULK FETCH if this is going to be more than a few dozen/hundred rows

Fri Dec 12, 05:56:00 PM EST  

Blogger Aman Sharma said....

@Aman

the poster that wrote I am not getting it. The requirement doesn't make sense. nailed it dead on.

That was me only sir :-).
Thanks , I got the reply.
Regards
Aman....

Fri Dec 12, 10:33:00 PM EST  

Blogger Stew said....

You wrote:

That hurts me in so many ways.
[snip]
* A function to count rows ...

I'm thinking Steven Feuerstein would be thrilled that they wrote a function to count rows. I like much of what he suggests, but his penchant for endless functions, procedures and modularizing modules goes a bit far for my tastes. I do it much more than my co-workers, but there are limits, eh?

Thanks for the great example.

Sun Dec 14, 11:17:00 PM EST  

Anonymous Anonymous said....

Doesn't select count(*) lead to Block Cleanout? If so, then there MAY be a good reason to do the count and then the processing: if one wanted to compare like-for-like runs of the processing, without Block Cleanout fuzzing the timings?

Then again, I may be talking rubbish, again!

Tue Dec 16, 12:02:00 PM EST  

Blogger Gary Myers said....

"Block Cleanout"
But which block(s) ? The query may or may not use an index. Any intention like that should at least be commented

Tue Dec 16, 08:46:00 PM EST  

Blogger Chen Shapira said....

If I understand correctly, select will not cleanout blocks permanently, and subsequent selects may still do the same cleanouts until DML writes the cleaned block to file permanently.

Mon Feb 09, 09:21:00 PM EST  

Blogger Thomas Kyte said....

@Chen, incorrect, selects can clean out the blocks totally (this is one way a select can generate redo for example)

Mon Feb 09, 09:25:00 PM EST  

Blogger Chen Shapira said....

I think my source is simply outdated, but Steve Adams wrote:
(http://www.ixora.com.au/q+a/cr.htm)

?
Delayed block cleanout

27 December 1999
Why would delayed block cleanout be needed on a table that is only ever queried and never updated?

? Every data block that is needed for the query will have been changed by at least one transaction at some point in the history of the database. If the row-level locks required for the last (possibly ancient) transaction to modify the block were not able to be cleaned out at the time, and if delayed_logging_block_cleanouts is TRUE (which is the default) then all subsequent queries will need to perform delayed block cleanout until another transaction modifies the block and cleans out the old row-level locks permanently.


The "all subsequent queries" part is the behavior I was talking about.

Mon Feb 09, 09:48:00 PM EST  

Blogger Rahul said....

Tom,

>>Use implicit cursors when you can, use explicit when it makes sense.

I used the examples you gave for my earlier posts here for the use of explicit cursors. Although, I wasn't able to refactor that code, I am now using explicit cursor(because the requirement warrants it) for a new requirement. Thank you for your help.

Thu Nov 05, 11:58:00 AM EST  

POST A COMMENT

<< Home