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, than 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.

Comments

  1. Bad practice: Run a count first, before running a query.

    e.g.
    [code]
    select count(*)
    into v_count
    from orders
    where customer_id = v_customer;

    if v_count > 0 then
    for o in (select * from orders where customer_id = v_customer)
    loop
    -- do_something here

    end loop;
    end if;
    [/code]

    better:
    [code]
    for o in (select * from orders where customer_id = v_customer)
    loop
    -- do_something here

    end loop;
    [/code]

    ReplyDelete
  2. Hi Steven,

    Thank you Steven for these good and bad code examples.

    Some of these bad code can be detected automatically. Within Oracle SQL Developer by the Trivadis PL/SQL Cop extension (https://www.salvis.com/blog/plsql-cop-for-sql-developer/).

    This extension in free and checks the code against the Trivadis SQL & PL/SQL Guidelines. You may configure the guidelines to be checked within the SQL Developer preferences. It’s even possible to include custom guidelines.

    The guidelines are open sourced at https://github.com/Trivadis/plsql-and-sql-coding-guidelines. Everyone is invited to contribute.

    So what about your code examples?

    Set default value to NULL
    ------------------------------
    G-2140: Never initialize variables with NULL.
    https://trivadis.github.io/plsql-and-sql-coding-guidelines/4-language-usage/2-variables-and-types/1-general/g-2140/

    Select from DUAL for....just about anything
    -------------------------------------------------
    G-7810: Do not use SQL inside PL/SQL to read sequence numbers (or SYSDATE)
    https://trivadis.github.io/plsql-and-sql-coding-guidelines/4-language-usage/7-stored-objects/8-sequences/g-7810/

    Declaration of FOR loop iterator
    ------------------------------------
    (not covered yet)

    WHEN NO_DATA_FOUND for non-query DML
    -----------------------------------------------------
    (not covered yet), but G-1040: Avoid dead code.
    https://trivadis.github.io/plsql-and-sql-coding-guidelines/4-language-usage/1-general/g-1040/

    Loop containing non-query DML inside
    ---------------------------------------------
    G-3210: Always use BULK OPERATIONS (BULK COLLECT, FORALL) whenever you have to execute a DML statement more than 4 times.
    https://trivadis.github.io/plsql-and-sql-coding-guidelines/4-language-usage/3-dml-and-sql/2-bulk-operations/g-3210/

    I hope that’s helpful.

    Cheers,
    Philipp

    ReplyDelete
  3. Here's one I see I lot:

    BEGIN
    SELECT COUNT(*)
    INTO my_variable
    FROM some_table
    WHERE ...
    EXCEPTION
    WHEN NO_DATA_FOUND THEN
    my_variable := 0;
    END;

    The people who write such code don't know that aggregate functions always return a value, even if no rows are retrieved, so the code in the exception handler never runs.

    ReplyDelete
  4. Great topic for newcomers! I'd inly say that the first tip could be kind of confusing. I mean, maybe it's a good idea to point out the fact that it's ok to use `default null` for procedure/function parameters, but not for global and local variables in declaration sections.

    ReplyDelete
    Replies
    1. Denis, sorry I did not reply sooner. I don't understand your concern. Please elaborate.

      Delete
    2. Presumably for optional plsql parameters, which you don't want to always to pass in an explicit null.

      Delete
    3. Exactly. The first point was about setting default value of variables to NULL. This is surely a bad habit with one exception - setting default value to NULL is very useful for parameters of a function or a procedure.

      And my concern was about the fact that people who read your blog post could generalize and will try to avoid `default null` everywhere without thinking - so I suggested to point out this case deliberately.

      Delete
    4. Thanks, I will add a point about that. My tip talks about variables, not parameters, but sure I don't want anyone to be confused.

      Delete
  5. Steven, in cleaned up code "Loop containing non-query DML inside" should't you use associative array?

    ReplyDelete
    Replies
    1. Ok, probably you forgot put line : l_parts.EXTEND(10000);

      Delete
    2. Ha! Thanks for that. Will fix my code.

      Delete
  6. In your Never set default value = NULL part, I disagree with your example. I do agree that it automatically defaults to null, but your example referenced a numeric datatype. My standard rule is if that number is going to be in calculations, you almost always default the value to 0 not NULL.

    ReplyDelete
    Replies
    1. Huh? Always set to 0? Why? Do you think that is the numeric equivalent of NULL. I suggest this is a VERY bad idea.

      Delete
    2. It does help to have the variable in anonymous block set to "null" or "o", for testing purposes. We can use the nvl to switch to generalise the testing.

      declare
      l_val number := 10; --testing for single value

      begin
      select * from tab_name when col1 in (nvl(l_val, select val from tab2));
      end;

      /***************/

      declare
      l_val number := null; --testing for set of values

      begin
      select * from tab_name when col1 in (nvl(l_val, select val from tab2));
      end;

      Just thought to share something I learned. Let me know if the concept is faulty.

      Thanks.:)

      Delete
    3. I agree that when putting together test scripts, isolating values that will change into variables is a great idea (actually could be a constant even!).

      And if you want to document that you have a test case with NULL as the input, great. Be explicit.

      As a general rule, though, I don't think it's a good idea to attach := NULL to all your declarations.

      Delete
  7. (Mis)usage of functions or procedures - something I often notice: people aren't certain about when to use what.....

    for example:

    ex 1:

    function do_something return boolean is
    begin
    [....]
    return true;
    exception
    when others then
    return false;
    end;

    leads to funny code blocks like

    declare
    ok boolean;
    begin
    ok := do_something;
    if ok then
    ok := do_something_else;
    end if;
    --
    if ok then
    ok := do_something_completely_else;
    end if;
    [...]

    (similarities to https://esolangs.org/wiki/Ook! come to my mind...anyway)

    or 7 levels of nested calls without the boolean variable - I guess you know what I mean.

    if as a caller you want to know what exactly went wrong - no problemo, here's another version which is even better:

    ex 2:

    function do_something(ret_code number, ret_msg out varchar2) return boolean is
    begin
    [...]
    return true;
    exception
    when others then
    ret_code := sqlcode;
    ret_msg := sqlerrm;
    return false;
    end;

    If I want to e.g. do something special when a TOO_MANY_ROWS happens in the called procedure - just check the out variable for the procedure if it is 1422. Very readable and understandable at first sight! If you don't happen to memorize all exception numbers - no problem as well - there are constants for you ;-).


    And of course there's this beauty:

    ex 3:

    procedure get_something(some_val in number, outval1 out number, outval2 out number, outval3 out number, [.....] outval123456 out number) is
    [.....]

    Apart from the obvious flaws this is particularly funny if by any chance you are only interested in only one of the out parameters which will inevitably lead to code like this:

    declare
    dummy number;
    variable_that_interests_me number;
    begin
    get_something(1, dummy, dummy, dummy, [... now on what exact position is my variable], variable_that_interests_me, dummy, dummy, dummy, [...]

    Instead of

    - Use a Procedure, no exception handler in ex 1 & 2
    - Use a Function that returns a record type in ex 3

    cheers

    ReplyDelete
  8. In addition to the declaration of the for loop variable, I do not like code where a for loop is done and exited after the first row. Imo, you should code an open-fetch-close block. As well as in the case of the select-into ignoring the no-data-found.

    ReplyDelete
    Replies
    1. I agree - though I find this pattern over and over again. I've even recently seen a CFL with a RETURN inside it. Found a row? Great, done!

      It's a quick and lazy way to write your code but the misuse of the control statement and in the above case the embedding of a return makes it confusing to read and understand.

      Delete
  9. I also would use an open fetch close in stead of an select-into if I expect a no-data-found. Because the %found variable is meant to functionally check if I found a row. I don't like to rely on a no-data-found exception if it is functionally not really an exception and I have means to handle it in a logical way.

    ReplyDelete
  10. How about functions with out parameters and procedures with only one out parameters?
    And multiple return statements in a function, specifically in nested if constructs? In my opinion a function should have one and only one return statement, and it should be the last statement in the code. For multiple conditional return values, I'd use a result variable.

    ReplyDelete
    Replies
    1. I don't have a problem with a procedure with a single out parameter. It may be doing a whole bunch of different things and then return a value.

      I don't see a function as "the thing you call when you return a single value".

      I see it more as: the thing you call when you want to get something back:".

      Delete
  11. Thanks, Martien and others, for your excellent suggestions! As soon as I have time to go through them in detail, I will move at least some of them "up" into the post.

    ReplyDelete

Post a Comment

Popular posts from this blog

Table Functions, Part 1: Introduction and Exploration

Quick Guide to User-Defined Types in Oracle PL/SQL

Recommendations for unit testing PL/SQL programs