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>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.
+ Note that if a trigger has multiple 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 ...])
execute procedure 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();
CREATE TRIGGER
postgres=# COPY t1 FROM stdin;
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself.
>> 2 bbb
>> 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: 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 optimized out>, lineNumber=<value optimized out>) at assert.c:57
#4 0x081956d9 in TriggerEnabled (trigger=<value optimized out>, event=<value optimized out>,
modifiedCols=<value optimized out>, estate=<value optimized out>, tupdesc=<value optimized out>,
oldtup=<value optimized out>, newtup=<value optimized out>) at trigger.c:2410
#5 0x08196410 in ExecBRInsertTriggers (estate=<value optimized out>, relinfo=<value optimized out>,
trigtuple=<value optimized out>) at trigger.c:1835
#6 0x08162e0e in CopyFrom (cstate=<value optimized out>) at copy.c:2137
#7 DoCopy (cstate=<value optimized out>) at copy.c:1189
#8 0x0827c653 in ProcessUtility (parsetree=<value optimized out>, queryString=<value optimized out>,
params=<value optimized out>, isTopLevel=<value optimized out>, dest=<value optimized out>,
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=<value optimized out>)
at pquery.c:1192
#10 0x0827a227 in PortalRunMulti (portal=0x94a0e4c, isTopLevel=<value optimized out>,
dest=<value optimized out>, altdest=<value optimized out>, completionTag=<value optimized out>)
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=<value optimized 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=<value optimized 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 optimized out>) 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 assigned on 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(at)ak(dot)jp(dot)nec(dot)com>