Horton

Thread

Commits

  1. Fix SQL-spec incompatibilities in new transition table feature.

  2. Quick-hack fix for foreign key cascade vs triggers with transition tables.

  3. Fix transition tables for ON CONFLICT.

  4. Fix transition tables for wCTEs.

  5. Repair problems occurring when multiple RI updates have to be done to the same

  1. BUG #14808: V10-beta4, backend abort

    phb07 <phb07@apra.asso.fr> — 2017-09-09T06:48:53Z

    The following bug has been logged on the website:

Bug reference:      14808
Logged by:          Philippe BEAUDOIN
Email address:      phb07@apra.asso.fr
PostgreSQL version: 10beta4
Operating system:   Linux
Description:        

Hi all,

While continuing to play with transition tables in statement level trigger,
I have encountered what looks like a backend abort.

I have been able to reproduce the case with the following simple script:

#!/bin/sh
export PGHOST=localhost
export PGPORT=5410

dropdb test
createdb test

psql test  <<*EOF*
\set ON_ERROR_STOP on

CREATE OR REPLACE FUNCTION create_tbl(grpdef_schema TEXT, grpdef_tblseq
TEXT)
RETURNS VOID LANGUAGE plpgsql SECURITY DEFINER AS
\$_create_tbl\$
  DECLARE
    v_fullTableName     TEXT;
    v_logTableName      TEXT;
    v_logFnctName       TEXT;
    v_colList1          TEXT;
    v_colList2          TEXT;
    v_colList3          TEXT;
    v_colList4          TEXT;
  BEGIN
-- build the different name for table, trigger, functions,...
    v_fullTableName   = grpdef_schema || '.' || grpdef_tblseq;
    v_logTableName    = grpdef_tblseq || '_log';
    v_logFnctName     = grpdef_tblseq || '_log_idx';
-- build the tables's columns lists
    SELECT string_agg('tbl.' || col_name, ','),
           string_agg('o.' || col_name || ' AS ' || col_name_o || ', n.' ||
col_name || ' AS ' || col_name_n, ','),
           string_agg('r.' || col_name_o, ','),
           string_agg('r.' || col_name_n, ',')
      INTO v_colList1, v_colList2, v_colList3, v_colList4 FROM (
      SELECT quote_ident(attname) AS col_name, quote_ident('o_' || attname)
AS col_name_o, quote_ident('n_' || attname) AS col_name_n
        FROM pg_catalog.pg_attribute
        WHERE attrelid = v_fullTableName::regclass
          AND attnum > 0 AND NOT attisdropped
        ORDER BY attnum) AS t;
-- create the log table: it looks like the application table, with some
additional technical columns
    EXECUTE 'DROP TABLE IF EXISTS ' || v_logTableName;
    EXECUTE 'CREATE TABLE ' || v_logTableName
         || ' (LIKE ' || v_fullTableName || ') ';
    EXECUTE 'ALTER TABLE ' || v_logTableName
         || ' ADD COLUMN verb      VARCHAR(3),'
         || ' ADD COLUMN tuple     VARCHAR(3)';
-- create the log function
    EXECUTE 'CREATE OR REPLACE FUNCTION ' || v_logFnctName || '() RETURNS
TRIGGER AS \$logfnct\$'
         || 'DECLARE'
         || '  r          RECORD;'
         || 'BEGIN'
         || '  IF    (TG_OP = ''DELETE'') THEN'
         || '    INSERT INTO ' || v_logTableName || ' SELECT ' || v_colList1
|| ', ''DEL'', ''OLD'' FROM old_table tbl;'
         || '  ELSIF (TG_OP = ''INSERT'') THEN'
         || '    INSERT INTO ' || v_logTableName || ' SELECT ' || v_colList1
|| ', ''INS'', ''NEW'' FROM new_table tbl;'
         || '  ELSIF (TG_OP = ''UPDATE'') THEN'
         || '    FOR r IN'
         || '      WITH'
         || '          o AS (SELECT ' || v_colList1 || ', row_number() OVER
() AS ord FROM old_table tbl'
         || '          ),'
         || '          n AS (SELECT ' || v_colList1 || ', row_number() OVER
() AS ord FROM new_table tbl'
         || '      )'
         || '      SELECT ' || v_colList2
         || '      FROM o JOIN n USING(ord)'
         || '      LOOP'
         || '        INSERT INTO ' || v_logTableName 
         || '          SELECT ' || v_colList3 || ', ''UPD'', ''OLD'';'
         || '        INSERT INTO ' || v_logTableName
         || '          SELECT ' || v_colList4 || ', ''UPD'', ''NEW'';'
         || '    END LOOP;'
         || '  END IF;'
         || '  RETURN NULL;'
         || 'END;'
         || '\$logfnct\$ LANGUAGE plpgsql SECURITY DEFINER;';
-- creation of the log trigger on the application table, using the
previously created log function
    EXECUTE 'CREATE TRIGGER insert_log_trg'
         || '  AFTER INSERT ON ' || v_fullTableName || ' REFERENCING NEW
TABLE AS new_table'
         || '  FOR EACH STATEMENT EXECUTE PROCEDURE ' || v_logFnctName ||
'()';
    EXECUTE 'CREATE TRIGGER update_log_trg'
         || '  AFTER UPDATE ON ' || v_fullTableName || ' REFERENCING OLD
TABLE AS old_table NEW TABLE AS new_table'
         || '  FOR EACH STATEMENT EXECUTE PROCEDURE ' || v_logFnctName ||
'()';
    EXECUTE 'CREATE TRIGGER delete_log_trg'
         || '  AFTER DELETE ON ' || v_fullTableName || ' REFERENCING OLD
TABLE AS old_table'
         || '  FOR EACH STATEMENT EXECUTE PROCEDURE ' || v_logFnctName ||
'()';
    RETURN;
  END;
\$_create_tbl\$;

CREATE TABLE myTbl1 (
  col11       INT      NOT NULL,
  col12       TEXT     ,
  col13       TEXT     ,
  PRIMARY KEY (col11)
);

CREATE TABLE myTbl3 (
  col41       INT      NOT NULL,
  col44       INT      ,
  PRIMARY KEY (col41),
  FOREIGN KEY (col44) REFERENCES myTbl1 (col11) ON DELETE CASCADE ON UPDATE
SET NULL
);

select create_tbl('public','mytbl1');
select create_tbl('public','mytbl3');

insert into myTbl1 select i, 'ABC', 'abc' from generate_series (1,10100) as
i;
update myTbl1 set col13=E'\\034'::bytea where col11 <= 500;
delete from myTbl1 where col11 > 10000;

*EOF*

As a result, the last DELETE statement fails. I get:

CREATE FUNCTION
CREATE TABLE
CREATE TABLE
NOTICE:  table "mytbl1_log" does not exist, skipping
 create_tbl 
------------
 
(1 row)

NOTICE:  table "mytbl3_log" does not exist, skipping
 create_tbl 
------------
 
(1 row)

INSERT 0 1101
UPDATE 0
server closed the connection unexpectedly
	This probably means the server terminated abnormally
	before or while processing the request.
connection to server was lost


The postgresql.conf file has default parameters, except:
listen_addresses = '*'
port = 5410
max_prepared_transactions 5
logging_collector = on
track_functions = all
track_commit_timestamp = on

Best regards.
Philippe Beaudoin.

  2. Re: BUG #14808: V10-beta4, backend abort

    Michael Paquier <michael.paquier@gmail.com> — 2017-09-09T11:27:05Z

    On Sat, Sep 9, 2017 at 3:48 PM,  <phb07@apra.asso.fr> wrote:
> INSERT 0 1101
> UPDATE 0
> server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
> connection to server was lost

Crash is confirmed here so I am adding an open item. I have not dug
into the issue seriously, but at short glance we have a code path
trying to access something that has already been free'd:
#0  0x0000561bfe0f0959 in tuplestore_tuple_count
(state=0x7f7f7f7f7f7f7f7f) at tuplestore.c:548
548        return state->tuples;
(gdb) bt
#0  0x0000561bfe0f0959 in tuplestore_tuple_count
(state=0x7f7f7f7f7f7f7f7f) at tuplestore.c:548
#1  0x0000561bfdd8ca22 in SPI_register_trigger_data
(tdata=0x7ffc92083860) at spi.c:2764
#2  0x00007f7075da7156 in plpgsql_exec_trigger (func=0x561bfe7bc5a8,
trigdata=0x7ffc92083860) at pl_exec.c:692
#3  0x00007f7075da08e7 in plpgsql_call_handler (fcinfo=0x7ffc920833d0)
at pl_handler.c:24
(gdb) p state
$1 = (Tuplestorestate *) 0x7f7f7f7f7f7f7f7
-- 
Michael

  3. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-09T17:08:50Z

    Michael Paquier <michael.paquier@gmail.com> writes:
> Crash is confirmed here so I am adding an open item. I have not dug
> into the issue seriously, but at short glance we have a code path
> trying to access something that has already been free'd:

I think this can be blamed on commit c46c0e52.

What is happening is that the AFTER triggers are queuing more triggers,
which have TransitionCaptureStates associated, but 
ExecEndModifyTable thinks it can DestroyTransitionCaptureState
unconditionally.  When the subsidiary triggers eventually get executed,
their ats_transition_capture pointers are pointing at garbage.

My first instinct is to get rid of DestroyTransitionCaptureState
altogether, on the grounds that the TransitionCaptureState will
go away at transaction cleanup and we can't really get rid of it
any sooner than that.

The other question that seems pretty relevant is why the subsidiary
triggers, which are the constraint triggers associated with the
tables' foreign keys, are getting queued with TransitionCaptureState
pointers in the first place.  This seems horridly expensive and
unnecessary.  It also directly contradicts the claim in
MakeTransitionCaptureState that constraint triggers cannot have
transition tables.

			regards, tom lane

  4. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-09T18:59:54Z

    I wrote:
> What is happening is that the AFTER triggers are queuing more triggers,
> which have TransitionCaptureStates associated, but 
> ExecEndModifyTable thinks it can DestroyTransitionCaptureState
> unconditionally.  When the subsidiary triggers eventually get executed,
> their ats_transition_capture pointers are pointing at garbage.

On closer inspection, the issue is specific to TCS-using AFTER triggers
that are being fired as a result of foreign key enforcement triggers.
In the example, each row deleted from myTbl1 causes firing of the
ON DELETE CASCADE enforcement trigger, which will execute a DELETE
against myTbl3.  That won't delete anything (since myTbl3 is empty)
but we nonetheless queue a firing of the TCS-using AFTER STATEMENT
trigger for myTbl3.

Now, the ExecEndModifyTable instance for the DELETE supposes that
all TCS-using triggers must have been fired during ExecutorFinish;
but *that doesn't happen if EXEC_FLAG_SKIP_TRIGGERS is set*, which
it is because ri_PerformCheck tells SPI not to fire triggers.

I do not recall the exact details of why that is, but I believe
the intention is to fire RI-triggered triggers at the end of the
outer statement rather than exposing the individual RI action as
a statement.

I made a quick hack to not delete the TCS if EXEC_FLAG_SKIP_TRIGGERS
is set, as attached.  The example succeeds given this.  However,
note that the AFTER STATEMENT trigger will be fired once per myTbl1 row
deletion, each time seeing a transition table consisting of only the
myTbl3 rows that went away as a result of that one single row deletion.
I'm not sure that this is per the letter or spirit of the SQL spec.

If it isn't, I don't think there is much we can do about it for v10.
In v11 or later, we could think about somehow merging the transition
tables for all the RI actions triggered by one statement.  This might
well need to go along with rewriting the RI framework to use statement
triggers and TCS state itself.  I think people had already muttered
about doing that.

> The other question that seems pretty relevant is why the subsidiary
> triggers, which are the constraint triggers associated with the
> tables' foreign keys, are getting queued with TransitionCaptureState
> pointers in the first place.  This seems horridly expensive and
> unnecessary.  It also directly contradicts the claim in
> MakeTransitionCaptureState that constraint triggers cannot have
> transition tables.

The other part of the attached patch tweaks AfterTriggerSaveEvent
to not store an ats_transition_capture pointer for a deferrable
trigger event.  This doesn't have anything directly to do with
the current bug, because although the RI triggers are being stored
with such pointers, they aren't actually dereferencing them.  However,
it seems like a good idea anyway, to (1) ensure that we don't have
dangling pointers in the trigger queue, and (2) possibly allow
more merging of shared trigger states.

			regards, tom lane

  5. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-10T19:02:53Z

    I wrote:
> Now, the ExecEndModifyTable instance for the DELETE supposes that
> all TCS-using triggers must have been fired during ExecutorFinish;
> but *that doesn't happen if EXEC_FLAG_SKIP_TRIGGERS is set*, which
> it is because ri_PerformCheck tells SPI not to fire triggers.

In view of the fact that 10rc1 wraps tomorrow, there doesn't seem
to be time to do anything better about this than my quick hack.
So I went ahead and pushed that, with a regression test case.

			regards, tom lane

  6. Re: BUG #14808: V10-beta4, backend abort

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-10T21:46:52Z

    On Mon, Sep 11, 2017 at 7:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Now, the ExecEndModifyTable instance for the DELETE supposes that
>> all TCS-using triggers must have been fired during ExecutorFinish;
>> but *that doesn't happen if EXEC_FLAG_SKIP_TRIGGERS is set*, which
>> it is because ri_PerformCheck tells SPI not to fire triggers.
>
> In view of the fact that 10rc1 wraps tomorrow, there doesn't seem
> to be time to do anything better about this than my quick hack.
> So I went ahead and pushed that, with a regression test case.

Thank you for the testing and report Philippe, and for the analysis and fix Tom.

@@ -2318,8 +2318,14 @@ ExecEndModifyTable(ModifyTableState *node)
 {
        int                     i;

-       /* Free transition tables */
-       if (node->mt_transition_capture != NULL)
+       /*
+        * Free transition tables, unless this query is being run in
+        * EXEC_FLAG_SKIP_TRIGGERS mode, which means that it may have
queued AFTER
+        * triggers that won't be run till later.  In that case we'll just leak
+        * the transition tables till end of (sub)transaction.
+        */
+       if (node->mt_transition_capture != NULL &&
+               !(node->ps.state->es_top_eflags & EXEC_FLAG_SKIP_TRIGGERS))
                DestroyTransitionCaptureState(node->mt_transition_capture);

As an idea for a v11 patch, I wonder if it would be better to count
references instead of leaking the TCS as soon as fk-on-cascade
triggers enter the picture.  The ModifyTable node would release its
reference in ExecEndModifyTable(), and the queued events' references
would be counted too.  I briefly considered a scheme like that before
proposing 501ed02c, but concluded, as it turns out incorrectly, that
there was no way for a trigger referencing node->mt_transition_capture
to fire after that point.

-- 
Thomas Munro
http://www.enterprisedb.com

  7. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-11T13:08:44Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> As an idea for a v11 patch, I wonder if it would be better to count
> references instead of leaking the TCS as soon as fk-on-cascade
> triggers enter the picture.

I thought of reference counting as well, but I think it's not really
necessary if we organize things correctly.  The key problem here IMO
is that the transition tables need to be attached to a trigger execution
context, rather than supposing that they can be completely managed by
ModifyTable plan nodes.

I dug around in the SQL spec and convinced myself that indeed it does
require all RI updates triggered by a single statement to be presented
in a single transition table.  Concretely, it says

         A trigger execution context consists of a set of state changes.
         Within a trigger execution context, each state change is uniquely
         identified by a trigger event, a subject table, and a column list.
         The trigger event can be DELETE, INSERT, or UPDATE. A state change
         SC contains a set of transitions, a set of statement-level triggers
         considered as executed for SC, and a set of row-level triggers,
         each paired with the set of rows in SC for which it is considered
         as executed.

Note the "uniquely identified" bit --- you aren't allowed to have multiple
SCs for the same table and same kind of event, at least up to the bit
about column lists.  I've not fully wrapped my head around the column list
part of it, but certainly all effects of a particular RI constraint will
have the same column list.

Now, the lifespan of a trigger execution context is one SQL-statement,
but that's one user-executed SQL-statement --- the queries that we gin
up for RI enforcement are an implementation detail that don't get their
own context.  (The part of the spec that defines RI actions seems pretty
clear that the actions insert state changes into the active statement's
trigger execution context, not create their own context.)

It's also interesting in this context to re-read commit 9cb840976,
which is what created the "skip trigger" business.  That exhibits a
practical problem that you hit if you don't do it like this.

So ISTM that basically we need trigger.c to own the transition tables.
ModifyTable, instead of just creating a TCS, needs to ask trigger.c for a
TCS relevant to its target table and command type (and column list if we
decide we need to bother with that).  trigger.c would discard TCSes during
AfterTriggerEndQuery, where it closes out a given query_depth level.

This actually seems like it might not be that big a change, other than
the issue of what do the column lists mean exactly.  Maybe we should try
to get it done for v10, rather than shipping known-non-spec-compliant
behavior.

			regards, tom lane

  8. Re: BUG #14808: V10-beta4, backend abort

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2017-09-11T13:18:41Z

    Tom Lane wrote:

> This actually seems like it might not be that big a change, other than
> the issue of what do the column lists mean exactly.  Maybe we should try
> to get it done for v10, rather than shipping known-non-spec-compliant
> behavior.

I think this means we need to abort RC1 today and get another beta out.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-11T17:03:51Z

    I wrote:
> Note the "uniquely identified" bit --- you aren't allowed to have multiple
> SCs for the same table and same kind of event, at least up to the bit
> about column lists.  I've not fully wrapped my head around the column list
> part of it, but certainly all effects of a particular RI constraint will
> have the same column list.

After further study of the standard, it seems that the column list
business is meant to allow identification of which UPDATE triggers
with column lists ("AFTER UPDATE OF columnList" syntax) are supposed
to be triggered.  The spec is confusing because they describe this in
a way that would be impossibly inefficient if implemented verbatim.
They say that, given a row UPDATE affecting a certain set of columns,
you're supposed to create state changes labeled with every possibly
relevant trigger column set:

— Let OC be the set of column names identifying the columns [being updated]
— Let PSC be the set consisting of the empty set and every subset of the
  set of column names of [the target table] that has at least one column
  that is in OC
- [ create a state change for each element of PSC ]

Then an update trigger is triggered by a particular state change if its
column list exactly matches that state change's list.  This seems like a
remarkably stupid way to go about it; you'd end up with many state changes
that correspond to no existing trigger and are never of any use.

However, if I'm reading it right, there is a property of this that is
very significant in the context of RI updates.  For an ordinary UPDATE
SQL command, all the row updates have the same set of target columns,
but that's not necessarily true for all the RI updates that a single SQL
command could trigger.  If there's more than one RI constraint between
two tables then an update on the referenced table could trigger sets of
updates that affect overlapping, but not identical, sets of rows in the
referencing tables --- and those sets would have different sets of target
columns.  So a given column-specific trigger might be interested in some
or all of the RI updates.  And if it is interested, and is a statement
trigger, it is supposed to be fired just once with a transition table
containing all the rows it is interested in.

In other words, UPDATE triggers with different column lists potentially
need to see different transition tables, and any given row that was
updated might need to appear in some subset of those tables.

This seems like rather a mess to implement.  I wonder whether I'm
reading it right, and whether other DBMSes actually implement it
like that.

I think that what might be a good short-term solution is to refuse
creation of column-specific triggers with transition tables (ie,
if you ask for a transition table then you can only say AFTER UPDATE
not AFTER UPDATE OF columnList).  Then, all TT-using triggers are
interested in all modified rows and we don't have to distinguish
different column lists for the purposes of transition tables.
Then the problem reduces to one TCS per target table and event type,
which doesn't seem too hard to do.

			regards, tom lane

  10. Re: BUG #14808: V10-beta4, backend abort

    Robert Haas <robertmhaas@gmail.com> — 2017-09-11T19:17:44Z

    On Sat, Sep 9, 2017 at 1:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> My first instinct is to get rid of DestroyTransitionCaptureState
> altogether, on the grounds that the TransitionCaptureState will
> go away at transaction cleanup and we can't really get rid of it
> any sooner than that.

End of transaction, or end of query?  I'm not sure what happens when
triggers are deferred, but I think there are a lot of cases when we
want to throw away the tuplestore immediately, not hold on to it for
the rest of the transaction.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  11. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-11T19:24:40Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Sep 9, 2017 at 1:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> My first instinct is to get rid of DestroyTransitionCaptureState
>> altogether, on the grounds that the TransitionCaptureState will
>> go away at transaction cleanup and we can't really get rid of it
>> any sooner than that.

> End of transaction, or end of query?  I'm not sure what happens when
> triggers are deferred, but I think there are a lot of cases when we
> want to throw away the tuplestore immediately, not hold on to it for
> the rest of the transaction.

As things stand, it's end of subtransaction, because the TCSes
are allocated in CurTransactionContext.  See the argument in
MakeTransitionCaptureState.

And yes, this is inefficient.  The quick-hack patch I committed yesterday
only pays the price if you have RI triggers cascading changes into a table
that also has triggers-with-transition-tables, but I can certainly believe
that it could get expensive in such a case.

The fix proposal discussed downthread should fix the inefficiency as well
as the spec compliance problem.  But personally I'm far more worried about
the latter.

			regards, tom lane

  12. Re: BUG #14808: V10-beta4, backend abort

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-12T00:38:28Z

    On Tue, Sep 12, 2017 at 5:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Note the "uniquely identified" bit --- you aren't allowed to have multiple
>> SCs for the same table and same kind of event, at least up to the bit
>> about column lists.  I've not fully wrapped my head around the column list
>> part of it, but certainly all effects of a particular RI constraint will
>> have the same column list.

Aside from the RI case, the other user visible change in behaviour
will be for statements that update the same table via multiple
ModifyTable nodes (wCTEs).  Our regression test has:

with wcte as (insert into table1 values (42))
  insert into table2 values ('hello world');

... which demonstrates the fix for the original complaint that table1
and table2 earlier tried to use the same transition table (and
crashed).  A new variant inserting into table1 twice would show the
difference.  Today we get:

postgres=# with wcte as (insert into table1 values (42))
             insert into table1 values (43);
NOTICE:  trigger = table1_trig, new table = (43,)
NOTICE:  trigger = table1_trig, new table = (42,)
INSERT 0 1

Presumably with your change there will be a single transition table
for inserts into table, holding both (42,) and (43,).  But will we
fire the trigger once or twice?  There is something fishy about making
it fire twice but show the same tuples to both invocations (for
example, it might break Kevin's proposed counting algorithm which this
feature is intended to support), but firing only once requires some
new inter-node co-ordination.

> In other words, UPDATE triggers with different column lists potentially
> need to see different transition tables, and any given row that was
> updated might need to appear in some subset of those tables.
>
> This seems like rather a mess to implement.  I wonder whether I'm
> reading it right, and whether other DBMSes actually implement it
> like that.

I guess the alternative is storing extra per-tuple metadata,
transferring more work to the reader.

The DB2 documentation has this example[1]:

 CREATE TRIGGER REORDER
     AFTER UPDATE OF ON_HAND, MAX_STOCKED ON PARTS
     REFERENCING NEW_TABLE AS NTABLE
     FOR EACH STATEMENT MODE DB2SQL
       BEGIN ATOMIC
         SELECT ISSUE_SHIP_REQUEST(MAX_STOCKED - ON_HAND, PARTNO)
           FROM NTABLE
         WHERE (ON_HAND < 0.10 * MAX_STOCKED);
     END

I can't find any explicit discussion of whether this trigger could
ever see a transition row that results from an update that didn't name
ON_HAND or MAX_STOCKED.  I don't have DB2 access and I'm not sure how
I'd test that...  maybe with a self-referencing fk declared ON UPDATE
CASCADE?

Thanks to prodding from Peter Geoghegan we tackled the question of
whether the <trigger action time> clause controls just when the
trigger fires or also which transition tuples it sees.  By looking at
some wording relating to MERGE we concluded it must be both,
culminating in commit 8c55244a which separates the UPDATEs and INSERTs
resulting from INSERT ... ON CONFLICT DO UPDATE.  That had the
annoying result that we had to ban the use of (non-standard) "OR" in
<trigger action time> when transition tables are in play.  This FOR
UPDATE OF ... transition business seems sort of similar, but would
create arbitrarily many transition tables and require the executor to
write into all of them, or perhaps store extra meta data along with
captured rows for later filtering during scan.

> I think that what might be a good short-term solution is to refuse
> creation of column-specific triggers with transition tables (ie,
> if you ask for a transition table then you can only say AFTER UPDATE
> not AFTER UPDATE OF columnList).  Then, all TT-using triggers are
> interested in all modified rows and we don't have to distinguish
> different column lists for the purposes of transition tables.
> Then the problem reduces to one TCS per target table and event type,
> which doesn't seem too hard to do.

+1

Presumably would-be authors of triggers-with-transition-tables that
would fire only AFTER UPDATE OF foo already have to deal with the
possibility that you updated foo to the same value.  So I don't think
too much is lost, except perhaps some efficiency.

Thinking a bit harder about whether you might have semantic (rather
than performance) reasons to use AFTER UPDATE OF ... with subset TTs,
it occurs to me that there may be implications for transition tables
with inheritance.  We decided to disallow transition tables on
non-partition inheritance children anyway (see 501ed02c), but DB2
supports the equivalent.  It has a system of inheritance ("typed
tables", possibly conforming to SQL:1999 though I've never looked into
that) but has different rules about when row triggers and statement
triggers fire when you run DML statements on a table hierarchy.  Don't
quote me but it's something like our rules plus (1) row triggers of
all supertables of an affected table also fire (unless created with
CREATE TRIGGER ... ONLY), and (2) statement triggers of affected
subtables also fire.  Implementing that for our transition tables
would probably require more tuplestores and/or dynamic tuple
conversion and filtering during later scan.  Perhaps AFTER UPDATE OF
column_that_only_this_child_and_its_children_have would fire for
direct and subtable updates but not via-the-supertable updates.  That
is currently completely irrelevant due to our set of supported
features: different firing rules, and prohibition on children with
transition tables.  Some related topics might return soon when people
get more experience with partitions and start wanting to declare row
triggers on partitioned tables (perhaps including foreign key checks)
or implement Kevin's clever batch-mode foreign key check concept.

[1] https://www.ibm.com/support/knowledgecenter/en/SSEPEK_10.0.0/sqlref/src/tpc/db2z_sql_createtrigger.html

-- 
Thomas Munro
http://www.enterprisedb.com

  13. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-12T00:53:00Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Aside from the RI case, the other user visible change in behaviour
> will be for statements that update the same table via multiple
> ModifyTable nodes (wCTEs).  Our regression test has:

> with wcte as (insert into table1 values (42))
>   insert into table2 values ('hello world');

> ... which demonstrates the fix for the original complaint that table1
> and table2 earlier tried to use the same transition table (and
> crashed).  A new variant inserting into table1 twice would show the
> difference.  Today we get:

> postgres=# with wcte as (insert into table1 values (42))
>              insert into table1 values (43);
> NOTICE:  trigger = table1_trig, new table = (43,)
> NOTICE:  trigger = table1_trig, new table = (42,)
> INSERT 0 1

> Presumably with your change there will be a single transition table
> for inserts into table, holding both (42,) and (43,).  But will we
> fire the trigger once or twice?

Not necessarily.  That would be true only if we allow the WCTE to share
trigger context with the outer query, which I think it does not today.
I've not checked the code, but presumably if we fire the trigger twice
right now, that means there are separate trigger contexts, ie somebody
is calling AfterTriggerBeginQuery/AfterTriggerEndQuery around the WCTE.
If not, maybe we could make it do so.  OTOH, looking at the text of
the spec, I think it's darn hard to justify the behavior shown above.

The reason that the RI case would share trigger context with the outer
query is that we'd *not* call AfterTriggerBeginQuery/AfterTriggerEndQuery
around the RI query, which would be driven by the same skip_trigger
logic that exists today.

>> This seems like rather a mess to implement.  I wonder whether I'm
>> reading it right, and whether other DBMSes actually implement it
>> like that.

> I guess the alternative is storing extra per-tuple metadata,
> transferring more work to the reader.

I really don't want any more per-tuple state.  Adding the TCS link
was costly enough in terms of how big the tuple queue storage is.

			regards, tom lane

  14. Re: BUG #14808: V10-beta4, backend abort

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-12T01:22:36Z

    On Tue, Sep 12, 2017 at 12:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> OTOH, looking at the text of
> the spec, I think it's darn hard to justify the behavior shown above.

Yeah.  I assume we always fired statement triggers for each separate
instance of the same table mentioned in a wCTE since they were
invented.  I just confirmed that that is the case in 9.6.  That may
not have been in the spirit of the spec, but it's hard to say because
the spec doesn't have wCTEs IIUC and it mattered less because they
didn't receive any data.

Now that they can optionally see data resulting from modifications, it
seems pretty hard to use this feature to build anything that consumes
the transition data and has to be reliable (matview state,
replication-like systems etc) if we make any choice other than (1)
each instance of a given table fires a statement trigger separately
and sees only the rows it touched, or (2) the statement trigger is
fired once for all instances of a table, and sees all the transition
tuples.  Based on the SQL spec excerpts you've highlighted, I suppose
it seems likely that if the spec had wCTEs it would expect them to
work like (2).

-- 
Thomas Munro
http://www.enterprisedb.com

  15. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-13T22:44:02Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Aside from the RI case, the other user visible change in behaviour
> will be for statements that update the same table via multiple
> ModifyTable nodes (wCTEs).  Our regression test has:

> with wcte as (insert into table1 values (42))
>   insert into table2 values ('hello world');

> ... which demonstrates the fix for the original complaint that table1
> and table2 earlier tried to use the same transition table (and
> crashed).

BTW, as I'm digging around in trigger.c, I can't help noticing that
it provides a single "fdw_tuplestore" per trigger query level (a/k/a
trigger execution context).  I've not tried to test this, but it
sure looks like a wCTE like your example above, directed at two
separate foreign tables with triggers, would fail for exactly the
same reason.  That'd be a bug of pretty long standing.

			regards, tom lane

  16. Re: BUG #14808: V10-beta4, backend abort

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-13T23:00:40Z

    On Thu, Sep 14, 2017 at 10:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> Aside from the RI case, the other user visible change in behaviour
>> will be for statements that update the same table via multiple
>> ModifyTable nodes (wCTEs).  Our regression test has:
>
>> with wcte as (insert into table1 values (42))
>>   insert into table2 values ('hello world');
>
>> ... which demonstrates the fix for the original complaint that table1
>> and table2 earlier tried to use the same transition table (and
>> crashed).
>
> BTW, as I'm digging around in trigger.c, I can't help noticing that
> it provides a single "fdw_tuplestore" per trigger query level (a/k/a
> trigger execution context).  I've not tried to test this, but it
> sure looks like a wCTE like your example above, directed at two
> separate foreign tables with triggers, would fail for exactly the
> same reason.  That'd be a bug of pretty long standing.

I had the impression that that fdw_tuplestore was doing something a
bit sneaky that actually works out OK: tuples get enqueued and later
dequeued in exactly the same sequence as the after row trigger events
that need them, so even though it seems to violate at least the POLA
if not the spirit of tuplestores by storing tuples of potentially
different types in one tuplestore, nothing bad should happen.  I
suppose it was by copying that coding that Kevin finished up with the
initial bug that wCTEs mix stuff from different wCTEs and it all blows
up, because it has no similar sequencing trick: triggers with
transition tables were seeing all of them, and they weren't even
guaranteed to be of the right type.

-- 
Thomas Munro
http://www.enterprisedb.com

  17. Re: BUG #14808: V10-beta4, backend abort

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-13T23:40:59Z

    On Thu, Sep 14, 2017 at 11:00 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Thu, Sep 14, 2017 at 10:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, as I'm digging around in trigger.c, I can't help noticing that
>> it provides a single "fdw_tuplestore" per trigger query level (a/k/a
>> trigger execution context).  I've not tried to test this, but it
>> sure looks like a wCTE like your example above, directed at two
>> separate foreign tables with triggers, would fail for exactly the
>> same reason.  That'd be a bug of pretty long standing.
>
> I had the impression that that fdw_tuplestore was doing something a
> bit sneaky that actually works out OK: tuples get enqueued and later
> dequeued in exactly the same sequence as the after row trigger events
> that need them, so even though it seems to violate at least the POLA
> if not the spirit of tuplestores by storing tuples of potentially
> different types in one tuplestore, nothing bad should happen.  I
> suppose it was by copying that coding that Kevin finished up with the
> initial bug that wCTEs mix stuff from different wCTEs and it all blows
> up, because it has no similar sequencing trick: triggers with
> transition tables were seeing all of them, and they weren't even
> guaranteed to be of the right type.

Incidentally, understanding that made me wonder why we don't have a
binary chunk-oriented in-memory-up-to-some-size-then-spill-to-disk
spooling mechanism that could be used for the trigger queue itself
(which currently doesn't know how to spill to disk and therefore can
take your server out), including holding these tuple images directly
(instead of spilling just the tuples in synchronised order with the
in-memory trigger queue).

-- 
Thomas Munro
http://www.enterprisedb.com

  18. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-13T23:45:46Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Thu, Sep 14, 2017 at 10:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, as I'm digging around in trigger.c, I can't help noticing that
>> it provides a single "fdw_tuplestore" per trigger query level (a/k/a
>> trigger execution context).  I've not tried to test this, but it
>> sure looks like a wCTE like your example above, directed at two
>> separate foreign tables with triggers, would fail for exactly the
>> same reason.  That'd be a bug of pretty long standing.

> I had the impression that that fdw_tuplestore was doing something a
> bit sneaky that actually works out OK: tuples get enqueued and later
> dequeued in exactly the same sequence as the after row trigger events
> that need them, so even though it seems to violate at least the POLA
> if not the spirit of tuplestores by storing tuples of potentially
> different types in one tuplestore, nothing bad should happen.

Oh?  Now my fear level is up to 11, because it is completely trivial to
cause triggers to fire in a different order than they were enqueued.
All you need is a mix of deferrable and nondeferrable triggers.

In fact, it also seems entirely broken that a per-query-level tuplestore
is being used at all, because deferrable triggers might not get fired
until some outer query level.

[ Pokes around... ]  Hm, looks like we get around that by forbidding
constraint triggers on foreign tables, but I don't see anything in the
CREATE TRIGGER man page saying that there's such a prohibition.  And
there's certainly no comments in the source code explaining this rickety
set of requirements :-(

			regards, tom lane

  19. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-13T23:54:48Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Incidentally, understanding that made me wonder why we don't have a
> binary chunk-oriented in-memory-up-to-some-size-then-spill-to-disk
> spooling mechanism that could be used for the trigger queue itself
> (which currently doesn't know how to spill to disk and therefore can
> take your server out), including holding these tuple images directly
> (instead of spilling just the tuples in synchronised order with the
> in-memory trigger queue).

The past discussions about spilling the trigger queue have generally
concluded that by the time your event list was long enough to cause
serious pain, you already had a query that was never gonna complete.
That may be getting less true as time goes on, but I'm not sure ---
seems like RAM capacity is growing faster than CPU speed.  Anyway,
that's why it never got done.

Given the addition of transition tables, I suspect there will be
even less motivation to fix it: the right thing to do with mass
updates will be to use a TT with an after-statement trigger, and
that fixes it by putting the bulk data into a spillable tuplestore.

			regards, tom lane

  20. Re: BUG #14808: V10-beta4, backend abort

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-14T20:43:25Z

    On Tue, Sep 12, 2017 at 1:22 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Tue, Sep 12, 2017 at 12:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> OTOH, looking at the text of
>> the spec, I think it's darn hard to justify the behavior shown above.
>
> Yeah.  I assume we always fired statement triggers for each separate
> instance of the same table mentioned in a wCTE since they were
> invented.  I just confirmed that that is the case in 9.6.  That may
> not have been in the spirit of the spec, but it's hard to say because
> the spec doesn't have wCTEs IIUC and it mattered less because they
> didn't receive any data.
>
> Now that they can optionally see data resulting from modifications, it
> seems pretty hard to use this feature to build anything that consumes
> the transition data and has to be reliable (matview state,
> replication-like systems etc) if we make any choice other than (1)
> each instance of a given table fires a statement trigger separately
> and sees only the rows it touched, or (2) the statement trigger is
> fired once for all instances of a table, and sees all the transition
> tuples.  Based on the SQL spec excerpts you've highlighted, I suppose
> it seems likely that if the spec had wCTEs it would expect them to
> work like (2).

So I guess there are about 3 parts to this puzzle:

1.  Merging the transition tables when there are multiple wCTEs
referencing the same table.  Here's one idea:  Rename
MakeTransitionCaptureState() to GetTransitionCaptureState() and use a
hash table keyed by table OID in
afterTriggers.transition_capture_states[afterTriggers.query_depth] to
find the TCS for the given TriggerDesc or create it if not found, so
that all wCTEs find the same TransitionCaptureState object.  The all
existing callers continue to do what they're doing now, but they'll be
sharing TCSs appropriately with other things in the plan.  Note that
TransitionCaptureState already holds tuplestores for each operation
(INSERT, UPDATE, DELETE) so the OID of the table alone is a suitable
key for the hash table (assuming we are ignoring the column-list part
of the spec as you suggested).

2.  Hiding the fact that we implement fk CASCADE using another level
of queries.   Perhaps we could arrange for
afterTriggers.transition_capture_states[afterTriggers.query_depth] to
point to the same hash table as query_depth - 1, so that the effects
of statements at this implementation-internal level appear to the user
as part of the the level below?

3.  Merging the invocation after statement firing so that if you
updated the same table directly and also via a wCTE and also
indirectly via fk ON DELETE/UPDATE trigger then you still only get one
invocation of the after statement trigger.  Not sure exactly how...
perhaps using a flag in the TransitionCaptureState to prevent multiple
firings.  As I argued in the above-quoted email, if we've merged the
transition tables then we'll need to merge the trigger firing too or
it won't be possible to make coherent integrity, summary, replication
etc systems using TT triggers (even though that's a user-visible
change in after statement firing behaviour for wCTEs compared to
earlier releases).

Does this make any sense?

-- 
Thomas Munro
http://www.enterprisedb.com

  21. Re: BUG #14808: V10-beta4, backend abort

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-14T21:10:25Z

    On Fri, Sep 15, 2017 at 8:43 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> Note that
> TransitionCaptureState already holds tuplestores for each operation
> (INSERT, UPDATE, DELETE)

Erm, that's not quite true -- it only separates INSERT and UPDATE for
now.  It would need to be true, so it would need to gain one more to
have the full set.

> ... perhaps using a flag in the TransitionCaptureState to prevent multiple
> firings.

Of course that would need to be per-trigger, not just one flag per
TCS.  Also the change in firing rules for multiply-referenced tables
would apply also when there are no TTs involved, so perhaps TCS is not
a good place for that state.

-- 
Thomas Munro
http://www.enterprisedb.com

  22. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-14T21:45:55Z

    Attached is a draft patch for this.

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> 1.  Merging the transition tables when there are multiple wCTEs
> referencing the same table.  Here's one idea:  Rename
> MakeTransitionCaptureState() to GetTransitionCaptureState() and use a
> hash table keyed by table OID in
> afterTriggers.transition_capture_states[afterTriggers.query_depth] to
> find the TCS for the given TriggerDesc or create it if not found, so
> that all wCTEs find the same TransitionCaptureState object.  The all
> existing callers continue to do what they're doing now, but they'll be
> sharing TCSs appropriately with other things in the plan.  Note that
> TransitionCaptureState already holds tuplestores for each operation
> (INSERT, UPDATE, DELETE) so the OID of the table alone is a suitable
> key for the hash table (assuming we are ignoring the column-list part
> of the spec as you suggested).

It seems unsafe to merge the TCS objects themselves, because the callers
assume that they can munge the tcs_map and tcs_original_insert_tuple
fields freely without regard for any other callers.  So as I have it,
we still have a TCS for each caller, but the TCSes point at tuplestores
that can be shared across multiple callers for the same event type.
The tuplestores themselves are managed by the AfterTrigger data
structures.  Also, because the TCS structs are just throwaway per-caller
data, it's uncool to reference them in the trigger event lists.
So I replaced ats_transition_capture with two pointers to the actual
tuplestores.  That bloats AfterTriggerSharedData a bit but I think it's
okay; we don't expect a lot of those structs in a normal query.

I chose to make the persistent state (AfterTriggersTableData) independent
for each operation type.  We could have done that differently perhaps, but
it seemed more complicated and less likely to match the spec's semantics.

The INSERT ON CONFLICT UPDATE mess is handled by creating two separate
TCSes with two different underlying AfterTriggersTableData structs.
The insertion tuplestore sees only the inserted tuples, the update
tuplestores see only the updated-pre-existing tuples.  That adds a little
code to nodeModifyTable but it seems conceptually much cleaner.

> 2.  Hiding the fact that we implement fk CASCADE using another level
> of queries.   Perhaps we could arrange for
> afterTriggers.transition_capture_states[afterTriggers.query_depth] to
> point to the same hash table as query_depth - 1, so that the effects
> of statements at this implementation-internal level appear to the user
> as part of the the level below?

That already happens, because query_depth doesn't increment for an FK
enforcement query --- we never call AfterTriggerBegin/EndQuery for it.

> 3.  Merging the invocation after statement firing so that if you
> updated the same table directly and also via a wCTE and also
> indirectly via fk ON DELETE/UPDATE trigger then you still only get one
> invocation of the after statement trigger.  Not sure exactly how...

What I did here was to use the AfterTriggersTableData structs to hold
a flag saying we'd already queued statement triggers for this rel and
cmdType.  There's probably more than one way to do that, but this seemed
convenient.

One thing I don't like too much about that is that it means there are
cases where the single statement trigger firing would occur before some
AFTER ROW trigger firings.  Not sure if we promise anything about the
ordering in the docs.  It looks quite expensive/complicated to try to
make it always happen afterwards, though, and it might well be totally
impossible if triggers cause more table updates to occur.

Because MakeTransitionCaptureState now depends on the trigger query
level being active, I had to relocate the AfterTriggerBeginQuery calls
to occur before it.

In passing, I refactored the AfterTriggers data structures a bit so
that we don't need to do as many palloc calls to manage them.  Instead
of several independent arrays there's now one array of structs.

			regards, tom lane

  23. Re: BUG #14808: V10-beta4, backend abort

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-15T00:12:35Z

    On Fri, Sep 15, 2017 at 9:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Attached is a draft patch for this.

Some initial feedback:

Compiles cleanly and make check-world passes here.

with wcte as (insert into table1 values (42))
   insert into table2 values ('hello world');
 NOTICE:  trigger = table2_trig, new table = ("hello world")
 NOTICE:  trigger = table1_trig, new table = (42)
+with wcte as (insert into table1 values (43))
+  insert into table1 values (44);
+NOTICE:  trigger = table1_trig, new table = (43), (44)

The effects of multiple ModifyTable nodes on the same table are
merged.  Good.  I doubt anyone will notice but it might warrant a note
somewhere that there is a user-visible change here: previously if you
did this (without TTs) your trigger would have fired twice.

+create trigger my_table_col_update_trig
+  after update of b on my_table referencing new table as new_table
+  for each statement execute procedure dump_insert();
+ERROR:  transition tables cannot be specified for triggers with column lists

Potential SQL standard non-compliance avoided.  Good.

 delete from refd_table where length(b) = 3;
-NOTICE:  trigger = trig_table_delete_trig, old table = (2,"two a"), (2,"two b")
-NOTICE:  trigger = trig_table_delete_trig, old table = (11,"one a"),
(11,"one b")
+NOTICE:  trigger = trig_table_delete_trig, old table = (2,"two a"),
(2,"two b"), (11,"one a"), (11,"one b")

The effects of fk cascade machinery are merged as discussed.  Good, I
think, but I'm a bit confused about how this works when the cascading
operation also fires triggers.

All the other tests show no change in behaviour.  Good.

What is going on here?

=== setup ===
create or replace function dump_delete() returns trigger language plpgsql as
$$
  begin
    raise notice 'trigger = %, old table = %, depth = %',
                 TG_NAME,
                 (select string_agg(old_table::text, ', ' order by a)
from old_table),
                 pg_trigger_depth();
    return null;
  end;
$$;
create table foo (a int primary key, b int references foo(a) on delete cascade);
create trigger foo_s_trig after delete on foo
  referencing old table as old_table
  for each statement
  execute procedure dump_delete();
create trigger foo_r_trig after delete on foo
  referencing old table as old_table
  for each row
  execute procedure dump_delete();
insert into foo values (1, null), (2, 1);
===8<===

postgres=# delete from foo where a = 1;
NOTICE:  trigger = foo_r_trig, old table = (1,), depth = 1
NOTICE:  trigger = foo_s_trig, old table = (1,), depth = 1
NOTICE:  trigger = foo_r_trig, old table = (2,1), depth = 1
NOTICE:  trigger = foo_s_trig, old table = (2,1), depth = 1
NOTICE:  trigger = foo_s_trig, old table = <NULL>, depth = 1
DELETE 1

> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> 1.  Merging the transition tables when there are multiple wCTEs
>> referencing the same table.  Here's one idea:  Rename
>> MakeTransitionCaptureState() to GetTransitionCaptureState() and use a
>> hash table keyed by table OID in
>> afterTriggers.transition_capture_states[afterTriggers.query_depth] to
>> find the TCS for the given TriggerDesc or create it if not found, so
>> that all wCTEs find the same TransitionCaptureState object.  The all
>> existing callers continue to do what they're doing now, but they'll be
>> sharing TCSs appropriately with other things in the plan.  Note that
>> TransitionCaptureState already holds tuplestores for each operation
>> (INSERT, UPDATE, DELETE) so the OID of the table alone is a suitable
>> key for the hash table (assuming we are ignoring the column-list part
>> of the spec as you suggested).
>
> It seems unsafe to merge the TCS objects themselves, because the callers
> assume that they can munge the tcs_map and tcs_original_insert_tuple
> fields freely without regard for any other callers.  So as I have it,
> we still have a TCS for each caller, but the TCSes point at tuplestores
> that can be shared across multiple callers for the same event type.
> The tuplestores themselves are managed by the AfterTrigger data
> structures.  Also, because the TCS structs are just throwaway per-caller
> data, it's uncool to reference them in the trigger event lists.
> So I replaced ats_transition_capture with two pointers to the actual
> tuplestores.

Ok, that works and yeah it may be conceptually better.  Also, maybe
the tcs_original_insert_tuple as member of TCS is a bit clunky and
could be reconsidered later: I was trying to avoid widening a bunch of
function calls, but that might have been optimising for the wrong
thing...

> That bloats AfterTriggerSharedData a bit but I think it's
> okay; we don't expect a lot of those structs in a normal query.

Yeah, it was bloat avoidance that led me to find a way to use a single
pointer.  But at least it's in the shared part, so I don't think it
matters too much.

> I chose to make the persistent state (AfterTriggersTableData) independent
> for each operation type.  We could have done that differently perhaps, but
> it seemed more complicated and less likely to match the spec's semantics.

OK.  Your GetAfterTriggersTableData(Oid relid, CmdType cmdType) is
mirroring the spec's way of describing what happens (without the
column list).

+       foreach(lc, qs->tables)
+       {
+               table = (AfterTriggersTableData *) lfirst(lc);
+               if (table->relid == relid && table->cmdType == cmdType &&
+                       !table->closed)
+                       return table;
+       }

Yeah, my suggestion of a hash table was overkill.  (Maybe in future if
we change our rules around inheritance this could finish up being
searched for a lot of child tables; we can cross that bridge when we
come to it.)

I'm a little confused about the "closed" flag.  This seems to imply
that it's possible for there to be more than one separate tuplestore
for a given (table, operation) in a given trigger execution context.
Why is that OK?

> The INSERT ON CONFLICT UPDATE mess is handled by creating two separate
> TCSes with two different underlying AfterTriggersTableData structs.
> The insertion tuplestore sees only the inserted tuples, the update
> tuplestores see only the updated-pre-existing tuples.  That adds a little
> code to nodeModifyTable but it seems conceptually much cleaner.

OK.  The resulting behaviour is unchanged.

>> 3.  Merging the invocation after statement firing so that if you
>> updated the same table directly and also via a wCTE and also
>> indirectly via fk ON DELETE/UPDATE trigger then you still only get one
>> invocation of the after statement trigger.  Not sure exactly how...
>
> What I did here was to use the AfterTriggersTableData structs to hold
> a flag saying we'd already queued statement triggers for this rel and
> cmdType.  There's probably more than one way to do that, but this seemed
> convenient.

Seems good.  That implements the following (from whatever random draft
spec I have): "A statement-level trigger that is considered as
executed for a state change SC (in a given trigger execution context)
is not subsequently executed for SC."

> One thing I don't like too much about that is that it means there are
> cases where the single statement trigger firing would occur before some
> AFTER ROW trigger firings.  Not sure if we promise anything about the
> ordering in the docs.  It looks quite expensive/complicated to try to
> make it always happen afterwards, though, and it might well be totally
> impossible if triggers cause more table updates to occur.

I suppose that it might be possible for AfterTriggersTableData to
record the location of a previously queued event so that you can later
disable it and queue a replacement, with the effect of suppressing
earlier firings rather than later ones.  That might make sense if you
think that after statement triggers should fire after all row
triggers.  I can't figure out from the spec whether that's expected
and I'm not sure if it's useful.

> Because MakeTransitionCaptureState now depends on the trigger query
> level being active, I had to relocate the AfterTriggerBeginQuery calls
> to occur before it.

Right.

> In passing, I refactored the AfterTriggers data structures a bit so
> that we don't need to do as many palloc calls to manage them.  Instead
> of several independent arrays there's now one array of structs.

Good improvement.

-- 
Thomas Munro
http://www.enterprisedb.com

  24. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-15T01:10:17Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Fri, Sep 15, 2017 at 9:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Attached is a draft patch for this.

> Some initial feedback:

> The effects of multiple ModifyTable nodes on the same table are
> merged.  Good.  I doubt anyone will notice but it might warrant a note
> somewhere that there is a user-visible change here: previously if you
> did this (without TTs) your trigger would have fired twice.

Check.  I've not yet looked at the documentation angle here.

> What is going on here?

Hmm ... that looks odd, but I'm too tired to debug it right now.

> Ok, that works and yeah it may be conceptually better.  Also, maybe
> the tcs_original_insert_tuple as member of TCS is a bit clunky and
> could be reconsidered later: I was trying to avoid widening a bunch of
> function calls, but that might have been optimising for the wrong
> thing...

Yeah, both tcs_map and tcs_original_insert_tuple feel like they ought
to be function parameters not persistent state.  But we can leave that
sort of refactoring for later.

> Yeah, my suggestion of a hash table was overkill.  (Maybe in future if
> we change our rules around inheritance this could finish up being
> searched for a lot of child tables; we can cross that bridge when we
> come to it.)

Exactly; I doubt it matters now, and if it ever does we can improve
the lookup infrastructure later.

> I'm a little confused about the "closed" flag.  This seems to imply
> that it's possible for there to be more than one separate tuplestore
> for a given (table, operation) in a given trigger execution context.
> Why is that OK?

The thought I had was that once we've fired any triggers that look at
a transition table, we don't want that table to change anymore; it'd
be bad if different triggers on the same event saw different tables.
So the "closed" flag is intended to shut off additional tuple additions.
If any happen (which I'm not sure if it's possible without a rogue
C-coded trigger), we'll establish a new set of transition tables for
them, and if relevant, fire AFTER STATEMENT triggers again to process
those tables.  So this sort of breaks the "one transition table and
one AFTER STATEMENT trigger firing per statement" concept, but the
alternative seems worse.  I hope that the case isn't reachable without
weird trigger behavior, though.

> I suppose that it might be possible for AfterTriggersTableData to
> record the location of a previously queued event so that you can later
> disable it and queue a replacement, with the effect of suppressing
> earlier firings rather than later ones.  That might make sense if you
> think that after statement triggers should fire after all row
> triggers.  I can't figure out from the spec whether that's expected
> and I'm not sure if it's useful.

Hm.  There could be more than one such event, but maybe we can make
something of that idea.  We could expect that the events in question
are consecutive in the event list, I think ...

			regards, tom lane

  25. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-15T15:28:34Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> What is going on here?
> ...
> insert into foo values (1, null), (2, 1);
>
> postgres=# delete from foo where a = 1;
> NOTICE:  trigger = foo_r_trig, old table = (1,), depth = 1
> NOTICE:  trigger = foo_s_trig, old table = (1,), depth = 1
> NOTICE:  trigger = foo_r_trig, old table = (2,1), depth = 1
> NOTICE:  trigger = foo_s_trig, old table = (2,1), depth = 1
> NOTICE:  trigger = foo_s_trig, old table = <NULL>, depth = 1
> DELETE 1

OK, now that I'm a little more awake, I looked at this, and I think it's
actually an instance of the "closed transition table" behavior you were
asking about.  Initially, we perform the delete of the (1,null) row,
and that queues AFTER ROW triggers -- both the RI enforcement one and
the explicit foo_r_trig one -- and puts the row into the transition table.
When the executor finishes that scan it queues the foo_s_trig AFTER
STATEMENT trigger.  Then we start to execute the triggers, and as I have
the patch now, it first marks all the transition tables closed.  I think
that the RI enforcement trigger fires before foo_r_trig on the basis of
name order, but it doesn't actually matter if it fires first or second.
Either way, it causes a cascaded delete of the (2,1) row, and again
we queue AFTER ROW triggers and put the row into the transition table.
But now, since the first transition table was already marked closed,
we create a new transition table and put (2,1) into it.  And then we
queue foo_s_trig, and since we're looking at a new (not closed)
AfterTriggersTableData struct, that's allowed to happen.  Then we
fire foo_r_trig and foo_s_trig referencing the original transition
table, which produce the first two NOTICE lines.  Then we repeat
this entire process with the newly queued triggers.  The second
invocation of the RI enforcement trigger doesn't find any rows to
delete, but it nonetheless causes queuing of a third AFTER STATEMENT
trigger, which eventually gets to run with an empty transition table.

So this is a bit annoying because we're marking the transition table
closed before the RI triggers can have the desired effects.  I wonder
if we can rejigger things so that the tables are only closed when
actually used.

			regards, tom lane

  26. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-15T19:26:35Z

    I wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> What is going on here?
>> ...
>> insert into foo values (1, null), (2, 1);
>> 
>> postgres=# delete from foo where a = 1;
>> NOTICE:  trigger = foo_r_trig, old table = (1,), depth = 1
>> NOTICE:  trigger = foo_s_trig, old table = (1,), depth = 1
>> NOTICE:  trigger = foo_r_trig, old table = (2,1), depth = 1
>> NOTICE:  trigger = foo_s_trig, old table = (2,1), depth = 1
>> NOTICE:  trigger = foo_s_trig, old table = <NULL>, depth = 1
>> DELETE 1

> OK, now that I'm a little more awake, I looked at this, and I think it's
> actually an instance of the "closed transition table" behavior you were
> asking about.

Attached is an updated patch that incorporates the ideas you suggested.
I added an extended version of this example, which has an additional
level of FK cascade happening:

insert into self_ref values (1, null), (2, 1), (3, 2);
delete from self_ref where a = 1;
NOTICE:  trigger = self_ref_r_trig, old table = (1,), (2,1)
NOTICE:  trigger = self_ref_r_trig, old table = (1,), (2,1)
NOTICE:  trigger = self_ref_s_trig, old table = (1,), (2,1)
NOTICE:  trigger = self_ref_r_trig, old table = (3,2)
NOTICE:  trigger = self_ref_s_trig, old table = (3,2)

What happens here is that the outer delete queues AR triggers for
RI enforcement and self_ref_r_trig, plus an AS trigger for
self_ref_s_trig.  Then the RI enforcement trigger deletes (2,1)
and queues AR+AS triggers for that.  At this point the initial
transition table is still open, so (2,1) goes into that table,
and we look back and cancel the previous queuing of self_ref_s_trig.
Now it's time for the first firing of self_ref_r_trig, and so now
we mark the transition table closed.  Then we skip the cancelled
self_ref_s_trig call, and then it's time for the second RI enforcement
trigger to fire, which deletes (3,2) but has to put it into a new
transition table.  Again we queue AR+AS triggers, but this time we
can't cancel the preceding AS call.  Then we fire self_ref_r_trig
again (for the (2,1) row), and then fire self_ref_s_trig; both of
them see the same transition table the first self_ref_r_trig call
did.  Now it's time for the third RI enforcement trigger; it finds
nothing to delete, so it adds nothing to the second transition table,
but it does queue an AS trigger call (canceling the one added by the
second RI trigger).  Finally we have the AR call queued by the second
RI trigger, and then the AS call queued by the third RI trigger,
both looking at the second transition table.

This is pretty messy but I think it's the best we can do as long as
RI actions are intermixed with other AFTER ROW triggers.  Maybe with
Kevin's ideas about converting RI actions to be statement-level,
we could arrange for all three deletions to show up in one transition
table ... but I don't know how we cause that to happen before the
user's AFTER ROW triggers run.  In any case, nothing will look odd
unless you have AR triggers using transition tables, which seems like
a niche usage case in the first place.

I also realized that we could undo the bloat I added to
AfterTriggerSharedData by storing a pointer to the AfterTriggersTableData
rather than the tuplestores themselves.

I feel that this is probably committable, except that I still need
to look for documentation changes.

			regards, tom lane

  27. Re: BUG #14808: V10-beta4, backend abort

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-09-16T01:45:27Z

    On Sat, Sep 16, 2017 at 7:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Attached is an updated patch that incorporates the ideas you suggested.

I was imagining that you would just need to keep a back pointer to the
last queued event for the same (relation, command), since that's the
only one you'd ever need to consider cancelling, and then no scanning
would be needed.  I am probably missing something.

> I added an extended version of this example, which has an additional
> level of FK cascade happening:
>
> insert into self_ref values (1, null), (2, 1), (3, 2);
> delete from self_ref where a = 1;
> NOTICE:  trigger = self_ref_r_trig, old table = (1,), (2,1)
> NOTICE:  trigger = self_ref_r_trig, old table = (1,), (2,1)
> NOTICE:  trigger = self_ref_s_trig, old table = (1,), (2,1)
> NOTICE:  trigger = self_ref_r_trig, old table = (3,2)
> NOTICE:  trigger = self_ref_s_trig, old table = (3,2)
>
> What happens here is that the outer delete queues AR triggers for
> RI enforcement and self_ref_r_trig, plus an AS trigger for
> self_ref_s_trig.  Then the RI enforcement trigger deletes (2,1)
> and queues AR+AS triggers for that.  At this point the initial
> transition table is still open, so (2,1) goes into that table,
> and we look back and cancel the previous queuing of self_ref_s_trig.
> Now it's time for the first firing of self_ref_r_trig, and so now
> we mark the transition table closed.  Then we skip the cancelled
> self_ref_s_trig call, and then it's time for the second RI enforcement
> trigger to fire, which deletes (3,2) but has to put it into a new
> transition table.  Again we queue AR+AS triggers, but this time we
> can't cancel the preceding AS call.  Then we fire self_ref_r_trig
> again (for the (2,1) row), and then fire self_ref_s_trig; both of
> them see the same transition table the first self_ref_r_trig call
> did.  Now it's time for the third RI enforcement trigger; it finds
> nothing to delete, so it adds nothing to the second transition table,
> but it does queue an AS trigger call (canceling the one added by the
> second RI trigger).  Finally we have the AR call queued by the second
> RI trigger, and then the AS call queued by the third RI trigger,
> both looking at the second transition table.
>
> This is pretty messy but I think it's the best we can do as long as
> RI actions are intermixed with other AFTER ROW triggers.  Maybe with
> Kevin's ideas about converting RI actions to be statement-level,
> we could arrange for all three deletions to show up in one transition
> table ... but I don't know how we cause that to happen before the
> user's AFTER ROW triggers run.  In any case, nothing will look odd
> unless you have AR triggers using transition tables, which seems like
> a niche usage case in the first place.

It does seem like an inconsistency that it would be good to fix, but I
don't immediately see how to make that happen with the current design.
It would be interesting to know what DB2 does here in terms of trigger
execution contexts and transition tables when you have a chain of 2, 3
and 4 foreign key referential actions.

Is it worth adding a test with an extra level of chaining in the self_ref case?

Is it worth adding tests for SET NULL and SET DEFAULT, to exercise the
complete set of referential actions?

> I also realized that we could undo the bloat I added to
> AfterTriggerSharedData by storing a pointer to the AfterTriggersTableData
> rather than the tuplestores themselves.

Makes sense.

> I feel that this is probably committable, except that I still need
> to look for documentation changes.

+1

-- 
Thomas Munro
http://www.enterprisedb.com

  28. Re: BUG #14808: V10-beta4, backend abort

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-16T03:38:20Z

    Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Sat, Sep 16, 2017 at 7:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Attached is an updated patch that incorporates the ideas you suggested.

> I was imagining that you would just need to keep a back pointer to the
> last queued event for the same (relation, command), since that's the
> only one you'd ever need to consider cancelling, and then no scanning
> would be needed.  I am probably missing something.

There could be more than one after-statement trigger, no?

>> This is pretty messy but I think it's the best we can do as long as
>> RI actions are intermixed with other AFTER ROW triggers.

> It does seem like an inconsistency that it would be good to fix, but I
> don't immediately see how to make that happen with the current design.
> It would be interesting to know what DB2 does here in terms of trigger
> execution contexts and transition tables when you have a chain of 2, 3
> and 4 foreign key referential actions.

> Is it worth adding a test with an extra level of chaining in the self_ref case?

Would it show anything not shown by the three-level case?

> Is it worth adding tests for SET NULL and SET DEFAULT, to exercise the
> complete set of referential actions?

I think they're all about the same as far as this is concerned.

			regards, tom lane