Content-Type: multipart/mixed; boundary="------------PjSSjm38vi0rcIYCGZEnPaov"
Message-ID: <74335d44-819d-60e0-859a-efe74d9c84c2@gmail.com>
Date: Wed, 10 Jan 2024 21:00:01 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #17798: Incorrect memory access occurs when using BEFORE ROW
 UPDATE trigger
Content-Language: en-US
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-bugs@lists.postgresql.org,
 Richard Guo <guofenglinux@gmail.com>, Andres Freund <andres@anarazel.de>,
 Peter Geoghegan <pg@bowt.ie>
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>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: 
 <CA+TgmoYAsQWMqMUNNdyp8V0KVxMoz=0y+3XNCf3ST-o1yyz5Jg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/74335d44-819d-60e0-859a-efe74d9c84c2%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------PjSSjm38vi0rcIYCGZEnPaov
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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
--------------PjSSjm38vi0rcIYCGZEnPaov
Content-Type: text/x-patch; charset=UTF-8; name="dirty-debugging-17798.patch"
Content-Disposition: attachment; filename="dirty-debugging-17798.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL3RyaWdnZXIuYyBiL3NyYy9iYWNr
ZW5kL2NvbW1hbmRzL3RyaWdnZXIuYwppbmRleCA2ODczYzNiNGQ5Li40MzYwY2RjMWZhIDEw
MDY0NAotLS0gYS9zcmMvYmFja2VuZC9jb21tYW5kcy90cmlnZ2VyLmMKKysrIGIvc3JjL2Jh
Y2tlbmQvY29tbWFuZHMvdHJpZ2dlci5jCkBAIC0yOTM2LDYgKzI5MzYsOCBAQCBFeGVjQVNV
cGRhdGVUcmlnZ2VycyhFU3RhdGUgKmVzdGF0ZSwgUmVzdWx0UmVsSW5mbyAqcmVsaW5mbywK
IAkJCQkJCQkgIGZhbHNlKTsKIH0KIAoraW50MzIgR2V0UHJpdmF0ZVJlZkNvdW50KEJ1ZmZl
ciBidWZmZXIpOworCiBib29sCiBFeGVjQlJVcGRhdGVUcmlnZ2VycyhFU3RhdGUgKmVzdGF0
ZSwgRVBRU3RhdGUgKmVwcXN0YXRlLAogCQkJCQkgUmVzdWx0UmVsSW5mbyAqcmVsaW5mbywK
QEAgLTI5OTAsNiArMjk5MiwzMiBAQCBFeGVjQlJVcGRhdGVUcmlnZ2VycyhFU3RhdGUgKmVz
dGF0ZSwgRVBRU3RhdGUgKmVwcXN0YXRlLAogCQkJZXBxc2xvdF9jbGVhbiA9IEV4ZWNHZXRV
cGRhdGVOZXdUdXBsZShyZWxpbmZvLCBlcHFzbG90X2NhbmRpZGF0ZSwKIAkJCQkJCQkJCQkJ
CSAgb2xkc2xvdCk7CiAKK3sKK1R1cGxlVGFibGVTbG90ICpzbG90ID0gb2xkc2xvdDsKKwor
Zm9yIChpbnQgaWkgPSAxOyBpaSA8IHNsb3QtPnR0c190dXBsZURlc2NyaXB0b3ItPm5hdHRz
OyBpaSsrKQoreworaWYgKHNsb3QtPnR0c19pc251bGxbaWldKSBjb250aW51ZTsKK2Vsb2co
TE9HLCAiRXhlY0JSVXBkYXRlVHJpZ2dlcnMgYWZ0ZXIgRXhlY0dldFVwZGF0ZU5ld1R1cGxl
KCkgIDE7IG9sZHNsb3Q6ICVwLCBzbG90LT50dHNfdHVwbGVEZXNjcmlwdG9yOiAlcCwgc2xv
dC0+YnVmZmVyOiAlZCwgc2xvdC0+YnVmZmVyLT5yZWZjb3VudDogJWQsIHNsb3QtPnR0c192
YWx1ZXM6ICVwLCBpOiAlZCwgc2xvdC0+dHRzX3ZhbHVlc1tpaV06ICVwIiwKK3Nsb3QsIHNs
b3QtPnR0c190dXBsZURlc2NyaXB0b3IsICgoQnVmZmVySGVhcFR1cGxlVGFibGVTbG90ICop
IHNsb3QpLT5idWZmZXIsCisoKEJ1ZmZlckhlYXBUdXBsZVRhYmxlU2xvdCAqKSBzbG90KS0+
YnVmZmVyID8gR2V0UHJpdmF0ZVJlZkNvdW50KCgoQnVmZmVySGVhcFR1cGxlVGFibGVTbG90
ICopIHNsb3QpLT5idWZmZXIpIDogMCwKK3Nsb3QtPnR0c192YWx1ZXMsIGlpLCBEYXR1bUdl
dFBvaW50ZXIoc2xvdC0+dHRzX3ZhbHVlc1tpaV0pKTsKK30KK30KKworeworVHVwbGVUYWJs
ZVNsb3QgKnNsb3QgPSBuZXdzbG90OworCitmb3IgKGludCBpaSA9IDE7IGlpIDwgc2xvdC0+
dHRzX3R1cGxlRGVzY3JpcHRvci0+bmF0dHM7IGlpKyspCit7CisJaWYgKHNsb3QtPnR0c19p
c251bGxbaWldKSBjb250aW51ZTsKK2Vsb2coTE9HLCAiRXhlY0JSVXBkYXRlVHJpZ2dlcnMg
YWZ0ZXIgRXhlY0dldFVwZGF0ZU5ld1R1cGxlKCkgIDE7IG5ld3Nsb3Q6ICVwLCBzbG90LT50
dHNfdHVwbGVEZXNjcmlwdG9yOiAlcCwgc2xvdC0+YnVmZmVyOiAlZCwgc2xvdC0+YnVmZmVy
LT5yZWZjb3VudDogJWQsIHNsb3QtPnR0c192YWx1ZXM6ICVwLCBpOiAlZCwgc2xvdC0+dHRz
X3ZhbHVlc1tpaV06ICVwIiwKK3Nsb3QsIHNsb3QtPnR0c190dXBsZURlc2NyaXB0b3IsICgo
QnVmZmVySGVhcFR1cGxlVGFibGVTbG90ICopIHNsb3QpLT5idWZmZXIsCisoKEJ1ZmZlckhl
YXBUdXBsZVRhYmxlU2xvdCAqKSBzbG90KS0+YnVmZmVyID8gR2V0UHJpdmF0ZVJlZkNvdW50
KCgoQnVmZmVySGVhcFR1cGxlVGFibGVTbG90ICopIHNsb3QpLT5idWZmZXIpIDogMCwKK3Ns
b3QtPnR0c192YWx1ZXMsIGlpLCBEYXR1bUdldFBvaW50ZXIoc2xvdC0+dHRzX3ZhbHVlc1tp
aV0pKTsKK30KK30KKwogCQkJaWYgKG5ld3Nsb3QgIT0gZXBxc2xvdF9jbGVhbikKIAkJCQlF
eGVjQ29weVNsb3QobmV3c2xvdCwgZXBxc2xvdF9jbGVhbik7CiAJCX0KZGlmZiAtLWdpdCBh
L3NyYy9iYWNrZW5kL3N0b3JhZ2UvYnVmZmVyL2J1Zm1nci5jIGIvc3JjL2JhY2tlbmQvc3Rv
cmFnZS9idWZmZXIvYnVmbWdyLmMKaW5kZXggN2Q2MDFiZWY2ZC4uMWY4Y2Q4OTRjMSAxMDA2
NDQKLS0tIGEvc3JjL2JhY2tlbmQvc3RvcmFnZS9idWZmZXIvYnVmbWdyLmMKKysrIGIvc3Jj
L2JhY2tlbmQvc3RvcmFnZS9idWZmZXIvYnVmbWdyLmMKQEAgLTIwMyw3ICsyMDMsNyBAQCBz
dGF0aWMgUHJpdmF0ZVJlZkNvdW50RW50cnkgKlJlc2VydmVkUmVmQ291bnRFbnRyeSA9IE5V
TEw7CiBzdGF0aWMgdm9pZCBSZXNlcnZlUHJpdmF0ZVJlZkNvdW50RW50cnkodm9pZCk7CiBz
dGF0aWMgUHJpdmF0ZVJlZkNvdW50RW50cnkgKk5ld1ByaXZhdGVSZWZDb3VudEVudHJ5KEJ1
ZmZlciBidWZmZXIpOwogc3RhdGljIFByaXZhdGVSZWZDb3VudEVudHJ5ICpHZXRQcml2YXRl
UmVmQ291bnRFbnRyeShCdWZmZXIgYnVmZmVyLCBib29sIGRvX21vdmUpOwotc3RhdGljIGlu
bGluZSBpbnQzMiBHZXRQcml2YXRlUmVmQ291bnQoQnVmZmVyIGJ1ZmZlcik7CitpbmxpbmUg
aW50MzIgR2V0UHJpdmF0ZVJlZkNvdW50KEJ1ZmZlciBidWZmZXIpOwogc3RhdGljIHZvaWQg
Rm9yZ2V0UHJpdmF0ZVJlZkNvdW50RW50cnkoUHJpdmF0ZVJlZkNvdW50RW50cnkgKnJlZik7
CiAKIC8qIFJlc291cmNlT3duZXIgY2FsbGJhY2tzIHRvIGhvbGQgaW4tcHJvZ3Jlc3MgSS9P
cyBhbmQgYnVmZmVyIHBpbnMgKi8KQEAgLTQwMSw3ICs0MDEsNyBAQCBHZXRQcml2YXRlUmVm
Q291bnRFbnRyeShCdWZmZXIgYnVmZmVyLCBib29sIGRvX21vdmUpCiAgKgogICogT25seSB3
b3JrcyBmb3Igc2hhcmVkIG1lbW9yeSBidWZmZXJzIQogICovCi1zdGF0aWMgaW5saW5lIGlu
dDMyCitpbnQzMgogR2V0UHJpdmF0ZVJlZkNvdW50KEJ1ZmZlciBidWZmZXIpCiB7CiAJUHJp
dmF0ZVJlZkNvdW50RW50cnkgKnJlZjsK

--------------PjSSjm38vi0rcIYCGZEnPaov--