-1

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?

Erwin Brandstetter
  • 605,456
  • 145
  • 1,078
  • 1,228
picmate 涅
  • 3,951
  • 5
  • 43
  • 52
  • 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 http://codereview.stackexchange.com/. – Kevin May 04 '15 at 15:44
  • 1
    @Kevin: You are right, I added the problem statement the OP forgot. I can tell from the previous question. – Erwin Brandstetter May 04 '15 at 16:02

1 Answers1

2

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:

    Using the hstore(text[], text[]) variant here to construct an hstore value with multiple fields on the fly.

  • The assignment operator in plpgsql is :=:

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

Community
  • 1
  • 1
Erwin Brandstetter
  • 605,456
  • 145
  • 1,078
  • 1,228
  • Erwin, I still don't get the inserts and updates updated with the mod_op and mod_dates. Do you know why that is? Anything wrong with my setup? – picmate 涅 May 04 '15 at 16:21
  • 1
    @picmate: Have you seen the fiddle? It works. I have used similar techniques many times. Do you have other triggers for the same event on the same table that might interfere? Or hard-coded table names accidentially? Or maybe you did not get `CREATE TRIGGER` right? I added it to the answer. Also note: `mod_datetime`! I added a note. – Erwin Brandstetter May 04 '15 at 16:30
  • Thanks Erwin. This is awesome. The problem was initializing the trigger as after. Changed it to before and is now working fine. Unfortunately, I am not in a position to add extra modules to the production environment (where the code will eventually go). Therefore, had to refrain from using hstore. But, got a lot of insight into the process from your answers. – picmate 涅 May 04 '15 at 17:36
  • @picmate; `hstore` is such a basic, powerful module, it really should be installed most of the time ... – Erwin Brandstetter May 04 '15 at 17:46
  • I will definitely check and if not installed suggest that to our ops guys. If they agree with me they will do the addition, which will let me retain the code you suggested. – picmate 涅 May 04 '15 at 17:51