Code You Should Never See in PL/SQL

If you ever run across any of the following, apply the suggested cleanup, or contact the owner of the code, or run for the hills.

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.

See below for the gory details.

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.

Bad Code

DECLARE
   l_number NUMBER := NULL;
   /* Same applies to using alternative syntax */
   l_date   DATE DEFAULT NULL;
BEGIN
   ...
END;

Cleaned Up

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:

Bad Code

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;

Cleaned Up

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:

Bad Code

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;
/

Cleaned Up

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!

Bad Code

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.

Cleaned Up

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:

Bad Code

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.

Cleaned Up

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.

Hiding errors

(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.

So what you should never do is something along these lines:

Bad Code

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'.

Cleaned Up

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:

Bad Code

   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.

Cleaned Up

   EXCEPTION
      WHEN OTHERS
      THEN
         logger.log_error (...);
         RAISE;
   END;

This is worse than doing n

Whew

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.