Re: [HACKERS] PG10 transition tables, wCTEs and multiple operations on the same table

On Sun, Jun 4, 2017 at 10:41 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Fri, Jun 2, 2017 at 8:46 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>
>> So, afterTriggers.query_stack is used to handle the reentrancy that
>> results from triggers running further statements that might fire
>> triggers.  It isn't used for dealing with extra ModifyTable nodes that
>> can appear in a plan because of wCTEs.  Could it also be used for that
>> purpose?  I think that would only work if it is the case that each
>> ModifyTable node begin and then runs to completion (ie no interleaving
>> of wCTE execution) and then its AS trigger fires, which I'm not sure
>> about.
>
> I don't think we want to commit to anything that depends on a CTE
> creating an optimization fence, although *maybe* that would be OK in
> the case of DML as a CTE.  That's a pretty special case; I'm not
> sure whether the standard discusses it.

According to our manual the standard doesn't allow DML in CTEs.  I
suppose it wouldn't make much sense without RETURNING, which is also
non-standard.

>> If that is the case, perhaps AfterTriggerBeginQuery and
>> AfterTriggerEndQuery could become the responsibility of
>> nodeModifyTable.c rather than execMain.c.  I haven't tried this yet
>> and it may well be too ambitious at this stage.
>
> In a world where one statement can contain multiple DML statements
> within CTEs, that may make sense regardless of transition table
> issues; but I agree we're past the time to be considering making
> such a change for v10.

Ok.

>> Other ideas: (1) ban wCTEs that target relations with transition
>> tables in PG10, because we're out of time;
>
> Given that we haven't even discussed what to do about an UPDATE
> statement with a CTE that updates the same table when there are
> BEFORE EACH ROW UPDATE triggers on that table (which perhaps return
> NULL for some but not all rows?), I'm not sure we've yet plumbed the
> depths of this morass.
>
> [...]
>
> Can we fix things exclusive of transition tables in this release?
> If not, the only reasonable thing to do is to try to avoid further
> complicating things with CTE/transition table usage until we sort
> out the basics of triggers firing from CTE DML in the first place.

At least it's well documented that the execution order is undefined,
but yeah triggers do make things a lot more complicated and I'm not
sure I understand to what extent that is unavoidable once you decide
to allow DML in CTEs.

In the meantime, it seems like you agree that rejecting wCTEs that
affect tables with triggers with transition tables is the best
response to this bug report?  Do you think that parse analysis is the
right time to do the check?  Here's a first attempt at that.

Later, for the next cycle, I think we should investigate storing the
transition tables in ModifyTableState rather than in query_stack, and
then passing them to the Exec*Triggers() functions as arguments
(possibly inside TransitionCaptureState, if you accept my proposal for
the inheritance bug).

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers