From: Alexander Lakhin <exclusion@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org, Richard Guo <guofenglinux@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Peter Geoghegan <pg@bowt.ie>
Subject: Re: BUG #17798: Incorrect memory access occurs when using BEFORE ROW UPDATE trigger
Date: Wed, 10 Jan 2024 21:00:01 +0300
Message-ID: <74335d44-819d-60e0-859a-efe74d9c84c2@gmail.com> (raw)
In-Reply-To: <CA+TgmoYAsQWMqMUNNdyp8V0KVxMoz=0y+3XNCf3ST-o1yyz5Jg@mail.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>
<1849610.1682807087@sss.pgh.pa.us>
<e989408c-1838-61cd-c968-1fcf47c6fbba@gmail.com>
<CA+TgmoZz1fGaTnOSf1yQhwYovEEkJZ9iioQXnLgaJz+1fBip=g@mail.gmail.com>
<494cd7c4-c109-6886-c0e3-138a95e7e38b@gmail.com>
<CA+TgmoYAsQWMqMUNNdyp8V0KVxMoz=0y+3XNCf3ST-o1yyz5Jg@mail.gmail.com>
08.01.2024 22:46, Robert Haas wrote:
> On Mon, Jan 8, 2024 at 3:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>>> 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
> Sorry, I still don't get it. I can't really follow the analysis in
> [2]. Your analysis there relies on analyzing how
> ExecBRUpdateTriggers() gets called, but it seems to me that what
> matters is what happens inside that function, specifically what
> GetTupleForTrigger does. And I think that function is going to either
> produce an EPQ tuple or not depending on whether table_tuple_lock
> returns TM_Ok and whether it sets tmfd.traversed, independently of how
> ExecBRUpdateTriggers() is called.
Yes, GetTupleForTrigger() returns an EPQ tuple in our case (because of
concurrent update).
> Also, even if you're right that epqslot_clean always ends up equal to
> newslot, my question was about how oldslot and newslot end up sharing
> the same buffer without holding two pins on it, and I don't quite see
> what epqslot_clean has to do with that. Apologies if I'm being dense
> here; this code seems dramatically under-commented to me. But FWICT
> the design here is that a slot only holds a pin until somebody copies
> it or materializes it. So I don't understand what conceptually could
> happen that would end up with two slots holding the same pin, unless
> it was just that you had two variables holding the same pointer value.
> But if that's what is happening, then materializing one slot would
> also materialize the other.
As far as I can see, oldslot and newslot hold different pointer values;
e. g., with debug logging added (see attached (please forgive me dirty
hacks inside)), I get:
LOG: ExecBRUpdateTriggers after ExecGetUpdateNewTuple() 1; oldslot: 0x758e3c8, slot->tts_tupleDescriptor: 0x7362fc8,
slot->buffer: 1810, slot->buffer->refcount: 1, slot->tts_values: 0x758e438, i: 1, slot->tts_values[ii]: 0x9982c74
STATEMENT: UPDATE bruttest SET cnt = cnt + 1;
LOG: ExecBRUpdateTriggers after ExecGetUpdateNewTuple() 1; newslot: 0x747fb48, slot->tts_tupleDescriptor: 0x7362fc8,
slot->buffer: 0, slot->buffer->refcount: 0, slot->tts_values: 0x747fbb8, i: 1, slot->tts_values[ii]: 0x9982c74
STATEMENT: UPDATE bruttest SET cnt = cnt + 1;
==00:00:00:05.052 3934530== Invalid read of size 1
==00:00:00:05.052 3934530== at 0x1EECAC: heap_compute_data_size (heaptuple.c:244)
...
==00:00:00:05.052 3934530== Address 0x9982c74 is in a rw- anonymous segment
==00:00:00:05.052 3934530==
(when executing `make check` for the isolation test [1] under Valgrind)
But as we can see, both slots use the same buffer (have pointers to the
same attribute values) as a result of the following operations:
EEOP_SCAN_FETCHSOME:
slot_getsomeattrs(scanslot, op->d.fetch.last_var);
...
EEOP_ASSIGN_SCAN_VAR:
resultslot->tts_values[resultnum] = scanslot->tts_values[attnum]
(More details upthread in [2])
In the meantime, newslot doesn't hold a pin at all.
[1] https://www.postgresql.org/message-id/950f4f1a-fb71-3e45-bb65-6a57da9f9b9e%40gmail.com
[2] https://www.postgresql.org/message-id/9f23591c-610a-60d4-2b29-26a860f7c3a8%40gmail.com
Best regards,
Alexander
Attachments:dirty-debugging-17798.patch (text/x-patch, 2.8K) downloaddiff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.cindex 6873c3b4d9..4360cdc1fa 100644--- a/src/backend/commands/trigger.c+++ b/src/backend/commands/trigger.c@@ -2936,6 +2936,8 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
false);
}
+int32 GetPrivateRefCount(Buffer buffer);+
bool
ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
ResultRelInfo *relinfo,
@@ -2990,6 +2992,32 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
epqslot_clean = ExecGetUpdateNewTuple(relinfo, epqslot_candidate,
oldslot);
+{+TupleTableSlot *slot = oldslot;++for (int ii = 1; ii < slot->tts_tupleDescriptor->natts; ii++)+{+if (slot->tts_isnull[ii]) continue;+elog(LOG, "ExecBRUpdateTriggers after ExecGetUpdateNewTuple() 1; oldslot: %p, slot->tts_tupleDescriptor: %p, slot->buffer: %d, slot->buffer->refcount: %d, slot->tts_values: %p, i: %d, slot->tts_values[ii]: %p",+slot, slot->tts_tupleDescriptor, ((BufferHeapTupleTableSlot *) slot)->buffer,+((BufferHeapTupleTableSlot *) slot)->buffer ? GetPrivateRefCount(((BufferHeapTupleTableSlot *) slot)->buffer) : 0,+slot->tts_values, ii, DatumGetPointer(slot->tts_values[ii]));+}+}++{+TupleTableSlot *slot = newslot;++for (int ii = 1; ii < slot->tts_tupleDescriptor->natts; ii++)+{+ if (slot->tts_isnull[ii]) continue;+elog(LOG, "ExecBRUpdateTriggers after ExecGetUpdateNewTuple() 1; newslot: %p, slot->tts_tupleDescriptor: %p, slot->buffer: %d, slot->buffer->refcount: %d, slot->tts_values: %p, i: %d, slot->tts_values[ii]: %p",+slot, slot->tts_tupleDescriptor, ((BufferHeapTupleTableSlot *) slot)->buffer,+((BufferHeapTupleTableSlot *) slot)->buffer ? GetPrivateRefCount(((BufferHeapTupleTableSlot *) slot)->buffer) : 0,+slot->tts_values, ii, DatumGetPointer(slot->tts_values[ii]));+}+}+
if (newslot != epqslot_clean)
ExecCopySlot(newslot, epqslot_clean);
}
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.cindex 7d601bef6d..1f8cd894c1 100644--- a/src/backend/storage/buffer/bufmgr.c+++ b/src/backend/storage/buffer/bufmgr.c@@ -203,7 +203,7 @@ static PrivateRefCountEntry *ReservedRefCountEntry = NULL;
static void ReservePrivateRefCountEntry(void);
static PrivateRefCountEntry *NewPrivateRefCountEntry(Buffer buffer);
static PrivateRefCountEntry *GetPrivateRefCountEntry(Buffer buffer, bool do_move);
-static inline int32 GetPrivateRefCount(Buffer buffer);+inline int32 GetPrivateRefCount(Buffer buffer);
static void ForgetPrivateRefCountEntry(PrivateRefCountEntry *ref);
/* ResourceOwner callbacks to hold in-progress I/Os and buffer pins */
@@ -401,7 +401,7 @@ GetPrivateRefCountEntry(Buffer buffer, bool do_move)
*
* Only works for shared memory buffers!
*/
-static inline int32+int32
GetPrivateRefCount(Buffer buffer)
{
PrivateRefCountEntry *ref;
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: exclusion@gmail.com, robertmhaas@gmail.com, tgl@sss.pgh.pa.us, guofenglinux@gmail.com, andres@anarazel.de, pg@bowt.ie
Subject: Re: BUG #17798: Incorrect memory access occurs when using BEFORE ROW UPDATE trigger
In-Reply-To: <74335d44-819d-60e0-859a-efe74d9c84c2@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox