Content-Type: multipart/mixed; boundary="------------Olp0uOgAvI5OVRrc4cZeyPlC"
Message-ID: <c5624ffb-5c15-9b23-c4de-39e8bc384622@enterprisedb.com>
Date: Sun, 4 Sep 2022 19:40:14 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: TRAP: FailedAssertion("prev_first_lsn < cur_txn->first_lsn",
 File: "reorderbuffer.c", Line: 927, PID: 568639)
Content-Language: en-US
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <a89b46b6-0239-2fd5-71a9-b19b1f7a7145@enterprisedb.com>
 <CAA4eK1+NNjJMnvRtee_e7pxSv2vaK2=iT-dbJPG1kNN+ELm7Nw@mail.gmail.com>
 <b3d74bba-743f-27fe-d098-841c8eb64562@enterprisedb.com>
 <7950f8c6-3fa9-ad32-b040-7024d8eb1bea@enterprisedb.com>
In-Reply-To: <7950f8c6-3fa9-ad32-b040-7024d8eb1bea@enterprisedb.com>
Archived-At: 
 <https://www.postgresql.org/message-id/c5624ffb-5c15-9b23-c4de-39e8bc384622%40enterprisedb.com>
Precedence: bulk

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

On 9/4/22 16:08, Tomas Vondra wrote:
> ...
> 
> so in fact we *know* 849 is a subxact of 848, but we don't call
> ReorderBufferAssignChild in this case. In fact we can't even do the
> assignment easily in this case, because we create the subxact first, so
> that the crash happens right when we attempt to create the toplevel one,
> and we never even get a chance to do the assignment:
> 
> 1) process the NEW_CID record, logged for 849 (subxact)
> 2) process CIDs in the WAL record, which has topleve_xid 848
> 
> 
> So IMHO we need to figure out what to do for WAL records that create
> both the toplevel and subxact - either we need to skip them, or rethink
> how we create the ReorderBufferTXN structs.
> 

This fixes the crash for me, by adding a ReorderBufferAssignChild call
to SnapBuildProcessNewCid, and tweaking ReorderBufferAssignChild to
ensure we don't try to create the top xact before updating the subxact
and removing it from the toplevel_by_lsn list.

Essentially, what's happening is this:

1) We read the NEW_CID record, which is logged with XID 849, i.e. the
subxact. But we don't know it's a subxact, so we create it as a
top-level xact with the LSN.

2) We start processing contents of the NEW_CID, which however has info
that 849 is subxact of 848, calls ReorderBufferAddNewTupleCids which
promptly does ReorderBufferTXNByXid() with the top-level XID, which
creates it with the same LSN, and crashes because of the assert.

I'm not sure what's the right/proper way to fix this ...

The problem is ReorderBufferAssignChild was coded in a way that did not
expect the subxact to be created first (as a top-level xact). And
indeed, if I add Assert(false) to the (!new_sub) branch that converts
top-level xact to subxact, check-world still passes. So we never test
this case, but the NEW_CID breaks this assumption and creates them in
the opposite order (i.e. subxact first).

So the patch "fixes" this by

(a) tweaking ReorderBufferAssignChild to first remove the subxact from
the list of top-level transactions

(b) call ReorderBufferAssignChild when processing NEW_CID

However, I wonder whether we even have to process these records? If the
restart_lsn is half-way through the xact, so can we even decode it?
Maybe we can just skip all of this, somehow? We'd still need to remember
849 is a subxact of 848, at least, so that we know to skip it too.


Thread [1] suggested to relax the assert to allow the same LSN, provided
it's xact and it's subxact. That goes directly against the expectation
the toplevel_by_lsn list contains no known subxacts, and I don't think
we should be relaxing that. After all, just tweaking the LSN does not
really fix the issue, because not remembering it's xact+subxact is part
of the issue. In principle, I think the issue is exactly the opposite,
i.e. that we don't realize 849 is a subxact, and leave it in the list.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--------------Olp0uOgAvI5OVRrc4cZeyPlC
Content-Type: text/x-patch; charset=UTF-8; name="crash-wip-fix.patch"
Content-Disposition: attachment; filename="crash-wip-fix.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvcmVvcmRlcmJ1
ZmZlci5jIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9yZW9yZGVyYnVmZmVy
LmMKaW5kZXggODljZjlmOTM4OWMuLjM0Y2E1MmQ2Y2MwIDEwMDY0NAotLS0gYS9zcmMvYmFj
a2VuZC9yZXBsaWNhdGlvbi9sb2dpY2FsL3Jlb3JkZXJidWZmZXIuYworKysgYi9zcmMvYmFj
a2VuZC9yZXBsaWNhdGlvbi9sb2dpY2FsL3Jlb3JkZXJidWZmZXIuYwpAQCAtMTAyOSw3ICsx
MDI5LDYgQEAgUmVvcmRlckJ1ZmZlckFzc2lnbkNoaWxkKFJlb3JkZXJCdWZmZXIgKnJiLCBU
cmFuc2FjdGlvbklkIHhpZCwKIAlib29sCQluZXdfdG9wOwogCWJvb2wJCW5ld19zdWI7CiAK
LQl0eG4gPSBSZW9yZGVyQnVmZmVyVFhOQnlYaWQocmIsIHhpZCwgdHJ1ZSwgJm5ld190b3As
IGxzbiwgdHJ1ZSk7CiAJc3VidHhuID0gUmVvcmRlckJ1ZmZlclRYTkJ5WGlkKHJiLCBzdWJ4
aWQsIHRydWUsICZuZXdfc3ViLCBsc24sIGZhbHNlKTsKIAogCWlmICghbmV3X3N1YikKQEAg
LTEwNTQsNiArMTA1Myw4IEBAIFJlb3JkZXJCdWZmZXJBc3NpZ25DaGlsZChSZW9yZGVyQnVm
ZmVyICpyYiwgVHJhbnNhY3Rpb25JZCB4aWQsCiAJc3VidHhuLT50b3BsZXZlbF94aWQgPSB4
aWQ7CiAJQXNzZXJ0KHN1YnR4bi0+bnN1YnR4bnMgPT0gMCk7CiAKKwl0eG4gPSBSZW9yZGVy
QnVmZmVyVFhOQnlYaWQocmIsIHhpZCwgdHJ1ZSwgJm5ld190b3AsIGxzbiwgdHJ1ZSk7CisK
IAkvKiBzZXQgdGhlIHJlZmVyZW5jZSB0byB0b3AtbGV2ZWwgdHJhbnNhY3Rpb24gKi8KIAlz
dWJ0eG4tPnRvcHR4biA9IHR4bjsKIApkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvcmVwbGlj
YXRpb24vbG9naWNhbC9zbmFwYnVpbGQuYyBiL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xv
Z2ljYWwvc25hcGJ1aWxkLmMKaW5kZXggYmY3MmFkNDVlYzcuLjAwNDI4ZTQ2ZDA5IDEwMDY0
NAotLS0gYS9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2FsL3NuYXBidWlsZC5jCisr
KyBiL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvc25hcGJ1aWxkLmMKQEAgLTgw
OCw2ICs4MDgsMTAgQEAgU25hcEJ1aWxkUHJvY2Vzc05ld0NpZChTbmFwQnVpbGQgKmJ1aWxk
ZXIsIFRyYW5zYWN0aW9uSWQgeGlkLAogewogCUNvbW1hbmRJZAljaWQ7CiAKKwkvKiB3ZSBo
YXZlIGEgdG9wIFhJRCwgYnV0IGl0J3Mgbm90IGtub3duIHRvIGJlIHN1YnhhY3QgKi8KKwlp
ZiAoeGlkICE9IHhscmVjLT50b3BfeGlkICYmIHhscmVjLT50b3BfeGlkICE9IEludmFsaWRU
cmFuc2FjdGlvbklkKQorCQlSZW9yZGVyQnVmZmVyQXNzaWduQ2hpbGQoYnVpbGRlci0+cmVv
cmRlciwgeGxyZWMtPnRvcF94aWQsIHhpZCwgbHNuKTsKKwogCS8qCiAJICogd2Ugb25seSBs
b2cgbmV3X2NpZCdzIGlmIGEgY2F0YWxvZyB0dXBsZSB3YXMgbW9kaWZpZWQsIHNvIG1hcmsg
dGhlCiAJICogdHJhbnNhY3Rpb24gYXMgY29udGFpbmluZyBjYXRhbG9nIG1vZGlmaWNhdGlv
bnMK

--------------Olp0uOgAvI5OVRrc4cZeyPlC--