Re: TRIGGER with WHEN clause - Mailing list pgsql-hackers

KaiGai Kohei wrote:
> Itagaki Takahiro wrote:
>> Here is a updated TRIGGER with WHEN cluase patch.
>> I adjusted it to work with recent parser cleanup (*OLD* and *NEW*).
> 
> I would like to volunteer for reviewing the patch in RR-stage.
> It seems to me an interesting feature.

It seems to me you did an impressive work.
However, I could find a few matters in this patch, as follows:

* It does not prevent set up a conditional "statement" trigger.

I'm uncertain how Oracle handles the condition on the statement
triggers. But it seems to me WHEN clause on the statement triggers
are nonsense.
If you have any opposition, the CreateTrigger() should raise an error
when a valid stmt->whenClause is given for statement triggers.

In addition, it makes a server crash. :-)
 postgres=# CREATE or replace function trig_func() returns trigger         language plpgsql as         'begin raise
notice''trigger invoked''; return null; end'; CREATE FUNCTION postgres=# CREATE TRIGGER t1_trig BEFORE update ON t1
   WHEN (true) EXECUTE PROCEDURE trig_func();         ^^^^^^^^^^^ CREATE TRIGGER postgres=# update t1 set b = b; server
closedthe connection unexpectedly         This probably means the server terminated abnormally         before or while
processingthe request. The connection to the server was lost. Attempting reset: Failed.
 

The core dump says:
 (gdb) bt #0  0x08195396 in TriggerEnabled (trigger=0x9c7a0c4, event=10, modifiedCols=0x9c484bc, estate=0x0,
tupdesc=0x0,oldtup=0x0, newtup=0x0) at trigger.c:2376 #1  0x08196783 in ExecBSUpdateTriggers (estate=0x9c79f44,
relinfo=0x9c79fec)at trigger.c:2040 #2  0x081cd4e9 in fireBSTriggers (node=<value optimized out>) at
nodeModifyTable.c:593#3  ExecModifyTable (node=<value optimized out>) at nodeModifyTable.c:657 #4  0x081b70b8 in
ExecProcNode(node=0x9c7a190) at execProcnode.c:359 #5  0x081b5c0d in ExecutePlan (dest=<value optimized out>,
direction=<valueoptimized out>,     numberTuples=<value optimized out>, sendTuples=<value optimized out>,
operation=<valueoptimized out>, planstate=<value optimized out>, estate=<value optimized out>)     at execMain.c:1189
#6 standard_ExecutorRun (dest=<value optimized out>, direction=<value optimized out>,     numberTuples=<value optimized
out>,sendTuples=<value optimized out>,     operation=<value optimized out>, planstate=<value optimized out>,
estate=<valueoptimized out>)     at execMain.c:278 #7  0x0827a016 in ProcessQuery (plan=<value optimized out>,
sourceText=<valueoptimized out>,     params=<value optimized out>, dest=0x9c72024, completionTag=0xbfb8a51e "") at
pquery.c:196#8  0x0827a24e in PortalRunMulti (portal=0x9beabec, isTopLevel=<value optimized out>,     dest=<value
optimizedout>, altdest=0x9c72024, completionTag=0xbfb8a51e "") at pquery.c:1269 #9  0x0827a9d4 in PortalRun
(portal=0x9beabec,count=2147483647, isTopLevel=36 '$', dest=0x9c72024,     altdest=0x9c72024, completionTag=0xbfb8a51e
"")at pquery.c:823 #10 0x08276cf0 in exec_simple_query (query_string=<value optimized out>) at postgres.c:1046 #11
0x08277f43in PostgresMain (argc=2, argv=0x9bcfb70, username=0x9bcfb40 "kaigai") at postgres.c:3624 #12 0x08241198 in
BackendRun(port=<value optimized out>) at postmaster.c:3366 #13 BackendStartup (port=<value optimized out>) at
postmaster.c:3073#14 ServerLoop (port=<value optimized out>) at postmaster.c:1399 #15 0x08243bd8 in PostmasterMain
(argc=1,argv=0x9bcda18) at postmaster.c:1064 #16 0x081e3d5f in main (argc=1, argv=0x9bcda18) at main.c:188
 
 2368      /* Check for WHEN clause */ 2369      if (trigger->tgqual) 2370      { 2371          ExprContext
*econtext;2372          List               *predicate; 2373          TupleTableSlot     *oldslot = NULL; 2374
TupleTableSlot    *newslot = NULL; 2375 2376 *        econtext = GetPerTupleExprContext(estate); 2377 2378
predicate= list_make1(ExecPrepareExpr((Expr *) trigger->tgqual, estate)); 2379
 


* the documentation seems to me misleading.
    <varlistentry>
+     <term><replaceable class="parameter">condition</replaceable></term>
+     <listitem>
+      <para>
+       Any <acronym>SQL</acronym> conditional expression (returning
+       <type>boolean</type>). Only <literal>FOR EACH ROW</literal> triggers
+       can refer <literal>NEW</> and <literal>OLD</> tuples.
+       <literal>INSERT</literal> trigger can refer <literal>NEW</>,
+       <literal>DELETE</literal> trigger can refer <literal>OLD</>,
+       and <literal>UPDATE</literal> trigger can refer both of them.
+      </para>
+     </listitem>
+    </varlistentry>

It saids, NEW and OLD are only available and ...o INSERT can refer NEWo UPDATE can refer NEW, OLDo DELETE can refer
OLD

But, it may actually incorrect, if user gives several events on a certain
trigger. For example, when a new trigger is invoked for each row on INSERT
or UPDATE statement, the function cannot refer the OLD.

+       if (TRIGGER_FOR_ROW(tgtype))
+       {
+           RangeTblEntry *rte;
+
+           if ((TRIGGER_FOR_DELETE(tgtype) || TRIGGER_FOR_UPDATE(tgtype)) &&
+               !TRIGGER_FOR_INSERT(tgtype))
+           {
+               rte = addRangeTableEntryForRelation(pstate, rel,
+                                       makeAlias("old", NIL), false, false);
+               rte->requiredPerms = 0;
+               addRTEtoQuery(pstate, rte, false, true, true);
+           }
+
+           if ((TRIGGER_FOR_INSERT(tgtype) || TRIGGER_FOR_UPDATE(tgtype)) &&
+               !TRIGGER_FOR_DELETE(tgtype))
+           {
+               rte = addRangeTableEntryForRelation(pstate, rel,
+                                       makeAlias("new", NIL), false, false);
+               rte->requiredPerms = 0;
+               addRTEtoQuery(pstate, rte, false, true, true);
+           }
+       }

I don't think the code is correct, but the SGML documentation does not reflect
the behavior correctly.

postgres=# CREATE TRIGGER t1_trig BEFORE update OR insert ON t1 FOR EACH ROW       WHEN (OLD.a % 2 = 1) EXECUTE
PROCEDUREtrig_func();
 
ERROR:  missing FROM-clause entry for table "old"
LINE 1: ... BEFORE update OR insert ON t1 FOR EACH ROW WHEN (OLD.a % 2 ...


* A minor coding style
 *************** CreateTrigger(CreateTrigStmt *stmt, *** 387,392 **** --- 453,469 ----     tgattr =
buildint2vector(columns,ncolumns);     values[Anum_pg_trigger_tgattr - 1] = PointerGetDatum(tgattr);
 
 +   /* set tgqual if trigger has WHEN clause */ +   if (qual) +   { +       values[Anum_pg_trigger_tgqual - 1] =
CStringGetTextDatum(qual);+   } +   else +   { +       values[Anum_pg_trigger_tgqual - 1] = InvalidOid; +
nulls[Anum_pg_trigger_tgqual- 1] = true; +   } +     tuple = heap_form_tuple(tgrel->rd_att, values, nulls);
 

Is it unnecessary to set InvalidOid on the values[Anum_pg_trigger_tgqual - 1]?

* doc/src/sgml/catalogs.sgml is not updated

Could you add a short description about pg_trigger.tgqual system catalog?


Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>