Обсуждение: [HACKERS] Pre-existing bug in trigger.c

While fooling with the transition-tables bug, I noticed a problem
in trigger.c that has been there a very long time.  AfterTriggerEndQuery
correctly notes
    * ... Be careful here: firing a    * trigger could result in query_stack being repalloc'd, so we can't save    *
itsaddress across afterTriggerInvokeEvents calls.
 

However, it then proceeds to pass the address of a query_stack item to
afterTriggerInvokeEvents, so that if such a repalloc occurs,
afterTriggerInvokeEvents is already working with an obsolete dangling
pointer while it scans the rest of the events.

This isn't very trivial to fix.  I thought of making a local copy of
the events pointer struct, but that's problematic because data can
pass in a couple of directions here.  Queuing of additional trigger
events would modify the events list from "outside", while
afterTriggerInvokeEvents occasionally wants to do
        if (chunk == events->tail)            events->tailfree = chunk->freeptr;

which has to be in sync with the real state of the events list
including any subsequent additions.

I think possibly the best solution is to change the query_stack
data structure enough so that pre-existing entries don't get
moved during an enlargement.  Or maybe we can fix it so the
"active" events list has a static location and the items in the
query_stack are only valid for levels below the top.

Given the lack of reports traceable to this, this doesn't seem
super urgent to fix, so I'm not going to try to address it while
hacking the transition table business.  But we'll need to return
to it later.  Whatever we do about it has to be back-patched
further than v10, anyway.
        regards, tom lane


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

I wrote:
> While fooling with the transition-tables bug, I noticed a problem
> in trigger.c that has been there a very long time.
> ...
> I think possibly the best solution is to change the query_stack
> data structure enough so that pre-existing entries don't get
> moved during an enlargement.  Or maybe we can fix it so the
> "active" events list has a static location and the items in the
> query_stack are only valid for levels below the top.

After studying this awhile, I've concluded that neither of those
ideas leads to a fix simple enough that I'd be comfortable with
back-patching it.  What seems like the best answer is to not pass
delete_ok = true to afterTriggerInvokeEvents in AfterTriggerEndQuery.
Then, afterTriggerInvokeEvents will examine the passed event list
pointer only during its loop initialization, at which time it's
surely still valid.

This loses a bit of efficiency in the situation where we have to
loop back because more events got added to the event list during
trigger execution: we're then looking at having to scan over all
the old events a second time in afterTriggerMarkEvents and
afterTriggerInvokeEvents.  But there's another solution to that,
which is just about as efficient, if not more so: if we'll need
to repeat the loop, we can first delete all event chunks before
the one that was the "tail" chunk at the head of the loop.  We
know that afterTriggerMarkEvents marked all events then in
existence as either firable or done, so any events remaining to
process must have been added since then, and therefore are in the
old "tail" chunk or a later one.  This way turns out to get rid
of exactly the same events that the delete_ok = true way did,
except in the narrow corner case that the old "tail" chunk was
completely full at the top of the loop, so that all the new events
are in new chunks.  (If it was only partly full, then it must
now contain both old and new events, so that afterTriggerInvokeEvents
could not have cleared it anyway.)  That's close enough for me.
We could possibly cover even that case by noting whether or not the tail
chunk's freeptr moved, but I think it's too much complication for too
little chance of gain.
        regards, tom lane


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

Tom Lane wrote:

> After studying this awhile, I've concluded that neither of those
> ideas leads to a fix simple enough that I'd be comfortable with
> back-patching it.  What seems like the best answer is to not pass
> delete_ok = true to afterTriggerInvokeEvents in AfterTriggerEndQuery.
> Then, afterTriggerInvokeEvents will examine the passed event list
> pointer only during its loop initialization, at which time it's
> surely still valid.

For reference: this fix was committed as 27c6619e9c8f.

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


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