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

Hello Robert,

06.01.2024 00:18, Robert Haas wrote:
> On Sun, Jul 9, 2023 at 11:00 AM Alexander Lakhin <exclusion(at)gmail(dot)com> wrote:
>> I've found enough time this week to investigate the issue deeper and now
>> I can answer your questions, hopefully.
> This patch lacks a commit message, which I think would be a good thing
> to add. Even if somebody doesn't use the message you write, it would
> help with understanding the patch itself. Deleting the call to
> ExecMaterializeSlot with no explanation in the patch file of why
> that's OK to do is not ideal.

Thanks for looking into this!

I've added a commit message, please look at the new patch version.

> But the real heart of this small patch
> seems to be this:
>
> + /*
> + * We need to materialize newslot because 1) it might
> share oldslot's,
> + * buffer, which might be released on fetching
> trigtuple from oldslot
> + * if oldslot is the only owner of buffer,
> + * and 2) if some trigger returns/stores the old trigtuple,
> + * and the heap tuple passed to the trigger was located locally,
> + * newslot might reference that tuple storage, which
> is freed at the
> + * end of this function.
> + */
>
> Reading this, I found (1) to be very surprising. I would expect that
> advancing the scan would potentially release the buffer pin, but
> fetching the tuple shouldn't advance the scan. I guess this is
> referring to the call, just below, to ExecFetchSlotHeapTuple. I'm
> guessing that can end up calling tts_buffer_heap_materialize which,
> after materializing copying the tuple, thinks that it's OK to release
> the buffer pin.

Yes, this is what happening there as I diagnosed in [1].

> But that makes me wonder ... how exactly do oldslot
> and newslot end up sharing the same buffer without holding two pins on
> it? tts_heap_copyslot() looks like it basically forces the tuple to be
> materialized first and then copies that, so you'd end up with, I
> guess, no buffer pins at all, rather than 1 or 2.
>
> I'm obviously missing something important here...

As my analysis [2] showed, epqslot_clean is always equal to newslot, so
ExecCopySlot() isn't called at all.

[1] https://www.postgresql.org/message-id/17798-0907404928dcf0dd%40postgresql.org
[2] https://www.postgresql.org/message-id/e989408c-1838-61cd-c968-1fcf47c6fbba%40gmail.com

Best regards,
Alexander