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>
 <CAA4eK1KMsU5PFHOvTD=3jHQ6aPa8N39eGwAVMVA6S0sXw1kMdw@mail.gmail.com>
 <CAD21AoD8v=pE+3AMezPrWJo=hpijNmop-XRbZ=3YG5zT5pBMnQ@mail.gmail.com>
 <CAA4eK1KuSxRhwUoJWVW--EseKcob94-hrP+JMCGvGTR+Fnx2aA@mail.gmail.com>
 <CAD21AoBHo3WQ+vaeiNzXuNw__08veBT9qYgCUD3jNLWG=4GzFw@mail.gmail.com>
 <CAA4eK1KMegGo+QwMxJcHMbYN-3QOLH6hD2thWXA2_M3MLH01Ug@mail.gmail.com>
 <CAD21AoCF=8SKOJYWF12fPEwXc5GLU2SvhfZ3QoR93Qqn+6oeSg@mail.gmail.com>
 <CAA4eK1LuFtCifCBY=uB_Orzjx-tc0O8DZqfFE_JuHptngeCcwA@mail.gmail.com>
 <CAD21AoCBPL10=X83g6J2CXQRE11kXgiyzfVY2m=hS1O7YRqiWQ@mail.gmail.com>
 <CAA4eK1JyJL7dACPQ51hQmSz5KrxghmABr_J9AZbSGzSLW3Ghig@mail.gmail.com>
 <CAD21AoCAPkPxsqbe6ZTqOe0bcVrEWHmA3ObYQtyu0Bv8hC=z_g@mail.gmail.com>
 <CAD21AoC4+GeT_HRnCFuy0L37FHUSXOPEpXYp0wbQnFxCMiuD=Q@mail.gmail.com>
 <CAD21AoA7WM=ptACtu0cg5XdQ86c5RJDg9efsdPRCxQnUqZ+omg@mail.gmail.com>
In-Reply-To: 
 <CAD21AoA7WM=ptACtu0cg5XdQ86c5RJDg9efsdPRCxQnUqZ+omg@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Tue, 12 Jul 2022 15:07:25 +0900
Message-ID: 
 <CAD21AoDFC5btYbcGOLMWimotg+N45pXJqyEOhbfsOHdz8XUKpg@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: Amit Kapila <amit.kapila16@gmail.com>
Cc: 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="000000000000b1ec1705e3957d40"
Archived-At: 
 <https://www.postgresql.org/message-id/CAD21AoDFC5btYbcGOLMWimotg%2BN45pXJqyEOhbfsOHdz8XUKpg%40mail.gmail.com>
Precedence: bulk

--000000000000b1ec1705e3957d40
Content-Type: text/plain; charset="UTF-8"

On Tue, Jul 12, 2022 at 10:28 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Jul 12, 2022 at 9:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Jul 8, 2022 at 8:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Fri, Jul 8, 2022 at 5:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Fri, Jul 8, 2022 at 12:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > >
> > > > > On Fri, Jul 8, 2022 at 3:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > > >
> > > > >
> > > > > > 1.
> > > > > > In ReorderBufferGetCatalogChangesXacts(), isn't it better to use the
> > > > > > list length of 'catchange_txns' to allocate xids array? If we can do
> > > > > > so, then we will save the need to repalloc as well.
> > > > >
> > > > > Since ReorderBufferGetcatalogChangesXacts() collects all ongoing
> > > > > catalog modifying transactions, the length of the array could be
> > > > > bigger than the one taken last time. We can start with the previous
> > > > > length but I think we cannot remove the need for repalloc.
> > > > >
> > > >
> > > > It is using the list "catchange_txns" to form xid array which
> > > > shouldn't change for the duration of
> > > > ReorderBufferGetCatalogChangesXacts(). Then the caller frees the xid
> > > > array after its use. Next time in
> > > > ReorderBufferGetCatalogChangesXacts(), the fresh allocation for xid
> > > > array happens, so not sure why repalloc would be required?
> > >
> > > Oops, I mistook catchange_txns for catchange->xcnt. You're right.
> > > Starting with the length of catchange_txns should be sufficient.
> > >
> >
> > I've attached an updated patch.
> >
> > While trying this idea, I noticed there is no API to get the length of
> > dlist, as we discussed offlist. Alternative idea was to use List
> > (T_XidList) but I'm not sure it's a great idea since deleting an xid
> > from the list is O(N), we need to implement list_delete_xid, and we
> > need to make sure allocating list node in the reorder buffer context.
> > So in the patch, I added a variable, catchange_ntxns, to keep track of
> > the length of the dlist. Please review it.
> >
>
> I'm doing benchmark tests and will share the results.
>

I've done benchmark tests to measure the overhead introduced by doing
bsearch() every time when decoding a commit record. I've simulated a
very intensified situation where we decode 1M commit records while
keeping builder->catchange.xip array but the overhead is negilible:

HEAD: 584 ms
Patched: 614 ms

I've attached the benchmark script I used. With increasing
LOG_SNAPSHOT_INTERVAL_MS to 90000, the last decoding by
pg_logicla_slot_get_changes() decodes 1M commit records while keeping
catalog modifying transactions.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

--000000000000b1ec1705e3957d40
Content-Type: application/octet-stream; name="bench.spec"
Content-Disposition: attachment; filename="bench.spec"
Content-Transfer-Encoding: base64
Content-ID: <f_l5hrsecq0>
X-Attachment-Id: f_l5hrsecq0

IyBUZXN0IHRoYXQgZGVjb2Rpbmcgb25seSB0aGUgY29tbWl0IHJlY29yZCBvZiB0aGUgdHJhbnNh
Y3Rpb24gdGhhdCBoYXZlCiMgY2F0YWxvZy1jaGFuZ2VkLgpzZXR1cAp7CiAgICBEUk9QIFRBQkxF
IElGIEVYSVNUUyB0YmwxOwogICAgQ1JFQVRFIFRBQkxFIHRibDEgKHZhbDEgaW50ZWdlciwgdmFs
MiBpbnRlZ2VyKTsKICAgIGNyZWF0ZSBwcm9jZWR1cmUgcnVuX3R4bnMoY250IGludCkgYXMgJCQK
ICAgIGRlY2xhcmUKICAgICAgaSBpbnQ7CiAgICBCRUdJTgogICAgZm9yIGkgaW4gMS4uY250IGxv
b3AKICAgICAgICBleGVjdXRlICdpbnNlcnQgaW50byB0YmwxIHZhbHVlcyAoMSknOwoJY29tbWl0
OwogICAgIGVuZCBsb29wOwogICAgIGVuZDsKICAgICAkJAogICAgIGxhbmd1YWdlIHBscGdzcWw7
Cn0KCnRlYXJkb3duCnsKICAgIERST1AgVEFCTEUgdGJsMTsKICAgIFNFTEVDVCAnc3RvcCcgRlJP
TSBwZ19kcm9wX3JlcGxpY2F0aW9uX3Nsb3QoJ2lzb2xhdGlvbl9zbG90Jyk7Cn0KCnNlc3Npb24g
InMwIgpzZXR1cCB7IFNFVCBzeW5jaHJvbm91c19jb21taXQ9b247IH0Kc3RlcCAiczBfaW5pdCIg
eyBTRUxFQ1QgJ2luaXQnIEZST00gcGdfY3JlYXRlX2xvZ2ljYWxfcmVwbGljYXRpb25fc2xvdCgn
aXNvbGF0aW9uX3Nsb3QnLCAndGVzdF9kZWNvZGluZycpOyB9CnN0ZXAgInMwX2JlZ2luIiB7IEJF
R0lOOyB9CnN0ZXAgInMwX3NhdmVwb2ludCIgeyBTQVZFUE9JTlQgc3AxOyB9CnN0ZXAgInMwX2Fs
dGVyIiB7IEFMVEVSIFRBQkxFIHRibDEgU0VUIChhdXRvdmFjdXVtX2VuYWJsZWQgPSBvZmYpOyB9
CnN0ZXAgInMwX2luc2VydCIgeyBJTlNFUlQgSU5UTyB0YmwxIHNlbGVjdCBjLCBjIGZyb20gZ2Vu
ZXJhdGVfc2VyaWVzKDEsIDEpIGM7IH0Kc3RlcCAiczBfY29tbWl0IiB7IENPTU1JVDsgfQoKc2Vz
c2lvbiAiczEiCnNldHVwIHsgU0VUIHN5bmNocm9ub3VzX2NvbW1pdD1vbjsgfQpzdGVwICJzMV9j
aGVja3BvaW50IiB7IENIRUNLUE9JTlQ7IH0Kc3RlcCAiczFfZ2V0X2NoYW5nZXMiIHsgU0VMRUNU
IGNvdW50KCopIEZST00gcGdfbG9naWNhbF9zbG90X2dldF9jaGFuZ2VzKCdpc29sYXRpb25fc2xv
dCcsIE5VTEwsIE5VTEwsICdza2lwLWVtcHR5LXhhY3RzJywgJzEnLCAnaW5jbHVkZS14aWRzJywg
JzAnKTsgfQpzdGVwICJ0aW1lIiB7IHNlbGVjdCBub3coKTsgfQoKc2Vzc2lvbiAiczIiCnN0ZXAg
InMyX3J1bl90eG4iIHsgY2FsbCBydW5fdHhucygxMDAwMDAwKTsgfQoKIyBGb3IgdGhlIHRyYW5z
YWN0aW9uIHRoYXQgVFJVTkNBVEVkIHRoZSB0YWJsZSB0YmwxLCB0aGUgbGFzdCBkZWNvZGluZyBk
ZWNvZGVzCiMgb25seSBpdHMgQ09NTUlUIHJlY29yZCwgYmVjYXVzZSBpdCBzdGFydHMgZnJvbSB0
aGUgUlVOTklOR19YQUNUIHJlY29yZCBlbWl0dGVkCiMgZHVyaW5nIHRoZSBmaXJzdCBjaGVja3Bv
aW50IGV4ZWN1dGlvbi4gIFRoaXMgdHJhbnNhY3Rpb24gbXVzdCBiZSBtYXJrZWQgYXMKIyBjb250
YWluaW5nIGNhdGFsb2cgY2hhbmdlcyB3aGlsZSBkZWNvZGluZyB0aGUgQ09NTUlUIHJlY29yZCBh
bmQgdGhlIGRlY29kaW5nCiMgb2YgdGhlIElOU0VSVCByZWNvcmQgbXVzdCByZWFkIHRoZSBwZ19j
bGFzcyB3aXRoIHRoZSBjb3JyZWN0IGhpc3RvcmljIHNuYXBzaG90LgojCiMgTm90ZSB0aGF0IGlu
IGEgY2FzZSB3aGVyZSBiZ3dyaXRlciB3cm90ZSB0aGUgWEFDVF9SVU5OSU5HIHJlY29yZCBiZXR3
ZWVuICJzMF9jb21taXQiCiMgYW5kICJzMF9iZWdpbiIsIHRoaXMgZG9lc24ndCBoYXBwZW4gYXMg
dGhlIGRlY29kaW5nIHN0YXJ0cyBmcm9tIHRoZSBSVU5OSU5HX1hBQ1QKIyByZWNvcmQgd3JpdHRl
biBieSBiZ3dyaXRlci4gIE9uZSBtaWdodCB0aGluayB3ZSBjYW4gZWl0aGVyIHN0b3AgdGhlIGJn
d3JpdGVyIG9yCiMgaW5jcmVhc2UgTE9HX1NOQVBTSE9UX0lOVEVSVkFMX01TIGJ1dCBpdCdzIG5v
dCBwcmFjdGljYWwgdmlhIHRlc3RzLgojcGVybXV0YXRpb24gInMwX2luaXQiICJzMF9iZWdpbiIg
InMwX3NhdmVwb2ludCIgInMwX3RydW5jYXRlIiAiczFfY2hlY2twb2ludCIgInMxX2dldF9jaGFu
Z2VzIiAiczBfY29tbWl0IiAiczBfYmVnaW4iICJzMF9pbnNlcnQiICJzMV9jaGVja3BvaW50IiAi
czFfZ2V0X2NoYW5nZXMiICJzMF9jb21taXQiICJzMV9nZXRfY2hhbmdlcyIKCnBlcm11dGF0aW9u
ICJzMF9pbml0IiAiczBfYmVnaW4iICJzMF9zYXZlcG9pbnQiICJzMF9hbHRlciIgICJzMV9jaGVj
a3BvaW50IiAiczFfZ2V0X2NoYW5nZXMiICJzMF9jb21taXQiICJzMl9ydW5fdHhuIiAiczBfYmVn
aW4iICJzMF9pbnNlcnQiICJzMV9jaGVja3BvaW50IiAiczFfZ2V0X2NoYW5nZXMiICJzMF9jb21t
aXQiICJ0aW1lIiAiczFfZ2V0X2NoYW5nZXMiICJ0aW1lIgo=
--000000000000b1ec1705e3957d40--