Re: TRIGGER with WHEN clause

Itagaki-san, I checked the latest patch.

It seems to me the patch getting improved from the prior version.
We are next to the commiter review phase.

But I could find a few matters. :-(
Please see the following comments, and fix it before sending it
to commiters.

> I fixed the bug and two other bugs:
>   * Crash in AFTER TRIGGER + WHEN clause.
>   * Incorrect behavior when multiple tuples are modified.
> Also regression tests for it are added.

It looks for me fine.

> I'd like to add support statement triggers with WHEN clause.
> Of cource Oracle is a good example, but it doesn't prevent us
> from developing a better copy :)

OK, it is your decision.

>> * the documentation seems to me misleading.
>> 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.
> 
> They are bitwise-AND flags. INSERT-OR-DELETE trigger cannot refer to both
> OLD and NEW tuples. It is possible to use a dummy tuple (filled with NULLs?)
> in the cases, but I want to just throw an error as of now. I'll fix
> documentation to reflect the code. Ideas for better descriptions welcome.
> 
> | Note that if a trigger has multiple events, it can refer only tuples
> | that can be referred in all of the events. For example,
> | INSERT OR DELETE trigger cannot refer neither NEW nor OLD tuples.

At least, it seems to me meaningful.
Is there any comments from native English users?
      <varlistentry> +     <term><replaceable class="parameter">condition</replaceable></term> +     <listitem> +
<para>+       Any <acronym>SQL</acronym> conditional expression (returning +       <type>boolean</type>). Only
<literal>FOREACH 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. +       Note that if a trigger
hasmultiple events, it can refer only tuples +       that can be referred in all of the events. For example, +
<literal>INSERT</><literal>OR</> <literal>DELETE</> trigger cannot +       refer neither <literal>NEW</> nor
<literal>OLD</>tuples. +      </para> +     </listitem> +    </varlistentry>
 


In addition, I could find a few matters.

* TOAST may be necessary for pg_trigger?
----------------------------------------
If we give very looooooong condition on the WHEN clause, the pg_trigger.tgqual
can over the limitation of block size.
 postgres=# CREATE TRIGGER hoge before insert on t1 for row        when (a < [... very long condition ...])
executeprocedure trig_func(); ERROR:  row is too big: size 12940, maximum size 8164
 

But it is a quite corner case in my opinion. It depends on your preference.


* ROW INSERT TRIGGER on COPY FROM statement
-------------------------------------------
The Assert() in TriggerEnabled (commands/trigger.c:2410) was mistaken bombing.
In the code path from copy.c, NULL can be set on the estate->es_trig_tuple_slot.
 postgres=# CREATE TRIGGER tg_ins_row BEFORE INSERT ON t1 FOR ROW         WHEN (true) EXECUTE PROCEDURE trig_func();
CREATETRIGGER postgres=# COPY t1 FROM stdin; Enter data to be copied followed by a newline. End with a backslash and a
periodon a line by itself. >> 2    bbb >> server closed the connection unexpectedly         This probably means the
serverterminated abnormally         before or while processing the request. The connection to the server was lost.
Attemptingreset: Succeeded.
 
 (gdb) bt #0  0x00cd4416 in __kernel_vsyscall () #1  0x009beba1 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64#2  0x009c046a in abort () at abort.c:92 #3  0x08330e7e in
ExceptionalCondition(conditionName=<value optimized out>, errorType=<value optimized out>,     fileName=<value
optimizedout>, lineNumber=<value optimized out>) at assert.c:57 #4  0x081956d9 in TriggerEnabled (trigger=<value
optimizedout>, event=<value optimized out>,     modifiedCols=<value optimized out>, estate=<value optimized out>,
tupdesc=<valueoptimized out>,     oldtup=<value optimized out>, newtup=<value optimized out>) at trigger.c:2410 #5
0x08196410in ExecBRInsertTriggers (estate=<value optimized out>, relinfo=<value optimized out>,     trigtuple=<value
optimizedout>) at trigger.c:1835 #6  0x08162e0e in CopyFrom (cstate=<value optimized out>) at copy.c:2137 #7  DoCopy
(cstate=<valueoptimized out>) at copy.c:1189 #8  0x0827c653 in ProcessUtility (parsetree=<value optimized out>,
queryString=<valueoptimized out>,     params=<value optimized out>, isTopLevel=<value optimized out>, dest=<value
optimizedout>,     completionTag=<value optimized out>) at utility.c:581 #9  0x0827931d in PortalRunUtility
(portal=0x94a0e4c,utilityStmt=0x944133c,     isTopLevel=<value optimized out>, dest=<value optimized out>,
completionTag=<valueoptimized out>)     at pquery.c:1192 #10 0x0827a227 in PortalRunMulti (portal=0x94a0e4c,
isTopLevel=<valueoptimized out>,     dest=<value optimized out>, altdest=<value optimized out>, completionTag=<value
optimizedout>)     at pquery.c:1299 #11 0x0827aa64 in PortalRun (portal=<value optimized out>, count=<value optimized
out>,    isTopLevel=<value optimized out>, dest=<value optimized out>, altdest=<value optimized out>,
completionTag=<valueoptimized out>) at pquery.c:823 #12 0x08276d80 in exec_simple_query (query_string=<value optimized
out>)at postgres.c:1046 #13 0x08277fd3 in PostgresMain (argc=<value optimized out>, argv=<value optimized out>,
username=<valueoptimized out>) at postgres.c:3624 #14 0x08241228 in BackendRun (port=<value optimized out>) at
postmaster.c:3366#15 BackendStartup (port=<value optimized out>) at postmaster.c:3073 #16 ServerLoop (port=<value
optimizedout>) at postmaster.c:1399 #17 0x08243c68 in PostmasterMain (argc=<value optimized out>, argv=<value optimized
out>)at postmaster.c:1064 #18 0x081e3d5f in main (argc=<value optimized out>, argv=0x93c5b38) at main.c:188
 
 2407      if (TRIGGER_FIRED_BY_INSERT(event) || TRIGGER_FIRED_BY_UPDATE(event)) 2408      { 2409
Assert(newtup!= NULL); 2410          Assert(estate->es_trig_tuple_slot != NULL);  <---- (*) 2411 2412          newslot
=estate->es_trig_tuple_slot; 2413          if (newslot->tts_tupleDescriptor != tupdesc) 2414
ExecSetSlotDescriptor(newslot,tupdesc); 2415          if (newslot->tts_tuple != newtup) 2416
ExecStoreTuple(newtup,newslot, InvalidBuffer, false); 2417      }
 


* Using system column in WHEN clause
------------------------------------
If we use references to the system columns in the WHEN clause, its result may
be unexpected one from user's perspective.
 postgres=# CREATE TRIGGER tg_upd_row BEFORE UPDATE ON t1 FOR ROW         WHEN (OLD.oid != NEW.oid) EXECUTE PROCEDURE
trig_func();CREATE TRIGGER postgres=# UPDATE t1 SET b = b; NOTICE:  old=(1,aaa) new=(1,aaa) NOTICE:  old=(2,bbb)
new=(2,bbb)NOTICE:  old=(3,ccc) new=(3,ccc) UPDATE 3
 

From develope's perspective, it is quite natural because we know "oid" is
set within heap_update(), so these identifiers still have InvalidOid when
triggers are invoked.

I think we have two options for the matter:

1) Set correct values on the "oid" just before trigger invocations.  (However, what values should be visible for
"tableoid","xmin",   "ctid" and others? Is it really reasonable?)
 

2) Describe a notice on the user documentation not to use system columns  in the WHEN clause, because these are
assignedon after the trigger  invocations.
 

Here was a similar issue on the discussion related to suppress_redundant_updates_trigger().
See the following thread: http://archives.postgresql.org/pgsql-hackers/2008-11/thrd7.php#00273

In this case, we put an expected oid prior to comparison.
However, note that it is a special purpose function to avoid nonsense
updating, so this kind of workaround performs fine.
In my opinion, the (2) is more reasonable solution.

Thanks,
-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>