Re: TRIGGER with WHEN clause

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 closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the 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=<value optimized out>,
numberTuples=<value optimized out>, sendTuples=<value optimized out>,
operation=<value optimized 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=<value optimized out>)
at execMain.c:278
#7 0x0827a016 in ProcessQuery (plan=<value optimized out>, sourceText=<value optimized 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 optimized out>, 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 0x08277f43 in 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 NEW
o UPDATE can refer NEW, OLD
o 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 PROCEDURE trig_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(at)kaigai(dot)gr(dot)jp>