Re: BUG #5798: Some weird error with pl/pgsql procedure - Mailing list pgsql-bugs

Maxim Boguk <maxim.boguk@gmail.com> writes:
> Yes here is one BEFORE UPDATE trigger on relation
> ...
> However that trigger should not fire at all because storable procedure work
> with set local session_replication_role to 'replica';

Would be nice if you'd provided those sorts of details earlier, instead
of us having to resort to interrogation to obtain them.

> I have an idea that
>    2033                 if (newtuple != tuple)  /* modified by Trigger(s) */
> sometime can return true even if no triggers really had been done.

No, you've got that backwards: if that test did succeed, the if-body
would guarantee that we have a materialized slot.  I think though that
I see what is going on:

1.  The slot passed to ExecUpdate is always going to be
estate->es_junkFilter->jf_resultSlot, as a consequence of the fact that
ExecutePlan will always have invoked ExecFilterJunk just above.

2. ExecUpdate forces the slot into the materialized state and sets its
local "tuple" variable to the address of the physical tuple contained in
the slot.

3. ExecBRUpdateTriggers runs and tries to lock the target tuple.
Occasionally, there is a concurrent update that's just committed,
in which case GetTupleForTrigger runs EvalPlanQual and gives back
a new tuple-to-be-stored in a different slot.  When that happens,
ExecBRUpdateTriggers cleans the tuple by applying ExecRemoveJunk
... using estate->es_junkFilter.

4. execJunk.c does ExecClearTuple on its result slot.  At this point,
ExecUpdate's local tuple variable is a dangling pointer, because we
just freed the tuple it is pointing at.

5. ExecFilterJunk's result is a virtual tuple, but ExecRemoveJunk
does ExecCopySlotTuple -> heap_form_tuple to produce a copied tuple
that isn't owned by the slot.  However, if you're unlucky and the phase
of the moon is just right, that copied tuple may get palloc'd in the
same place where we just pfree'd the tuple that had been owned by the
jf_resultSlot.

6. ExecBRUpdateTriggers falls through (not finding any triggers that
will actually fire) and returns the copied tuple to ExecUpdate.
Normally, ExecUpdate would stuff the copied tuple into
es_trig_tuple_slot and all would be well, because that slot would surely
be in materialized state.  However, if you were sufficiently unlucky
above, the "newtuple != tuple" test fails because of pointer equality
between the dangling local variable and the live copied tuple.

7. At this point ExecUpdate has a physical tuple that is what it is
supposed to store, so that's fine, and furthermore the slot contains
the right data values for all the user columns.  So the only visible
problem is that if you try to extract ctid or other system columns from
the slot, that will fail, because the physical tuple isn't owned by the
slot.  I think though that worse problems could ensue if there were
active triggers that modified the row --- if you were really *seriously*
unlucky, there could be a chance pointer match between the dangling
tuple variable and a modified tuple returned by a trigger.  In that case
the modified tuple would be what was stored, but the index entries
generated for it would be from the pre-trigger slot values.

Ugh.  That quick little "ExecRemoveJunk" is a lot more dangerous than it
looks.  I had actually looked at this before, but concluded it was OK
because I couldn't reproduce the problem with a trigger in place.
I guess I wasn't unlucky enough to have the chance pointer equality
occur.

The narrowest way to fix this would be to add a condition to the
"newtuple != tuple" test in ExecUpdate to verify that "tuple" still
matches what is stored in the slot.  That's pretty ugly though.

What would likely be better is to rearrange things so that
ExecBRUpdateTriggers doesn't change the es_junkFilter's state.  Or we
could have ExecBRUpdateTriggers store the tuple into es_trig_tuple_slot
for itself and return that slot, thus eliminating the need for a
pointer-equality-dependent test to see if something had been done.
Either way seems like a significantly more invasive patch, but it might
be less likely to break again in future.

            regards, tom lane