Content-Type: multipart/mixed; boundary="------------7wJpad57PwtLiEycvVu9zXVw"
Message-ID: <b77078c0-090c-0f0e-6f13-3bb7daca93ee@gmail.com>
Date: Sat, 2 Sep 2023 16:00:00 +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 #17893: Assert failed in heap_update()/_delete() when FK
 modiified by RI trigger in non-read-committed xact
Content-Language: en-US
To: pgsql-bugs@lists.postgresql.org
References: <17893-35847009eec517b5@postgresql.org>
From: Alexander Lakhin <exclusion@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
In-Reply-To: <17893-35847009eec517b5@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/b77078c0-090c-0f0e-6f13-3bb7daca93ee%40gmail.com>
Precedence: bulk

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

12.04.2023 15:00, PG Bug reporting form wrote:
> The following bug has been logged on the website:
>
> Bug reference:      17893
>
> The following script:
> ...
>
> triggers an assertion failure:

I've come to this assertion failure again, this time from another side, so I
have one reason more to move forward to the issue fix.

First, I've found that two instances of "additional check for
transaction-snapshot mode RI updates" in head_update() and heap_delete()
are not covered by the existing regression tests at all, so I would like to
propose an addition for the fk-snapshot test (see attached). It increases
coverage and causes the assertion failures in both functions.

Second, it looks like Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID))
was added to make sure that tmfd->xmax = HeapTupleHeaderGetUpdateXid(tp.t_data);
(which ends up with just returning t_xmax) returns valid xmax. Really, in the
problematic case we get zero in tmfd->xmax. But ExecDelete(), ExecUpdate() do:
...
         switch (result)
...
             case TM_Updated:
...
                     if (IsolationUsesXactSnapshot())
                         ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
                                  errmsg("could not serialize access due to concurrent update")));

before any processing of tmfd. So it seems like the invalid xmax is not
an issue in this case.
Thus, maybe
         Assert((!(tp.t_data->t_infomask & HEAP_XMAX_INVALID))
should be changed to
         Assert(!(tp.t_data->t_infomask & HEAP_XMAX_INVALID) ||
                (result == TM_Updated && crosscheck != InvalidSnapshot));

Third, with this change applied I immediately got a failure of the next
assert in heap_delete():
             Assert(!ItemPointerEquals(&oldtup.t_self, &oldtup.t_data->t_ctid));
which was introduced by 5db6df0c0.

(By the way, I've noticed a typo brought by that commit:
t_xmax, and, if possible, and, if possible, t_cmax
)

It's not clear to me yet, which anomalous condition that Assert should
detect, specifically in heap_update(), inside this branch:
         /* Perform additional check for transaction-snapshot mode RI updates */
         if (!HeapTupleSatisfiesVisibility(&oldtup, crosscheck, buffer))
         {
             result = TM_Updated;
             Assert(!ItemPointerEquals(&oldtup.t_self, &oldtup.t_data->t_ctid));
         }

Andres, could you shed some light on this, please?

Best regards,
Alexander
--------------7wJpad57PwtLiEycvVu9zXVw
Content-Type: text/x-patch; charset=UTF-8; name="fk-snapshot.patch"
Content-Disposition: attachment; filename="fk-snapshot.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy90ZXN0L2lzb2xhdGlvbi9leHBlY3RlZC9may1zbmFwc2hvdC5v
dXQgYi9zcmMvdGVzdC9pc29sYXRpb24vZXhwZWN0ZWQvZmstc25hcHNob3Qub3V0CmluZGV4
IDVmYWY4MGQ2Y2UuLmJkZDI2YmFjNmMgMTAwNjQ0Ci0tLSBhL3NyYy90ZXN0L2lzb2xhdGlv
bi9leHBlY3RlZC9may1zbmFwc2hvdC5vdXQKKysrIGIvc3JjL3Rlc3QvaXNvbGF0aW9uL2V4
cGVjdGVkL2ZrLXNuYXBzaG90Lm91dApAQCAtMTIyLDMgKzEyMiwyNSBAQCBhCiAxCiAoMSBy
b3cpCiAKKworc3RhcnRpbmcgcGVybXV0YXRpb246IHMyaXAyIHMxYnJyIHMxaWZwMiBzMmJy
ciBzMmRwMiBzMWMgczJjCitzdGVwIHMyaXAyOiBJTlNFUlQgSU5UTyBwa19ub3BhcnRlZCBW
QUxVRVMgKDIpOworc3RlcCBzMWJycjogQkVHSU4gSVNPTEFUSU9OIExFVkVMIFJFUEVBVEFC
TEUgUkVBRDsKK3N0ZXAgczFpZnAyOiBJTlNFUlQgSU5UTyBma19wYXJ0ZWRfcGsgVkFMVUVT
ICgyKTsKK3N0ZXAgczJicnI6IEJFR0lOIElTT0xBVElPTiBMRVZFTCBSRVBFQVRBQkxFIFJF
QUQ7CitzdGVwIHMyZHAyOiBERUxFVEUgRlJPTSBwa19ub3BhcnRlZCBXSEVSRSBhID0gMjsg
PHdhaXRpbmcgLi4uPgorc3RlcCBzMWM6IENPTU1JVDsKK3N0ZXAgczJkcDI6IDwuLi4gY29t
cGxldGVkPgorRVJST1I6ICBjb3VsZCBub3Qgc2VyaWFsaXplIGFjY2VzcyBkdWUgdG8gY29u
Y3VycmVudCB1cGRhdGUKK3N0ZXAgczJjOiBDT01NSVQ7CisKK3N0YXJ0aW5nIHBlcm11dGF0
aW9uOiBzMmlwMiBzMWJyciBzMWlmbjIgczJicnIgczJkcDIgczFjIHMyYworc3RlcCBzMmlw
MjogSU5TRVJUIElOVE8gcGtfbm9wYXJ0ZWQgVkFMVUVTICgyKTsKK3N0ZXAgczFicnI6IEJF
R0lOIElTT0xBVElPTiBMRVZFTCBSRVBFQVRBQkxFIFJFQUQ7CitzdGVwIHMxaWZuMjogSU5T
RVJUIElOVE8gZmtfbm9wYXJ0ZWRfc24gVkFMVUVTICgyKTsKK3N0ZXAgczJicnI6IEJFR0lO
IElTT0xBVElPTiBMRVZFTCBSRVBFQVRBQkxFIFJFQUQ7CitzdGVwIHMyZHAyOiBERUxFVEUg
RlJPTSBwa19ub3BhcnRlZCBXSEVSRSBhID0gMjsgPHdhaXRpbmcgLi4uPgorc3RlcCBzMWM6
IENPTU1JVDsKK3N0ZXAgczJkcDI6IDwuLi4gY29tcGxldGVkPgorRVJST1I6ICBjb3VsZCBu
b3Qgc2VyaWFsaXplIGFjY2VzcyBkdWUgdG8gY29uY3VycmVudCB1cGRhdGUKK3N0ZXAgczJj
OiBDT01NSVQ7CmRpZmYgLS1naXQgYS9zcmMvdGVzdC9pc29sYXRpb24vc3BlY3MvZmstc25h
cHNob3Quc3BlYyBiL3NyYy90ZXN0L2lzb2xhdGlvbi9zcGVjcy9may1zbmFwc2hvdC5zcGVj
CmluZGV4IDM3ODUwN2ZiYzMuLjlmYWQ1N2U3NjggMTAwNjQ0Ci0tLSBhL3NyYy90ZXN0L2lz
b2xhdGlvbi9zcGVjcy9may1zbmFwc2hvdC5zcGVjCisrKyBiL3NyYy90ZXN0L2lzb2xhdGlv
bi9zcGVjcy9may1zbmFwc2hvdC5zcGVjCkBAIC0xMyw2ICsxMywxMSBAQCBzZXR1cAogICBD
UkVBVEUgVEFCTEUgZmtfbm9wYXJ0ZWQgKAogCWEJCQlpbnQJCVJFRkVSRU5DRVMgZmtfcGFy
dGVkX3BrIE9OIERFTEVURSBOTyBBQ1RJT04gSU5JVElBTExZIERFRkVSUkVECiAgICk7CisK
KyAgQ1JFQVRFIFRBQkxFIGZrX25vcGFydGVkX3NuICgKKwlhCQkJaW50CQlSRUZFUkVOQ0VT
IHBrX25vcGFydGVkIE9OIERFTEVURSBTRVQgTlVMTAorICApOworCiAgIElOU0VSVCBJTlRP
IHBrX25vcGFydGVkIFZBTFVFUyAoMSk7CiAgIElOU0VSVCBJTlRPIGZrX3BhcnRlZF9wayBW
QUxVRVMgKDEpOwogICBJTlNFUlQgSU5UTyBma19ub3BhcnRlZCBWQUxVRVMgKDEpOwpAQCAt
MjAsNyArMjUsNyBAQCBzZXR1cAogCiB0ZWFyZG93bgogewotICBEUk9QIFRBQkxFIHBrX25v
cGFydGVkLCBma19wYXJ0ZWRfcGssIGZrX25vcGFydGVkOworICBEUk9QIFRBQkxFIHBrX25v
cGFydGVkLCBma19wYXJ0ZWRfcGssIGZrX25vcGFydGVkLCBma19ub3BhcnRlZF9zbjsKIH0K
IAogc2Vzc2lvbiBzMQpAQCAtMjgsNiArMzMsNyBAQCBzdGVwIHMxYnJyCXsgQkVHSU4gSVNP
TEFUSU9OIExFVkVMIFJFUEVBVEFCTEUgUkVBRDsgfQogc3RlcCBzMWJyYwl7IEJFR0lOIElT
T0xBVElPTiBMRVZFTCBSRUFEIENPTU1JVFRFRDsgfQogc3RlcCBzMWlmcDIJeyBJTlNFUlQg
SU5UTyBma19wYXJ0ZWRfcGsgVkFMVUVTICgyKTsgfQogc3RlcCBzMWlmcDEJeyBJTlNFUlQg
SU5UTyBma19wYXJ0ZWRfcGsgVkFMVUVTICgxKTsgfQorc3RlcCBzMWlmbjIJeyBJTlNFUlQg
SU5UTyBma19ub3BhcnRlZF9zbiBWQUxVRVMgKDIpOyB9CiBzdGVwIHMxZGZwCXsgREVMRVRF
IEZST00gZmtfcGFydGVkX3BrIFdIRVJFIGEgPSAxOyB9CiBzdGVwIHMxYwl7IENPTU1JVDsg
fQogc3RlcCBzMXNmcAl7IFNFTEVDVCAqIEZST00gZmtfcGFydGVkX3BrOyB9CkBAIC0zOCw2
ICs0NCw3IEBAIHNlc3Npb24gczIKIHN0ZXAgczJicnIJeyBCRUdJTiBJU09MQVRJT04gTEVW
RUwgUkVQRUFUQUJMRSBSRUFEOyB9CiBzdGVwIHMyYnJjCXsgQkVHSU4gSVNPTEFUSU9OIExF
VkVMIFJFQUQgQ09NTUlUVEVEOyB9CiBzdGVwIHMyaXAyCXsgSU5TRVJUIElOVE8gcGtfbm9w
YXJ0ZWQgVkFMVUVTICgyKTsgfQorc3RlcCBzMmRwMgl7IERFTEVURSBGUk9NIHBrX25vcGFy
dGVkIFdIRVJFIGEgPSAyOyB9CiBzdGVwIHMyaWZuMgl7IElOU0VSVCBJTlRPIGZrX25vcGFy
dGVkIFZBTFVFUyAoMik7IH0KIHN0ZXAgczJjCXsgQ09NTUlUOyB9CiBzdGVwIHMyc2ZwCXsg
U0VMRUNUICogRlJPTSBma19wYXJ0ZWRfcGs7IH0KQEAgLTU5LDMgKzY2LDExIEBAIHBlcm11
dGF0aW9uIHMxYnJjIHMyYnJjIHMyaXAyIHMxc3AgczJjIHMxc3AgczFpZnAyIHMyYnJjIHMy
c2ZwIHMxYyBzMXNmcCBzMmlmbjIKICMgdGhlIHNhbWUgbm8gbWF0dGVyIHRoZSBzbmFwc2hv
dCBtb2RlCiBwZXJtdXRhdGlvbiBzMWJyciBzMWRmcCBzMWlmcDEgczFjIHMxc2ZuCiBwZXJt
dXRhdGlvbiBzMWJyYyBzMWRmcCBzMWlmcDEgczFjIHMxc2ZuCisKKyMgdHJ5aW5nIHRvIGRl
bGV0ZSBhIHJvdyB0aHJvdWdoIERFTEVURSBDQVNDQURFLCB3aGlsc3QgdGhhdCByb3cgaXMg
ZGVsZXRlZAorIyBpbiBhIGNvbmN1cnJlbnQgdHJhbnNhY3Rpb24KK3Blcm11dGF0aW9uIHMy
aXAyIHMxYnJyIHMxaWZwMiBzMmJyciBzMmRwMiBzMWMgczJjCisKKyMgdHJ5aW5nIHRvIHVw
ZGF0ZSBhIHJvdyB0aHJvdWdoIERFTEVURSBTRVQgTlVMTCwgd2hpbHN0IHRoYXQgcm93IGlz
IGRlbGV0ZWQKKyMgaW4gYSBjb25jdXJyZW50IHRyYW5zYWN0aW9uCitwZXJtdXRhdGlvbiBz
MmlwMiBzMWJyciBzMWlmbjIgczJicnIgczJkcDIgczFjIHMyYwo=

--------------7wJpad57PwtLiEycvVu9zXVw--