And I am pretty sure many of my readers will have suggestions for other code that should never appear in your PL/SQL programs. Let me know in the comments and I will add them to the post (giving me all the credit of course - no, just joking! YOUR NAME will be bright lights. :-) ).
Here's the list of things to avoid:
- DON'T set default values of variables to NULL.
- DON'T select from dual for....just about anything.
- DON'T declare the FOR loop iterator.
- DON'T trap NO_DATA_FOUND for inserts, updates and deletes.
- DON'T execute the same DML statement inside a loop.
- DON'T hide errors.
- DON'T re-raise an error without logging first.
Set default value to NULL
Whenever you declare a variable it is assigned a default value of NULL. So you should not explicitly provide NULL as a default value. It won't do any harm, but it will tell others who read your code that your understanding of PL/SQL is, shall we say, incomplete.
DECLARE l_number NUMBER := NULL; /* Same applies to using alternative syntax */ l_date DATE DEFAULT NULL; BEGIN ... END;
Just remove the assignment.
DECLARE l_number NUMBER; l_date DATE; BEGIN ... END;
Note that this advice does not apply to assigning a default value to a parameter.
PROCEDURE do_stuff (start_date_in IN DATE, end_date_in IN DATE DEFAULT NULL) ...
If you do not provide a default value for a parameter, then you must pass an element (expression, variable, constant) for that parameter. If you provide a default value, then you can "leave it off" the parameter list when the subprogram is invoked, and the default value will be used.
In other words, including or excluding a default value of NULL for a parameter can change the behavior of the subprogram and the way it's used. Including or excluding a default value of NULL for a variable has no effect.
Select from DUAL for....just about anything
A long, long time ago, before PL/SQL was all grown up, it didn't have native implementations for some SQL functions like SYSDATE. So developers would use a "dummy" SELECT against a pre-defined table like DUAL in order to fall back on the SQL engine to get the job done, as in:
DECLARE l_now DATE; BEGIN SELECT SYSDATE INTO l_now FROM dual; END;
Plus, if you wanted to get the next (or current) value of a sequence, you had to call the appropriate function via SQL, like:
DECLARE l_next_pky INTEGER; BEGIN SELECT my_seq.NEXTVAL INTO l_next_pky FROM dual; END;
DECLARE l_now DATE; l_next_pky INTEGER; BEGIN l_now := SYSDATE; l_next_pky := my_seq.NEXTVAL; END;
Declaration of FOR loop iterator
When you use a FOR loop (numeric or cursor), PL/SQL automatically declares a variable for the record referenced in that loop, and then releases memory for that variable when the loop terminates. If you declare a variable with the same name, your code will compile, but you could easily introduce bugs into that code as you see below:
In both of the blocks below, the "confirming" call to DBMS_OUTPUT.put_line to confirm that the desired action occurred will never be executed. In the first block, the "indx"referenced in the IF statement resolves to the locally declared variable, which is not the same indx as the loop iterator. It is initialized to NULL and stays that well. Same with the explicitly declared rec variable in the second block.
DECLARE indx INTEGER; BEGIN FOR indx IN 1 .. 12 LOOP DBMS_OUTPUT.put_line (TO_DATE ('2018-' || indx || '-01', 'YYYY-MM-DD')); END LOOP; IF indx = 12 THEN DBMS_OUTPUT.put_line ('Displayed all twelve months!'); END IF; END; / DECLARE rec employees%ROWTYPE; BEGIN FOR rec IN (SELECT * FROM employees) LOOP DBMS_OUTPUT.put_line (rec.last_name); END LOOP; IF rec.employee_id IS NOT NULL THEN DBMS_OUTPUT.put_line ('Displayed all employees!'); END IF; END; /
First step in clean up is to remove the declarations of those unused and unnecessary variables. The next step is for you to decide what logic you need to replace the IF statements after the loops. In the first case, why would I need an IF statement? If I got past the loop without an exception, then I certainly did display all the months.
In the second block, it's a little bit trickier. When I use a cursor FOR loop, the PL/SQL engine does everything for me: open the cursor, fetch the rows, close the cursor. So once the cursor is closed, I have no visibility into what happened inside the cursor. If I want to know whether at least one row was fetched, for example, I need to set a local variable as you see below.
BEGIN FOR indx IN 1 .. 12 LOOP DBMS_OUTPUT.put_line (TO_DATE ('2018-' || indx || '-01', 'YYYY-MM-DD')); END LOOP; DBMS_OUTPUT.put_line ('Displayed all twelve months!'); END; / DECLARE l_displayed BOOLEAN := FALSE; BEGIN FOR rec IN (SELECT * FROM employees) LOOP DBMS_OUTPUT.put_line (rec.last_name); l_displayed := TRUE; END LOOP; IF l_displayed THEN DBMS_OUTPUT.put_line ('Displayed all employees!'); END IF; END; /
WHEN NO_DATA_FOUND for non-query DML
When you execute an implicit query (SELECT-INTO), PL/SQL raises NO_DATA_FOUND if no rows are returned by the query. As in:
DECLARE l_id employees.employee_id%TYPE; BEGIN SELECT employee_id INTO l_id FROM employees WHERE 1 = 2; EXCEPTION WHEN NO_DATA_FOUND THEN DBMS_OUTPUT.put_line ('Not with that WHERE clause!'); END; / Not with that WHERE clause!
But when I run this block (changing the SELECT to an UPDATE), no output is displayed.
BEGIN UPDATE employees SET last_name = UPPER (last_name) WHERE 1 = 2; EXCEPTION WHEN NO_DATA_FOUND THEN DBMS_OUTPUT.put_line ('Not with that WHERE clause!'); END; /
That's because the PL/SQL engine does not raise an error for non-query DML that change no rows.
If you need to know that a row was modified (or how many rows were modified), use the SQL%ROWCOUNT attribute.
BEGIN UPDATE employees SET last_name = UPPER (last_name) WHERE 1 = 2; IF SQL%ROWCOUNT = 0 THEN DBMS_OUTPUT.put_line ('Not with that WHERE clause!'); END IF; END; / Not with that WHERE clause!
Loop containing non-query DML inside
Last up, a biggie. That is, bad code with a potentially enormous negative impact on performance. Here goes:
CREATE TABLE parts ( partnum NUMBER PRIMARY KEY, partname VARCHAR2 (15) UNIQUE NOT NULL ) / BEGIN FOR indx IN 1 .. 10000 LOOP INSERT INTO parts (partnum, partname) VALUES (indx, 'Part' || indx); END LOOP; END; /
When you execute the same DML inside a loop repeatedly, changing only the variables that are bound into the statement, you are unnecessarily (and often dramatically) slowing down your code. The problem here is that I am switching between the PL/SQL and SQL engines 10000 times. Those context switches are relatively expensive.
You should avoid this kind of row-by-row processing whenever possible. The key anti-pattern to look for is a loop with non-query DML (insert, update, delete, merge) inside it.
There are two ways to clean up this slow code.
1. Write it in "pure SQL" if you can. If you don't need PL/SQL algorithms to process the data, and can do it all in SQL, then do it that way. For example in the above case, I could have written:
BEGIN INSERT INTO parts SELECT LEVEL, 'Part ' || LEVEL FROM DUAL CONNECT BY LEVEL <= 1000; END;
If you need PL/SQL or simply cannot figure out how to write it in pure SQL, then use the FORALL statement instead of a FOR loop. This statement results in just one context switch, with PL/SQL sending all the bind variables from the bind array (l_parts) across to SQL with a single INSERT statement (that statement, after all, never changes).
DECLARE TYPE parts_t IS TABLE OF parts%ROWTYPE; l_parts parts_t := parts_t (); BEGIN l_parts.EXTEND(10000); FOR indx IN 1 .. 10000 LOOP l_parts (indx).partnum := indx; l_parts (indx).partname := 'Part' || indx; END LOOP; FORALL indx IN 1 .. l_parts.COUNT INSERT INTO parts (partnum, partname) VALUES (l_parts (indx).partnum, l_parts (indx).partname); END; /
For a much more detailed demonstration of converting row-by-row processing to bulk processing, check out my LiveSQL script.
Check the doc for lots more information on PL/SQL bulk processing.
(suggested by Patrick Jolliffe, @jolliffe)
When an exception is raised in your code, you really should do something about it, namely:
- Write the error to your log table.
- Make nice with your users (don't show them an error stack or "internal" error message).
- Analyze the error and get it fixed ASAP.
BEGIN your_code_here; EXCEPTION /* This will completely ignore any error */ WHEN OTHERS THEN NULL; END; / BEGIN your_code_here; EXCEPTION /* This will write information to the output buffer. Can you see that in production? I doubt it! */ WHEN OTHERS THEN DBMS_OUTPUT.PUT_LINE ( 'OMG! Something went wrong: ' || SQLERRM); END; /
The problem with these exception handlers is that you are "swallowing up" errors - you will never see them, your users will never know something went wrong, bugs will persist, and your karma will take a major downturn. Just sayin'.
What should you do instead?
First of all, there are certainly situations in which an exception is raised and you actually, truly "don't care" - because it's not really an error. The classic example of this scenario is doing a SELECT-INTO that doesn't find a row. PL/SQL will raise the NO_DATA_FOUND exception, but that's not necessarily an error - it could just be a "data condition". As in: there's no row for that criteria, so now I will take branch A in my code instead of branch B.
So ignoring an exception is justified. But in that case, you should only ignore the single (or perhaps several) exception that is not really an exception, as in:
BEGIN BEGIN SELECT id INTO l_id FROM my_table WHERE nm = NAME_IN; EXCEPTION WHEN NO_DATA_FOUND THEN /* It's OK for the row not to exist. This just means we do A instead of B */ l_next := 'A'; END; IF l_next = 'A' THEN NULL; ELSE NULL; END IF; END; /
And I do encourage you to include a comment explaining why you are not logging and re-raising the error.
More generally, I urge you to carefully consider whether or not an exception should be ignored.
If the exception is to be ignored, then ask yourself: do you want to know this happened? If not, then just be sure to "ignore" / handle specific named exceptions, never WHEN OTHERS.
If the error should not be ignored, then you will want to log it and continue.
I have several places in the backend code for the Oracle Dev Gym in which something went wrong (for example, an error occurred when trying to send an email verification of some step taken), but I do not want to stop processing, I do not want to bother the user. So I log the error, but allow execution to continue.
Here's the exception section for my send_mail procedure:
EXCEPTION WHEN OTHERS THEN qdb_utilities.syslog_warning ('SEND_EMAIL', 'Unable to send email ID', email_id_in); END send_mail;
I post a warning to my log, and let things go on as normal.
Which brings me to my final point for this section: if you are not already using a standard error logging API (log error, log warning, log information, trace execution, etc.), please put in one place ASAP. For the Dev Gym, we use our own qdb_utilities package. But I suggest you check out the open source logger utility. Used by thousands around the world, very robust, supported by some really excellent developers. No reason to build your own!
Worse-than useless re-raising of an error
OK, so you have an exception handler, you don't just let the exception escape the block. But inside that handler all you do is:
EXCEPTION WHEN OTHERS THEN RAISE; END;
This is worse than doing nothing at all. It doesn't add value. It actually makes it harder to find the cause of the exception.
The reason is that at some as this exception propagates to outer blocks, I sincerely hope that you do trap and log the error. At which point, the logging procedure should call the DBMS_UTILIY.FORMAT_ERROR_BACKTRACE procedure, which will give you the line number on which the exception was raised. But the backtrace only traces back to the most recent raise. So when you re-raise an exception, you lose that "original" line number.
So it's OK to re-raise an exception, just log first, so you can grab and store the backtrace.
EXCEPTION WHEN OTHERS THEN logger.log_error (...); RAISE; END;
This is worse than doing n
Well, I could probably go on and on, but this should be enough to give you some solid tips for writing better PL/SQL code - and inspire my readers to offer their own additions to the list.