Thread
-
TRIGGER with WHEN clause
Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-10-22T02:33:10Z
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 -
Re: TRIGGER with WHEN clause
Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-11-13T03:22:36Z
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
-
Re: TRIGGER with WHEN clause
Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2009-11-13T03:46:57Z
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@ak.jp.nec.com>
-
Re: [HACKERS] TRIGGER with WHEN clause
Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2009-11-13T04:46:35Z
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@ak.jp.nec.com>
-
Re: [HACKERS] TRIGGER with WHEN clause
Greg Smith <greg@2ndquadrant.com> — 2009-11-13T07:16:43Z
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@2ndQuadrant.com www.2ndQuadrant.com
-
Re: TRIGGER with WHEN clause
Kohei KaiGai <kaigai@kaigai.gr.jp> — 2009-11-15T02:38:17Z
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@kaigai.gr.jp> -
Re: TRIGGER with WHEN clause
Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-11-16T08:04:47Z
Thank for your reviewing! KaiGai Kohei <kaigai@kaigai.gr.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 -
Re: TRIGGER with WHEN clause
Albe Laurenz <laurenz.albe@wien.gv.at> — 2009-11-16T10:42:37Z
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 -
Re: TRIGGER with WHEN clause
Kohei KaiGai <kaigai@kaigai.gr.jp> — 2009-11-16T11:03:24Z
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@kaigai.gr.jp>
-
Re: TRIGGER with WHEN clause
Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-11-17T02:35:43Z
KaiGai Kohei <kaigai@kaigai.gr.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
-
Re: TRIGGER with WHEN clause
Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2009-11-17T07:30:06Z
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@ak.jp.nec.com> -
Re: TRIGGER with WHEN clause
Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-11-18T08:46:02Z
KaiGai Kohei <kaigai@ak.jp.nec.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 -
Re: TRIGGER with WHEN clause
Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2009-11-19T01:09:28Z
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@ak.jp.nec.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@ak.jp.nec.com>
-
Re: [HACKERS] TRIGGER with WHEN clause
Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2009-11-19T01:54:02Z
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@ak.jp.nec.com>
-
Re: TRIGGER with WHEN clause
Tom Lane <tgl@sss.pgh.pa.us> — 2009-11-19T19:00:29Z
Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.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
-
Re: TRIGGER with WHEN clause
Tom Lane <tgl@sss.pgh.pa.us> — 2009-11-20T20:41:45Z
Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.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
-
Re: TRIGGER with WHEN clause
Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-11-24T00:42:57Z
Tom Lane <tgl@sss.pgh.pa.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