Well, not really a competition because we haven't got any prizes to give away, but the code below (thanks Padders) has a real "How Many WTFs Can You Spot?" feel about it. I make it four.
PROCEDURE log_error
( p_source IN VARCHAR2
, p_result IN VARCHAR2)
IS
PRAGMA AUTONOMOUS_TRANSACTION;
p_primarykey errorlogging.primarykey%TYPE;
BEGIN
SELECT errorlogging_seq.NEXTVAL
INTO p_primarykey
FROM dual;
INSERT /*+ APPEND */ INTO errorlogging NOLOGGING
( primarykey
, source
, result
, timestamp
, wherewasi
, processed_count
, process_id )
VALUES
( p_primarykey
, p_source
, SUBSTR(p_result, 1, 1000)
, SYSDATE
, NULL
, NULL
, 1 );
COMMIT;
EXCEPTION
WHEN OTHERS THEN NULL;
END;
7 comments:
Let's see:
1. p_source and p_result could be %TYPEd on the underlying table columns.
2. p_primarykey should use some other prefix than p_ because it's not a parameter.
3. WTF select on sequence from dual? Move it to the insert statement.
4. WTF insert with append and nologging on a log table? So if the database crashes just after logging you've lost what you logged anyway. Besides for one record it's not going to yield the expected massive performance improvements :-)
5. WTF ignore all exceptions?
6. why is there a SUBSTR on p_result but not on p_source?
7. timestamp can be DEFAULT SYSDATE.
8. wherewasi, processed_count being filled with NULL: can drop these columns if they're never filled, or fill them; if they really are optional why include them in the insert statement? In general I use DEFAULTs and don't specify these columns and optional columns when inserting.
9. why not fill process_id with NULL if it's not specified, or is this some business logic that this is process "1"?
10. drop the surrogate key.
I got three WTF's... could be four if the use of APPEND and nologging are separate items.
Yes I was counting APPEND and NOLOGGING as two separate WTFs, but not quite for the reason you mention. There is actually nothing in the INSERT statement to prevent logging. The developer seems to have misunderstood INSERT /*+ APPEND */ and the NOLOGGING attribute, and the database will just see a regular INSERT with a meaningless hint and table alias, which it will ignore.
I agree about using "p_" for things that aren't parameters though. I wonder whether the procedure once had an OUT parameter, and this was why it had the separate step to derive errorlogging_seq.NEXTVAL (they didn't know about RETURNING either).
Only perhaps without the NOLOGGING ;)
Given that direct path INSERT uses blocks above the high water mark, I wonder what that would do to your space usage.
I don't agree with
"10. drop the surrogate key."
When a lot of log entries are created within one second, the timestamp is not enough to tell the order of creation. And it is easier to tell a coworker "please have a look at log entries 124817..124922, looks odd" if you have such a key.
I disagree with
5. WTF ignore all exceptions?
as we do not know the exact requirements for this proc. If the requirement was to log the errors/exceptions without disrupting the main process, this makes sense.
> I disagree with
> 5. WTF ignore all exceptions?
Here we go.
http://tkyte.blogspot.com/2006/08/ouch-that-hurts.html
If the requirement was to log the errors/exceptions without disrupting the main process, this makes sense.
Spoken like a man drafted on to the overnight roster.
Post a Comment