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>
 <CAD21AoDFC5btYbcGOLMWimotg+N45pXJqyEOhbfsOHdz8XUKpg@mail.gmail.com>
 <CAA4eK1KZHnHhZdL1-Fa-6+sw4JHEPjLjZ=+wTwRHnkooXoxp2A@mail.gmail.com>
 <CAD21AoDJiAiyp3AHbaPJ1w=twWs2sThUiNDxvpJ+k98voV2PSg@mail.gmail.com>
 <CAA4eK1KxnYuhU5AyusDAP1SKtVmwAS1wYJ+VNL155x=swbx+gA@mail.gmail.com>
 <CAD21AoAyNPrOFg+QGh+=4205TU0=yrE+QyMgzStkH85uBZXptQ@mail.gmail.com>
 <TYCPR01MB8373EFE369CD515805C16292ED8B9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CAD21AoCdiqOcVmZgoh3nRxhPNqueUGCbPRkzcmjSgD6GUU2=gA@mail.gmail.com>
 <CAA4eK1K07S5tiHq7P+T51PDhHxaYiyfWZW4OP52gtTxBGV2EMA@mail.gmail.com>
 <CAD21AoDfZCmjUS74t+_7j2g+MshYcp7026mufR3SRKW6XAgAHA@mail.gmail.com>
 <CAA4eK1JWaY5t+20iteUnPZy0E+nJi4KPqH-wEYyKx7YkcHmEqA@mail.gmail.com>
In-Reply-To: 
 <CAA4eK1JWaY5t+20iteUnPZy0E+nJi4KPqH-wEYyKx7YkcHmEqA@mail.gmail.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Tue, 19 Jul 2022 16:02:26 +0900
Message-ID: 
 <CAD21AoC1E09F_swzOPQQE9P4tf3ADr0cyATesGHA3JrySAytNA@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: "osumi.takamichi@fujitsu.com" <osumi.takamichi@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: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAD21AoC1E09F_swzOPQQE9P4tf3ADr0cyATesGHA3JrySAytNA%40mail.gmail.com>
Precedence: bulk

On Tue, Jul 19, 2022 at 1:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote=
:
>
> On Tue, Jul 19, 2022 at 6:34 AM Masahiko Sawada <sawada.mshk@gmail.com> w=
rote:
> >
> > On Mon, Jul 18, 2022 at 1:12 PM Amit Kapila <amit.kapila16@gmail.com> w=
rote:
> > >
> > > On Fri, Jul 15, 2022 at 8:09 PM Masahiko Sawada <sawada.mshk@gmail.co=
m> wrote:
> > > >
> > > > This patch should have the fix for the issue that Shi yu reported. =
Shi
> > > > yu, could you please test it again with this patch?
> > > >
> > >
> > > Can you explain the cause of the failure and your fix for the same?
> >
> > @@ -1694,6 +1788,8 @@ out:
> >     /* be tidy */
> >     if (ondisk)
> >         pfree(ondisk);
> > +   if (catchange_xip)
> > +       pfree(catchange_xip);
> >
> > Regarding the above code in the previous version patch, looking at the
> > generated assembler code shared by Shi yu offlist, I realized that the
> > =E2=80=9Cif (catchange_xip)=E2=80=9D is removed (folded) by gcc optimiz=
ation. This is
> > because we dereference catchange_xip before null-pointer check as
> > follow:
> >
> > +   /* copy catalog modifying xacts */
> > +   sz =3D sizeof(TransactionId) * catchange_xcnt;
> > +   memcpy(ondisk_c, catchange_xip, sz);
> > +   COMP_CRC32C(ondisk->checksum, ondisk_c, sz);
> > +   ondisk_c +=3D sz;
> >
> > Since sz is 0 in this case, memcpy doesn=E2=80=99t do anything actually=
.
> >
> > By checking the assembler code, I=E2=80=99ve confirmed that gcc does th=
e
> > optimization for these code and setting
> > -fno-delete-null-pointer-checks flag prevents the if statement from
> > being folded. Also, I=E2=80=99ve confirmed that adding the check if
> > "catchange.xcnt > 0=E2=80=9D before the null-pointer check also can pre=
vent
> > that. Adding a check  if "catchange.xcnt > 0=E2=80=9D looks more robust=
. I=E2=80=99ve
> > added a similar check for builder->committed.xcnt as well for
> > consistency. builder->committed.xip could have no transactions.
> >
>
> Good work. I wonder without comments this may create a problem in the
> future. OTOH, I don't see adding a check "catchange.xcnt > 0" before
> freeing the memory any less robust. Also, for consistency, we can use
> a similar check based on xcnt in the SnapBuildRestore to free the
> memory in the below code:
> + /* set catalog modifying transactions */
> + if (builder->catchange.xip)
> + pfree(builder->catchange.xip);

I would hesitate to add comments about preventing the particular
optimization. I think we do null-pointer-check-then-pfree many place.
It seems to me that checking the array length before memcpy is more
natural than checking both the array length and the array existence
before pfree.

Regards,

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