MIME-Version: 1.0
References: <20211008.165055.1621145185927268721.horikyota.ntt@gmail.com>
 <CAD21AoD00wV4gt-53ze+ZB8n4bqJrdH8J_UnDHddy8S2A+a25g@mail.gmail.com>
 <CAA4eK1L_e4r4=10TTzoOiWGQ6hH4vxWAwD=0aofz04czCTV0kQ@mail.gmail.com>
 <20220523.133323.444599955292141474.horikyota.ntt@gmail.com>
 <CAA4eK1JoKV2qmp916gFk=9SX=Qo21+sN4n-yjbB2b0Q1xxOKJw@mail.gmail.com>
 <CAD21AoAn9T6mS8sMF5=tFV66Wru92s933ihRVunLPEU_+0=Xiw@mail.gmail.com>
 <CAA4eK1Lgfj2ww4V_zMxTmduB0BQpEsS-j7RT8BV-cy7Qyp1E3A@mail.gmail.com>
 <CAD21AoDNJgYkrO1PQ=k6G9MfvRjhawMN-aZVT2spMr_ymmOhCQ@mail.gmail.com>
 <CAD21AoC4x3uOw5rUcSYZkWob5s5ottGt_RPLxCEpHimFRDjrEg@mail.gmail.com>
 <CAA4eK1L_Br0wNHwY1PrnusX1H2bvWR+iUnNC=1anKqhPBtnoMg@mail.gmail.com>
 <CAD21AoBeMVfY6dvg5J8XmSHGbc3+yTA8k_GVRzwZR24zc4TfSw@mail.gmail.com>
 <CAA4eK1Jg+u=ouXbC5EVLVnQ4sUhrR1P+p0LQSjPFmMP4jNYmuw@mail.gmail.com>
 <CAD21AoCEF6FAvRRP6LKzq2-2oVvpdQbmZDwzRAdseA9AA2mE-Q@mail.gmail.com>
 <OSZPR01MB63106E9CE179D80F46CB3154FD8B9@OSZPR01MB6310.jpnprd01.prod.outlook.com>
 <CAD21AoAUx28e-1YQDA6Uhda+=jkidAy+HY8Zv5SYjtXpRcBiVg@mail.gmail.com>
 <CAA4eK1LmyPryHUP3kaWRBbz88aCs5J_8-vL2wqwi5wnSwa24tw@mail.gmail.com>
 <CAD21AoC1H8tDo=h-xsm5Und-c4FQbNCAOuRoBaMhsxNsJrM8vA@mail.gmail.com>
 <CAA4eK1JfcR96cxqb+NYmDueirpvLytfvP-UZPz-E6-U6Lv-+tw@mail.gmail.com>
 <CAD21AoCjfmuCRUpsn+nuqRuWW3gQCBdTKuYGYvYiwLVtpTQYSg@mail.gmail.com>
 <CAA4eK1LXpPnLEdTttCmMbw3bSnrn=k_58ZUDvx3zbAHLjgod2A@mail.gmail.com>
 <CAD21AoBKb_XMdOO0vCAHKieWYqHmJnEN0bGmV5j7PvOGOEkmrQ@mail.gmail.com>
 <CAA4eK1LpxNwGKntRJSND8H-DmN8XuZYoaooQmaX4+g-ePqg2=Q@mail.gmail.com>
 <CAD21AoCzuV5yhOGw8=nVkymkE0KaHkxMMy5R-HsfBQw9=h0asA@mail.gmail.com>
 <CAA4eK1LH54gTKnxhBJRy9GHjoPxp7ggA+B9Cy7VLtXToCVukhw@mail.gmail.com>
 <CAD21AoC8NLGJ-Q+u60cGPSnxXqexEFPLo-5-offsgCE_ikVxRw@mail.gmail.com>
In-Reply-To: 
 <CAD21AoC8NLGJ-Q+u60cGPSnxXqexEFPLo-5-offsgCE_ikVxRw@mail.gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Sat, 23 Jul 2022 17:02:02 +0530
Message-ID: 
 <CAA4eK1+U1b7P2mMTZHGE0GaT4vYhGsShQx_6070a6xgwj9JQAQ@mail.gmail.com>
Subject: Re: [BUG] Logical replication failure "ERROR: could not map filenode
 "base/13237/442428" to relation OID" with catalog modifying txns
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	"Drouvot, Bertrand" <bdrouvot@amazon.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, "Oh,
 Mike" <minsoo@amazon.com>
Content-Type: multipart/mixed; boundary="00000000000067492805e4774d4e"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1%2BU1b7P2mMTZHGE0GaT4vYhGsShQx_6070a6xgwj9JQAQ%40mail.gmail.com>
Precedence: bulk

--00000000000067492805e4774d4e
Content-Type: text/plain; charset="UTF-8"

On Fri, Jul 22, 2022 at 11:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Jul 20, 2022 at 5:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Jul 20, 2022 at 1:28 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> >
> > This is required if we don't want to introduce a new set of functions
> > as you proposed above. I am not sure which one is better w.r.t back
> > patching effort later but it seems to me using flag stuff would make
> > future back patches easier if we make any changes in
> > SnapBuildCommitTxn.
>
> Understood.
>
> I've implemented this idea as well for discussion. Both patches have
> the common change to remember the initial running transactions and to
> purge them when decoding xl_running_xacts records. The difference is
> how to mark the transactions as needing to be added to the snapshot.
>
> In v7-0001-Fix-catalog-lookup-with-the-wrong-snapshot-during.patch,
> when the transaction is in the initial running xact list and its
> commit record has XINFO_HAS_INVAL flag, we mark both the top
> transaction and its all subtransactions as containing catalog changes
> (which also means to create ReorderBufferTXN entries for them). These
> transactions are added to the snapshot in SnapBuildCommitTxn() since
> ReorderBufferXidHasCatalogChanges () for them returns true.
>
> In poc_mark_top_txn_has_inval.patch, when the transaction is in the
> initial running xacts list and its commit record has XINFO_HAS_INVALS
> flag, we set a new flag, say RBTXN_COMMIT_HAS_INVALS, only to the top
> transaction.
>

It seems that the patch has missed the part to check if the xid is in
the initial running xacts list?

> In SnapBuildCommitTxn(), we add all subtransactions to
> the snapshot without checking ReorderBufferXidHasCatalogChanges() for
> subtransactions if its top transaction has the RBTXN_COMMIT_HAS_INVALS
> flag.
>
> A difference between the two ideas is the scope of changes: the former
> changes only snapbuild.c but the latter changes both snapbuild.c and
> reorderbuffer.c. Moreover, while the former uses the existing flag,
> the latter adds a new flag to the reorder buffer for dealing with only
> this case. I think the former idea is simpler in terms of that. But,
> an advantage of the latter idea is that the latter idea can save to
> create ReorderBufferTXN entries for subtransactions.
>
> Overall I prefer the former for now but I'd like to hear what others think.
>

I agree that the latter idea can have better performance in extremely
special scenarios but introducing a new flag for the same sounds a bit
ugly to me. So, I would also prefer to go with the former idea,
however, I would also like to hear what Horiguchi-San and others have
to say.

Few comments on v7-0001-Fix-catalog-lookup-with-the-wrong-snapshot-during:
1.
+void
+SnapBuildInitialXactSetCatalogChanges(SnapBuild *builder, TransactionId xid,
+   int subxcnt, TransactionId *subxacts,
+   XLogRecPtr lsn)
+{

I think it is better to name this function as
SnapBuildXIDSetCatalogChanges as we use this to mark a particular
transaction as having catalog changes.

2. Changed/added a few comments in the attached.

-- 
With Regards,
Amit Kapila.

--00000000000067492805e4774d4e
Content-Type: application/octet-stream; name="v7-0001-diff-amit.patch"
Content-Disposition: attachment; filename="v7-0001-diff-amit.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_l5xt9zhv0>
X-Attachment-Id: f_l5xt9zhv0

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvZGVjb2RlLmMgYi9z
cmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2FsL2RlY29kZS5jCmluZGV4IDc4YjljYTQ4Yzcu
LjA1YjljNDFlODggMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwv
ZGVjb2RlLmMKKysrIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9kZWNvZGUuYwpA
QCAtNjI5LDExICs2MjksMTMgQEAgRGVjb2RlQ29tbWl0KExvZ2ljYWxEZWNvZGluZ0NvbnRleHQg
KmN0eCwgWExvZ1JlY29yZEJ1ZmZlciAqYnVmLAogCiAJLyoKIAkgKiBJZiB0aGUgQ09NTUlUIHJl
Y29yZCBoYXMgaW52YWxpZGF0aW9uIG1lc3NhZ2VzLCBpdCBjb3VsZCBoYXZlIGNhdGFsb2cKLQkg
KiBjaGFuZ2VzLiBXZSBjaGVjayBpZiBpdCdzIGluIHRoZSBsaXN0IG9mIHRoZSBpbml0aWFsIHJ1
bm5pbmcgdHJhbnNhY3Rpb25zCi0JICogYW5kIHRoZW4gbWFyayBpdCBhcyBjb250YWluaW5nIGNh
dGFsb2cgY2hhbmdlLgorCSAqIGNoYW5nZXMuIEl0IGlzIHBvc3NpYmxlIHRoYXQgd2UgZGlkbid0
IG1hcmsgdGhpcyB0cmFuc2FjdGlvbiBhcworCSAqIGNvbnRhaW5pbmcgY2F0YWxvZyBjaGFuZ2Vz
IHdoZW4gdGhlIGRlY29kaW5nIHN0YXJ0cyBmcm9tIGEgY29tbWl0IHJlY29yZAorCSAqIHdpdGhv
dXQgZGVjb2RpbmcgdGhlIHRyYW5zYWN0aW9uJ3Mgb3RoZXIgY2hhbmdlcy4gU28sIHdlIGVuc3Vy
ZSB0byBtYXJrCisJICogc3VjaCB0cmFuc2FjdGlvbnMgYXMgY29udGFpbmluZyBjYXRhbG9nIGNo
YW5nZS4KIAkgKgogCSAqIFRoaXMgbXVzdCBiZSBkb25lIGJlZm9yZSBTbmFwQnVpbGRDb21taXRU
eG4oKSBzbyB0aGF0IHdlIGNhbiBpbmNsdWRlCi0JICogY2F0YWxvZyBjaGFuZ2UgdHJhbnNhY3Rp
b25zIHRvIHRoZSBoaXN0b3JpYyBzbmFwc2hvdC4KKwkgKiB0aGVzZSB0cmFuc2FjdGlvbnMgaW4g
dGhlIGhpc3RvcmljIHNuYXBzaG90LgogCSAqLwogCWlmIChwYXJzZWQtPnhpbmZvICYgWEFDVF9Y
SU5GT19IQVNfSU5WQUxTKQogCQlTbmFwQnVpbGRJbml0aWFsWGFjdFNldENhdGFsb2dDaGFuZ2Vz
KGN0eC0+c25hcHNob3RfYnVpbGRlciwKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0
aW9uL2xvZ2ljYWwvc25hcGJ1aWxkLmMgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2Fs
L3NuYXBidWlsZC5jCmluZGV4IDI1ODU5MjJkYWQuLjNjZGVlMDBiOWIgMTAwNjQ0Ci0tLSBhL3Ny
Yy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvc25hcGJ1aWxkLmMKKysrIGIvc3JjL2JhY2tl
bmQvcmVwbGljYXRpb24vbG9naWNhbC9zbmFwYnVpbGQuYwpAQCAtMjUzLDEwICsyNTMsMTAgQEAg
c3RhdGljIGJvb2wgRXhwb3J0SW5Qcm9ncmVzcyA9IGZhbHNlOwogLyoKICAqIEFycmF5IG9mIHRy
YW5zYWN0aW9ucyBhbmQgc3VidHJhbnNhY3Rpb25zIHRoYXQgd2VyZSBydW5uaW5nIHdoZW4KICAq
IHRoZSB4bF9ydW5uaW5nX3hhY3RzIHJlY29yZCB0aGF0IHdlIGRlY29kZWQgZmlyc3Qgd2FzIHdy
aXR0ZW4uCi0gKiBUaGUgYXJyYXkgaXMgc29ydGVkIGluIHhpZENvbXBhcmF0b3Igb3JkZXIuIFhp
ZHMgYXJlIG9ubHkgcmVtb3ZlZAotICogZnJvbSB0aGUgYXJyYXkgd2hlbiBkZWNvZGluZyB4bF9y
dW5uaW5nX3hhY3RzIHJlY29yZCwgYW5kIHRoZW4KLSAqIHRoZSBhcnJheSBldmVudHVhbGx5IGJl
Y29tZXMgYW4gZW1wdHkuIFRoaXMgYXJyYXkgaXMgYWxsb2NhdGVkIGluCi0gKiBidWlsZGVyLT5j
b250ZXh0IHNvIGl0cyBsaWZldGltZSBpcyB0aGUgc2FtZSBhcyB0aGUgc25hcHNob3QgYnVpbGRl
ci4KKyAqIFRoZSBhcnJheSBpcyBzb3J0ZWQgaW4geGlkQ29tcGFyYXRvciBvcmRlci4gV2UgcmVt
b3ZlIFhpZHMgZnJvbQorICogdGhpcyBhcnJheSB3aGVuIHRoZXkgYmVjb21lIG9sZCBlbm91Z2gg
dG8gbWF0dGVyLiBUaGlzIGFycmF5IGlzCisgKiBhbGxvY2F0ZWQgaW4gYnVpbGRlci0+Y29udGV4
dCBzbyBpdHMgbGlmZXRpbWUgaXMgdGhlIHNhbWUgYXMgdGhlCisgKiBzbmFwc2hvdCBidWlsZGVy
LgogICoKICAqIFdlIHJlbHkgb24gSEVBUDJfTkVXX0NJRCByZWNvcmRzIGFuZCBYQUNUX0lOVkFM
SURBVElPTlMgdG8ga25vdwogICogaWYgdGhlIHRyYW5zYWN0aW9uIGhhcyBjaGFuZ2VkIHRoZSBj
YXRhbG9nLCBhbmQgdGhhdCBpbmZvcm1hdGlvbgpAQCAtMjA5NSw4ICsyMDk1LDkgQEAgQ2hlY2tQ
b2ludFNuYXBCdWlsZCh2b2lkKQogfQogCiAvKgotICogSWYgdGhlIGdpdmVuIHhpZCBpcyBpbiB0
aGUgbGlzdCBvZiB0aGUgaW5pdGlhbCBydW5uaW5nIHhhY3RzLCB3ZSBtYXJrIGJvdGggaXQKLSAq
IGFuZCBpdHMgc3VidHJhbnNhY3Rpb25zIGFzIGNvbnRhaW5pbmcgY2F0YWxvZyBjaGFuZ2VzIGlm
IG5vdCB5ZXQuCisgKiBJZiB0aGUgZ2l2ZW4geGlkIGlzIGluIHRoZSBsaXN0IG9mIHRoZSBpbml0
aWFsIHJ1bm5pbmcgeGFjdHMsIHdlIG1hcmsgdGhlCisgKiB0cmFuc2FjdGlvbiBhbmQgaXRzIHN1
YnRyYW5zYWN0aW9ucyBhcyBjb250YWluaW5nIGNhdGFsb2cgY2hhbmdlcy4gU2VlCisgKiBjb21t
ZW50cwogICovCiB2b2lkCiBTbmFwQnVpbGRJbml0aWFsWGFjdFNldENhdGFsb2dDaGFuZ2VzKFNu
YXBCdWlsZCAqYnVpbGRlciwgVHJhbnNhY3Rpb25JZCB4aWQsCg==
--00000000000067492805e4774d4e--