Re: BUG #17798: Incorrect memory access occurs when using BEFORE ROW UPDATE trigger

Thanks for taking care of this.

On Sat, Jan 13, 2024 at 6:29 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> So I'm now convinced that all we need is an ExecMaterializeSlot
> call in the EPQ path, much as in Alexander's first patch.
>
> I don't think that the changes in the v4 patch are improvements.
> I really do not like
>
> + newslot = ExecGetUpdateNewTuple(relinfo, epqslot_candidate,
> + oldslot);
>
> and here's why: it is critical that we update the caller's slot,
> because that's the only way trigger-driven changes will get back to
> ExecModifyTable. This coding obscures what's happening, and would
> break badly if ExecGetUpdateNewTuple ever acted differently than it
> does now or if a caller passed a slot that wasn't the same slot (which
> I'm not convinced never happens; see ExecSimpleRelationUpdate for what
> looks like a counterexample). And it's not even saving anything
> meaningful.

I find the way we program with slots to be unintuitive and, indeed,
outright confusing. Your comment here is about making sure that the
slot to which newslot pointed on entry to the function ends up with
the correct contents, which is fair enough. But the naive reader could
be forgiven for thinking that epqslot_clean =
ExecGetUpdateNewTuple(relinfo, epqslot_candidate, oldslot) doesn't
affect the validity of newslot, and it definitely does. It seems like
ExecBRUpdateTriggers critically depends on the fact that on entry,
oldslot is fully valid, whereas newslot may depend on oldslot ... but
that isn't clearly documented. Nor is the fact that the original
newslot must itself get updated for the benefit of the caller. There's
no header comment explaining what's supposed to be true on entry and
on exit -- you just have to know how it's supposed to work. And it
seems to me that we have a ton of slot-related code that's like that.

You might think for example that each executor node type would have
comments explaining what slots it used, what tuple descriptor each one
uses and why, and the anticipated lifetime of each slot. But you often
don't get that, or at least not very clearly. nodeIndexonlyscan.c
actually has some comments in this area that are pretty good ("We need
another slot, in a format that's suitable for the table AM, for when
we need to fetch a tuple from the table for rechecking visibility.")
But nodeMergejoin.c just kinda does a bunch of stuff without really
explaining anything, and unfortunately I think that's more typical.

I'm not blaming you for any of this, nor asking you to fix anything,
unless you feel so moved. I'm just explaining how I feel about it. I
have a feeling that if I had started hacking on PostgreSQL a decade or
two earlier when the code was simpler, my understanding probably would
have grown up with the code and this would all make more sense to me.
But I've "only" been around since 2008!

--
Robert Haas
EDB: http://www.enterprisedb.com