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

On Fri, Jun 9, 2017 at 12:00 AM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Wed, Jun 7, 2017 at 5:36 PM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>> I spent a couple of hours drafting a proof-of-concept to see if my
>> hunch was right. It seems to work correctly so far and isn't huge
>> (but certainly needs more testing and work):
>>
>> 6 files changed, 156 insertions(+), 109 deletions(-)
>
> [Adding Andrew Gierth]
>
> Here is a new version of the patch to fix transition tables with
> wCTEs, rebased on top of
> transition-tuples-from-child-tables-v10.patch[1] which must be applied
> first.
>
> This is patch 2 of a stack of 3 patches addressing currently known
> problems with transition tables.

I don't see a reason why MakeTransitionCaptureState needs to force the
tuplestores into TopTransactionContext or make them owned by
TopTransactionResourceOwner. I mean, it was like that before, but
afterTriggers is a global variable and, potentially, there could still
be a pointer accessible through it to a tuplestore linked from it even
after the corresponding subtransaction aborted, possibly causing some
cleanup code to trip and fall over. But that can't be used to justify
this case, because the TransitionCaptureState is only reached through
the PlanState tree; if that goes away, how is anybody going to
accidentally follow a pointer to the now-absent tuplestore?

The structure of AfterTriggerSaveEvent with this patch applied looks
pretty weird. IIUC, whenever we enter the block guarded by if
(row_trigger), then either (a) transition_capture != NULL or (b) each
of the four "if" statements inside that block will separately decide
to do nothing. I think now that you're requiring a transition_capture
for all instances where transition tuplestores are used, you could
simplify this code. Maybe just change the outer "if" statement to if
(row_trigger) and then Assert(transition_capture != NULL)?

Not this patch's problem directly, but while scrutinizing this it
crossed my mind that we would need to prohibit constraint triggers
with transition tables. It turns out that we do, in the parser:

create constraint trigger table2_trig
after insert on table2 referencing new table as new_table
for each statement execute procedure dump_insert();
ERROR: syntax error at or near "referencing"

Possibly it would be better to allow the syntax and error out in
CreateTrigger(), so that we can give a better error message. It's
certainly not obvious from the syntax summary produced by \h CREATE
TRIGGER why this doesn't work. This might be more future-proof, too,
if somebody someday adds support for REFERENCING { OLD | NEW } ROW to
constraint triggers and fails to realize that there's not a check
anywhere outside the parser for the table case.

I don't see anything else wrong with this, and it seems like it solves
the reported problem.

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