On Sun, Jul 9, 2023 at 11:00 AM Alexander Lakhin <exclusion@gmail.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. 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. 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...
--
Robert Haas
EDB: http://www.enterprisedb.com