Thread: Memory contexts reset for trigger invocations
Hi,
trigger.c goes through some trouble to free the tuples returned by
trigger functions. There's plenty codepaths that look roughly like:
if (oldtuple != newtuple && oldtuple != slottuple)
heap_freetuple(oldtuple);
if (newtuple == NULL)
{
if (trigtuple != fdw_trigtuple)
heap_freetuple(trigtuple);
return NULL; /* "do nothing" */
}
but we, as far as I can tell, do not reset the memory context in which
the trigger functions have been called.
Wouldn't it be better to reset an appropriate context after each
invocation? Yes, that'd require some care to manage the lifetime of
tuples returned by triggers, but that seems OK?
I get that most tables don't have dozens of triggers, but for more
complicated triggers/functions even a few seem like they'd matter?
Greetings,
Andres Freund
On Tue, Feb 5, 2019 at 4:29 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
trigger.c goes through some trouble to free the tuples returned by
trigger functions. There's plenty codepaths that look roughly like:
if (oldtuple != newtuple && oldtuple != slottuple)
heap_freetuple(oldtuple);
if (newtuple == NULL)
{
if (trigtuple != fdw_trigtuple)
heap_freetuple(trigtuple);
return NULL; /* "do nothing" */
}
but we, as far as I can tell, do not reset the memory context in which
the trigger functions have been called.
Wouldn't it be better to reset an appropriate context after each
invocation? Yes, that'd require some care to manage the lifetime of
tuples returned by triggers, but that seems OK?
Currently the trigger functions are executed under per tuple memory
context, but the returned tuples are allocated from the executor memory
context in some scenarios.
/*
* Copy tuple to upper executor memory. But if user just did
* "return new" or "return old" without changing anything, there's
* no need to copy; we can return the original tuple (which will
* save a few cycles in trigger.c as well as here).
*/
if (rettup != trigdata->tg_newtuple &&
rettup != trigdata->tg_trigtuple)
rettup = SPI_copytuple(rettup);
* Copy tuple to upper executor memory. But if user just did
* "return new" or "return old" without changing anything, there's
* no need to copy; we can return the original tuple (which will
* save a few cycles in trigger.c as well as here).
*/
if (rettup != trigdata->tg_newtuple &&
rettup != trigdata->tg_trigtuple)
rettup = SPI_copytuple(rettup);
we need to take care of these also before switch to a context?
Regards,
Haribabu Kommi
Fujitsu Australia
Andres Freund <andres@anarazel.de> writes:
> Wouldn't it be better to reset an appropriate context after each
> invocation? Yes, that'd require some care to manage the lifetime of
> tuples returned by triggers, but that seems OK?
I'm not sure that we can change much here without effectively breaking
the trigger-function API. I doubt that there's enough win to be had
to justify forcing external PLs etc to change.
Having said that, I recall that that API was kind of a pain in the
rear when I was redoing plpgsql's handling of composite variables
last year. Don't recall details right now (ENOCAFFEINE). Maybe
a wholesale rethink would be justified? But I'm not excited about
just twiddling things at the margin.
regards, tom lane
Hi, On February 5, 2019 7:55:28 PM GMT+05:30, Tom Lane <tgl@sss.pgh.pa.us> wrote: >Andres Freund <andres@anarazel.de> writes: >> Wouldn't it be better to reset an appropriate context after each >> invocation? Yes, that'd require some care to manage the lifetime of >> tuples returned by triggers, but that seems OK? > >I'm not sure that we can change much here without effectively breaking >the trigger-function API. HM, why? If we copy the tuple into a longer lived context we ought to be able to reset more aggressively. I doubt that there's enough win to be had >to justify forcing external PLs etc to change. > >Having said that, I recall that that API was kind of a pain in the >rear when I was redoing plpgsql's handling of composite variables >last year. Don't recall details right now (ENOCAFFEINE). Maybe >a wholesale rethink would be justified? But I'm not excited about >just twiddling things at the margin. Yea, we probably ought to: The pluggable storage patch set makes trigger.c use slots. But the trigger interface requiresheap tuples, so we extract heap tuples. But probably it'd be go full in slots at some point. Regards, Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.