Re: [HACKERS] PG10 transition tables, wCTEs and multiple operationson the same table - Mailing list pgsql-hackers

On Mon, Jun 12, 2017 at 9:29 AM, Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
>>>>>> "Robert" == Robert Haas <robertmhaas@gmail.com> writes:
>
>  Robert> I don't see a reason why MakeTransitionCaptureState needs to
>  Robert> force the tuplestores into TopTransactionContext or make them
>  Robert> owned by TopTransactionResourceOwner.
>
> Nor do I, and I'm pretty sure it's leaking memory wholesale within a
> transaction if you have aborted subxacts.
>
> e.g. a few iterations of this, on a table with an appropriate trigger:
>
> savepoint s1;
> insert into foo
>   select i, case when i < 100000 then repeat('a',100)
>             else repeat('a',1/(i-100000)) end
>     from generate_series(1,100000) i;
> rollback to s1;
>
> This is a bit contrived of course but it shows that there's missing
> cleanup somewhere, either in the form of poor choice of context or
> missing code in the subxact cleanup.

Right, thanks.  I think the fix for this is to use
CurTransactionContext (for memory cleanup) and
CurTransactionResourceOwner (for temporary file cleanup, in case the
tuplestores spill to disk).  The attached does it that way.

With the previous version of the patch, I could see temporary files
accumulating under base/pgsql_tmp from each aborted subtransaction
when I set work_mem = '64kB' to provoke spillage.  With the attached
version, the happy path cleans up (because ExecEndModifyTable calls
DestroyTransitionCaptureState), and the error path does too (because
the subxact's memory and temporary files are automatically cleaned
up).

>  Robert> I mean, it was like that before, but afterTriggers is a global
>  Robert> variable and, potentially, there could still be a pointer
>  Robert> accessible through it to a tuplestore linked from it even after
>  Robert> the corresponding subtransaction aborted, possibly causing some
>  Robert> cleanup code to trip and fall over.  But that can't be used to
>  Robert> justify this case, because the TransitionCaptureState is only
>  Robert> reached through the PlanState tree; if that goes away, how is
>  Robert> anybody going to accidentally follow a pointer to the
>  Robert> now-absent tuplestore?
>
> For per-row triggers with transition tables, a pointer to the transition
> capture state ends up in the shared-data record in the event queue?

Yes.  But the only way for event queue data to last longer than the
execution of the current statement is if it's a deferred trigger.
Only constraint triggers can be deferred, and constraint triggers are
not allowed to have transition tables.  If we wanted to support that
(for example, to handle set-orient FK checks as has been discussed),
we'd need to change the lifetime of the TransitionCaptureState object
(it would need to outlast ModifyTableState).  I've added a note to
that effect to MakeTransitionCaptureState.

So far there seem to be no objections to the inclusion of the pointer
in the event queue that records which transition tables each trigger
invocation should see...?  The extra memory consumption per chunk is
limited by the number of ModifyTable nodes, so it's not going to be a
memory hog, but I thought someone might not like it on other grounds.

On Sat, Jun 10, 2017 at 7:48 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> 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)?

Yeah, that was silly, and also the comment was misleading.  It is
possible for row_trigger to be true and transition_capture to be NULL,
so I did it slightly differently.  Fixed in the attached.

Thanks both for the review.  New version of patch #2 attached.

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