From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alexander Lakhin <exclusion@gmail.com>
cc: pgsql-bugs@lists.postgresql.org, Richard Guo <guofenglinux@gmail.com>,
        Andres Freund <andres@anarazel.de>
Subject: Re: BUG #17798: Incorrect memory access occurs when using BEFORE ROW
 UPDATE trigger
In-reply-to: <a4a8cf37-e9d5-3c58-fc2e-d217cf66f315@gmail.com>
References: <17798-0907404928dcf0dd@postgresql.org>
 <87fe6c28-80b2-f709-a8db-d105e5240e39@gmail.com>
 <CAMbWs4_pg7ab=OJF2CGLzs=noWEjujerxfo92sm3RL9ce=ogsQ@mail.gmail.com>
 <CAMbWs48FHhuuuWbxygO4Wey7ahdaSwRjzJODKXx7=GFsge_aLg@mail.gmail.com>
 <f5ff446f-72e2-44f7-80c6-4256cf7caeb3@gmail.com>
 <a4a8cf37-e9d5-3c58-fc2e-d217cf66f315@gmail.com>
Comments: In-reply-to Alexander Lakhin <exclusion@gmail.com>
	message dated "Sat, 29 Apr 2023 12:00:00 +0300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1849609.1682807087.1@sss.pgh.pa.us>
Date: Sat, 29 Apr 2023 18:24:47 -0400
Message-ID: <1849610.1682807087@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1849610.1682807087%40sss.pgh.pa.us>
Precedence: bulk

Alexander Lakhin <exclusion@gmail.com> writes:
> Could you please confirm the issue, Richard's and my conclusions, and the
> correctness of the patch, in light of the upcoming May releases?
> Maybe I'm wrong, but I think that this bug can lead to data corruption in
> databases where BRU triggers are used.

Clearly it could, though the probability seems low since the just-released
buffer would have to get recycled very quickly.

I am not entirely comfortable with blaming this on 86dc90056 though,
even though "git bisect" seems to finger that.  The previous coding
there with ExecFilterJunk() also produced a virtual tuple, as you
can see by examining ExecFilterJunk, so why didn't the problem
manifest before?  I think the answer may be that before 86dc90056,
we were effectively relying on the underlying SeqScan node to keep
the buffer pinned, but now we're re-fetching the tuple and no pin
is held by lower plan nodes.  I'm not entirely sure about that though;
ISTM a SeqScan ought to hold pin on its current tuple in any case.
However, if the UPDATE plan were more complicated (involving a join)
then it's quite believable that we'd get here with no relevant pin
being held.

The practical impact of that concern is that I'm not convinced that
the proposed patch fixes all variants of the issue.  Maybe we need
to materialize even if newslot != epqslot_clean.  Maybe we need to
do it even if there wasn't a concurrent update.  Maybe we need to
do it in pre-v14 branches, too.  It's certainly not obvious that this
function ought to assume anything about which slots are pointing at
what.

Also, if we do materialize here, does this code a little further down
become redundant?

			if (should_free_trig && newtuple == trigtuple)
				ExecMaterializeSlot(newslot);

A completely different way of looking at it is that we should not
treat this as ExecBRUpdateTriggers's bug at all.  The slot mechanisms
are supposed to protect the data referenced by a slot, so why is that
failing to happen in this example?  The correct fix might involve
newslot acquiring a buffer pin, for example.

Bottom line is that I'm afraid the present patch is band-aiding a
specific problem without solving the general case.

			regards, tom lane