TRIGGER with WHEN clause

Here is a patch to provide WHEN clause on triggers discussed here:
http://archives.postgresql.org/pgsql-hackers/2009-10/msg00869.php

Of course we can check conditions in the body of triggers, but
there are some benefits to have separate WHEN clause for SQL standard
compliance, portability, and saving AFTER trigger queues.

There might be still incomplete and debatable part in the patch:

* Usages of TupleTableSlot and RangeTblEntry might be ugly.
Please notice me if there are better ways.

* There are no "retry" feature to evaluate condition when
the NEW tuple is modifed by another trigger. Triggers are
still executed in alphabetical order, but it is adjustable.

Comments welcome.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Here is a updated TRIGGER with WHEN cluase patch.
I adjusted it to work with recent parser cleanup (*OLD* and *NEW*).

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

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.

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

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.

Sorry, I forgot to Cc: pgsql-rrreviewers.

I'll be available to volunteer to review his "TRIGGER with WHEN clause"
patch in RR-stage.

Any comments?
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

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.
>
That one is now yours. It would be nice if you subscribed to the
pgsql-rrreviewers list if you're not already on there, but it's not
really necessary if you just want to grab that one patch and report on
it to pgsql-hackers.

Dimitri Fontaine had expressed some interest in that one as well.
Dimitri, since KaiGai wants specifically to take a shot at that one, I'm
going to assign you the "Syntax for partitioning" patch you said was
your second choice instead.

--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.com

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>

Thank for your reviewing!

KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:

> However, I could find a few matters in this patch, as follows:
>
> * It does not prevent set up a conditional "statement" trigger.

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.

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

I am also not sure about Oracle, but I think there are usage of
statement trigger with WHEN cluase something like:
=# CREATE TRIGGER log_trig BEFORE UPDATE ON tbl
WHEN (is_superuser()) EXECUTE PROCEDURE log_current_stmt();

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

> * A minor coding style
> Is it unnecessary to set InvalidOid on the values[Anum_pg_trigger_tgqual - 1]?

Strange. There was something wrong with me.

> * doc/src/sgml/catalogs.sgml is not updated
> Could you add a short description about pg_trigger.tgqual system catalog?

Added.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

KaiGai Kohei wrote:
> 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.

SQL> CREATE TRIGGER dummy BEFORE DELETE ON employees WHEN (1 = 1)
2 BEGIN
3 END;
4 /
CREATE TRIGGER dummy BEFORE DELETE ON employees WHEN (1 = 1)
*
ERROR at line 1:
ORA-04077: WHEN clause cannot be used with table level triggers

Yours,
Laurenz Albe

Albe Laurenz wrote:
> SQL> CREATE TRIGGER dummy BEFORE DELETE ON employees WHEN (1 = 1)
> 2 BEGIN
> 3 END;
> 4 /
> CREATE TRIGGER dummy BEFORE DELETE ON employees WHEN (1 = 1)
> *
> ERROR at line 1:
> ORA-04077: WHEN clause cannot be used with table level triggers

Thanks for your information.

> I am also not sure about Oracle, but I think there are usage of
> statement trigger with WHEN cluase something like:
> =# CREATE TRIGGER log_trig BEFORE UPDATE ON tbl
> WHEN (is_superuser()) EXECUTE PROCEDURE log_current_stmt();

Itagaki-san, I also think your example usage is enough valueable.
However, Oracle does not have the feature apparently, although the
purpose of this patch is to provide a compatible feature, IIRC.

I don't have any preference on either of them.
If you make a decision, I'll review the patch according to your
decision. So, I like to ask you which is your preference again.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:

> Itagaki-san, I also think your example usage is enough valueable.
> However, Oracle does not have the feature apparently, although the
> purpose of this patch is to provide a compatible feature, IIRC.
>
> I don't have any preference on either of them.
> If you make a decision, I'll review the patch according to your
> decision. So, I like to ask you which is your preference again.

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 :)

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

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>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:

> In addition, I could find a few matters.
> * TOAST may be necessary for pg_trigger?

I added toast relation to pg_trigger.
DECLARE_TOAST(pg_trigger, 2336, 2337);

I think having a toast relation for pg_trigger is reasonable
because pg_trigger already has a variable field "tgargs"
even if we don't have the new field "tgqual" from the patch.
I'm not sure why we don't have a toast relation for pg_trigger
because user might pass very long trigger arguments.

> * ROW INSERT TRIGGER on COPY FROM statement

Thanks. Good catch! Fixed and regression test added.

> * Using system column in WHEN clause
> 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.

I'd like to only add documentation because I don't have a whole solution.
----
System columns are not available in the <literal>WHEN</> clause
because those values are initialized after triggers are called.
They might return wrong values if they used in expressions of the clause.
----

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Itagaki-san,

I don't have any more comments in this patch, so I hope it to be reviewed
by committers then upstreamed.

Thanks for your good jobs.

Itagaki Takahiro wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>
>> In addition, I could find a few matters.
>> * TOAST may be necessary for pg_trigger?
>
> I added toast relation to pg_trigger.
> DECLARE_TOAST(pg_trigger, 2336, 2337);
>
> I think having a toast relation for pg_trigger is reasonable
> because pg_trigger already has a variable field "tgargs"
> even if we don't have the new field "tgqual" from the patch.
> I'm not sure why we don't have a toast relation for pg_trigger
> because user might pass very long trigger arguments.
>
>> * ROW INSERT TRIGGER on COPY FROM statement
>
> Thanks. Good catch! Fixed and regression test added.
>
>> * Using system column in WHEN clause
>> 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.
>
> I'd like to only add documentation because I don't have a whole solution.
> ----
> System columns are not available in the <literal>WHEN</> clause
> because those values are initialized after triggers are called.
> They might return wrong values if they used in expressions of the clause.
> ----
>
> Regards,
> ---
> ITAGAKI Takahiro
> NTT Open Source Software Center
>

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Greg Smith wrote:
> 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.
>>
> That one is now yours. It would be nice if you subscribed to the
> pgsql-rrreviewers list if you're not already on there, but it's not
> really necessary if you just want to grab that one patch and report on
> it to pgsql-hackers.

I tagged "Ready for Committer" flag on the patch, so my hands are now getting
free. :-)

I can find some patches which still don't have any feedbacks on the CF page.
https://commitfest.postgresql.org/action/commitfest_view?id=4

If RR-reviewer's availability is not enough now, I will be able to help
review the patches as long as my capability allows it.

In addition, I'm strongly finding volunteers to review the SE-PostgreSQL/Lite
patch. At the first view, it may seem to you a bit large patch, but 40%
of them are documentations (except for source code comments) and test cases.
I believe rest of code will consist of enough simple implementations for
average pgsql-hackers, because I separated a lot of features from full
functionalities revision, as follows:
- No access controls except for database, schemas, tables and columns
- No row-level granularity in access controls
- No caches for access control decisions
- No security identifier in HeapTupleHeader and No new system columns.

The following code example will help understand its design and implementation.
http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/backend/security/sepgsql/README#784

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> [ TRIGGER WHEN patch ]

I'm starting to work this over now, and I've found one rather serious
omission: FreeTriggerDesc doesn't free the expression tree. This means
that trigger WHEN clauses will leak memory in CacheMemoryContext any
time we do a relcache flush on the relation having the trigger. Over
time that could be pretty nasty.

There is no mechanism for freeing an expression tree explicitly, and
creating one is not feasible because of the possibility of multiple
references to subtrees, so this isn't trivial to fix.

There are two alternatives that seem reasonable to me:

* Keep the expression in nodeToString string form within the TriggerDesc
structure; then it's just one more pfree in FreeTriggerDesc. The main
disadvantage of this is that we'd have to repeat stringToNode every time
the trigger is used. This might not be a big deal considering the other
overhead of preparing an expression for execution --- check constraint
expressions are handled that way IIRC --- but it's still a bit annoying.

* Create a separate memory context for each TriggerDesc. This would
simplify FreeTriggerDesc() to a MemoryContextDelete call, which seems
attractive from both speed and code maintenance standpoints; but it
would probably end up wasting a fair amount of space since the context
would likely be mostly empty in most cases.

Not sure which way to jump. Comments?

regards, tom lane

Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
> [ TRIGGER WHEN patch ]

Applied with assorted revisions. AFAICT the system column issue is only
a problem for NEW references in BEFORE triggers --- those columns do
read correctly in OLD, and all the time in AFTER triggers. I revised
the parsing logic to enforce that. The patch also missed establishing
dependencies for stuff referenced in the WHEN expression, and there were
a few other minor problems.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> > [ TRIGGER WHEN patch ]
> Applied with assorted revisions. AFAICT the system column issue is only
> a problem for NEW references in BEFORE triggers --- those columns do
> read correctly in OLD, and all the time in AFTER triggers. I revised
> the parsing logic to enforce that. The patch also missed establishing
> dependencies for stuff referenced in the WHEN expression, and there were
> a few other minor problems.

Thanks for your fix.

>> * Keep the expression in nodeToString string form within the TriggerDesc
>> structure; then it's just one more pfree in FreeTriggerDesc. The main
>> disadvantage of this is that we'd have to repeat stringToNode every time
>> the trigger is used.

I read your fix for the repeated compiling issue is to cache compiled
expressions in ResultRelInfo->ri_TrigWhenExprs. So, we can reuse the
result of stringToNode when we modify multiple rows in a query.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center