Endless loop in trigger function

Question

This is a trigger that is called by either an insert, update or a delete on a table. It is guaranteed the calling table has all the columns impacted and a deletes table also exists.

CREATE OR REPLACE FUNCTION sample_trigger_func() RETURNS TRIGGER AS $$
DECLARE
    operation_code char;
    table_name varchar(50);
    delete_table_name varchar(50);
    old_id integer; 

BEGIN
table_name = TG_TABLE_NAME;
delete_table_name = TG_TABLE_NAME || '_deletes';

SELECT SUBSTR(TG_OP, 1, 1)::CHAR INTO operation_code;

IF TG_OP = 'DELETE' THEN
    OLD.mod_op = operation_code;
    OLD.mod_date = now();

    RAISE INFO 'OLD: %', (OLD).name;

    EXECUTE format('INSERT INTO %s VALUES %s', delete_table_name, (OLD).*);

ELSE
    EXECUTE format('UPDATE TABLE %s SET mod_op = %s AND mod_date = %s'
                  , TG_TABLE_NAME, operation_code, now());
END IF;

RETURN NEW;
END;

$$ LANGUAGE plpgsql;

The ELSE branch triggers an endless loop. There may be more problems. How to fix it?

I'm voting to close this question as off-topic because there is no problem statement and it looks as if OP is asking for a code review. The question might be on-topic at codereview.stackexchange.com. — Kevin
@Kevin: You are right, I added the problem statement the OP forgot. I can tell from the previous question. — Erwin Brandstetter

Erwin Brandstetter Erwin Brandstetter · Accepted Answer · 2015-05-04T15:53:33

The ELSE branch can be radically simplified. But a couple more things are inefficient / inaccurate / dangerous:

CREATE OR REPLACE FUNCTION sample_trigger_func()
  RETURNS TRIGGER AS
$func$
BEGIN
   IF TG_OP = 'DELETE' THEN
      RAISE INFO 'OLD: %', OLD.name;

      EXECUTE format('INSERT INTO %I SELECT ($1).*', TG_TABLE_NAME || '_deletes')
      USING OLD #= hstore('{mod_op, mod_datetime}'::text[]
                         , ARRAY[left(TG_OP, 1), now()::text]);
      RETURN OLD;
   ELSE  -- insert, update
      NEW.mod_op       := left(TG_OP, 1);
      NEW.mod_datetime := now();

      RETURN NEW;
   END IF;
END
$func$  LANGUAGE plpgsql;

In the ELSE branch just assign to NEW directly. No need for more dynamic SQL - which would fire the same trigger again causing an endless loop. That's the primary error.
RETURN NEW; outside the IF construct would break your trigger function for DELETE, since NEW is not assigned for DELETEs.
A key feature is the use of hstore and the hstore operator #= to dynamically change two selected fields of the well-known row type - that is unknown at the time of writing the code. This way you do not tamper with the original OLD value, which might have surprising side effect if you have more triggers down the chain of events.
```
OLD #= hstore('{mod_op, mod_datetime}'::text[]
             , ARRAY[left(TG_OP, 1), now()::text]);
```
The additional module hstore must be installed. Details:
- How to set value of composite variable field using dynamic SQL
- Passing column names dynamically for a record variable in PostgreSQL
Using the hstore(text[], text[]) variant here to construct an hstore value with multiple fields on the fly.
The assignment operator in plpgsql is :=:
- The forgotten assignment operator "=" and the commonplace ":="
Note that I used the column name mod_datetime instead of the misleading mod_date, since the column is obviously a timestamp and not a date.

I added a couple of other improvements while being at it. And the trigger itself should look like this:

CREATE TRIGGER insupdel_bef
BEFORE INSERT OR UPDATE OR DELETE ON table_name
FOR EACH ROW EXECUTE PROCEDURE sample_trigger_func();

SQL Fiddle.

Endless loop in trigger function

1 Answers