Re: BUG #19536: UPDATE RETURNING OLD value is stale after concurrent update when table has a BEFORE UPDATE trigger

On Mon, 29 Jun 2026 at 00:35, Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Thanks for the patch! This matches the fix I had in mind after my
> analysis as well.
>
> A couple of comments:
>
> 1/ I think the refetch (which is only needed when RETURNING references
> the OLD tuple) seems unnecessary. Fetching the row version comes with
> an additional buffer pool lookup (possibly a cache miss), a buffer
> pin, etc. Could we instead reuse the tuple that GetTupleForTrigger has
> already fetched? I might be overthinking the performance overhead
> here, but on busy production systems we can't ignore these costs.
> Others may have a different take.

Thanks for reviewing.

I did think of that, but I was uneasy about copying the trigger's old
tuple after trigger invocation, in case the trigger scribbled on it
somehow. I think that's not possible, but it just didn't seem good
practice to rely on it being unchanged. ExecBRUpdateTriggers() could
copy and return oldslot before invoking the triggers, but that would
require changing its API. In the end, I think that in the context of
having just waited for the other session to commit or rollback, and
then executing trigger code, the cost of refetching the tuple ought to
be negligible, so trying to reuse the tuple that the trigger code
fetched feels like an unnecessary optimisation.

Actually, I think a neater long-term solution would be to move all the
EPQ rechecking and re-computing of the new tuple out of trigger.c, and
let the caller deal with it, as we do for the MERGE code, or perhaps
even make the caller responsible for locking the old tuple, but that's
beyond the scope of this kind of bug fix.

> 2/
> + context->tmfd.traversed = false;
> if (!ExecUpdatePrologue(context, resultRelInfo, tupleid, oldtuple,
> slot, NULL))
> return NULL;
>
> Setting traversed to false here seems a bit off. Is this needed
> because the caller doesn't initialize ModifyTableContext properly?

Yes, that's right. The caller does not initialise context->tmfd at
all, and ExecUpdatePrologue() doesn't touch it if there are no
triggers. The other way to do it would have been to make
ExecUpdatePrologue() always set it, but I prefer this way, since it's
more obvious that ExecUpdate() is responding to ExecUpdatePrologue()
changing it (kind-of like setting errno to 0 before calling a function
that might set it to a non-zero value).

> 3/
> > This doesn't affect DELETE, because the DELETE code always fetches the
> > most recent version of the old tuple just before processing the
> > RETURNING clause.
> >
> > Similarly, it doesn't affect a cross-partition
> > UPDATE, which does a DELETE followed by an INSERT.
> >
> > It also doesn't affect MERGE UPDATE/DELETE, because that does its own
> > EPQ handling, rather than relying on the trigger code to do it (see
> > 9321c79c86e).
>
> I haven't checked these paths in depth, but I think it's worth adding
> tests for all of them.

OK, I've added a bunch of tests for those cases. As expected, they
pass even without the code changes, but it's good to confirm that.

> 4/
> + if (context->tmfd.traversed)
> + {
> + if (!table_tuple_fetch_row_version(resultRelInfo->ri_RelationDesc,
> + tupleid,
> + SnapshotAny,
> + oldSlot))
> + elog(ERROR, "failed to fetch tuple being updated");
> + }
>
> Could we add some context to the error message, e.g., mentioning that
> this happens after processing the BEFORE UPDATE row triggers? That
> would make it easier to distinguish from the other similar ones.

I'm pretty sure this is a can't-happen error condition, but OK.

Regards,
Dean