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.

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 in bright lights).

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;
BEGIN
   ...
END;

Cleaned Up

Just remove the assignment.

DECLARE
   l_number NUMBER;
BEGIN
   ...
END;

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 

The first step in the cleanup 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
   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.

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.