MIME-Version: 1.0
References: 
 <CAA4eK1KCxW8bA+LrXZYsww42KoYo4nQ_Kycz1cw7TR0UXDXfMA@mail.gmail.com>
 <CAD21AoBRtPLEYcTi5YOzRbh06xXqM1aZmi9Dkczfbd5=6hKTrg@mail.gmail.com>
 <CAA4eK1KiiXHtj641_G2v7s+S33CVPG9k3OBMVHb3hn+f9YBC7A@mail.gmail.com>
 <20220802.153012.363912034520325077.horikyota.ntt@gmail.com>
In-Reply-To: <20220802.153012.363912034520325077.horikyota.ntt@gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 2 Aug 2022 13:54:43 +0530
Message-ID: 
 <CAA4eK1LUis6LrKDi=VAWhP-0OniC4ThbXueAhdaQHpdCg4MvRg@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: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: sawada.mshk@gmail.com, shiy.fnst@fujitsu.com, bdrouvot@amazon.com,
	pgsql-hackers@lists.postgresql.org, minsoo@amazon.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1LUis6LrKDi%3DVAWhP-0OniC4ThbXueAhdaQHpdCg4MvRg%40mail.gmail.com>
Precedence: bulk

On Tue, Aug 2, 2022 at 12:00 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Mon, 1 Aug 2022 20:01:00 +0530, Amit Kapila <amit.kapila16@gmail.com> =
wrote in
> > On Mon, Aug 1, 2022 at 7:46 AM Masahiko Sawada <sawada.mshk@gmail.com> =
wrote:
> > >
> > > On Fri, Jul 29, 2022 at 3:45 PM Amit Kapila <amit.kapila16@gmail.com>=
 wrote:
> > > >
> > >
> > > I've attached updated patches for all branches. Please review them.
> > >
> >
> > Thanks, the patches look mostly good to me. I have made minor edits by
> > removing 'likely' from a few places as those don't seem to be adding
> > much value, changed comments at a few places, and was getting
> > compilation in error in v11/10 (snapbuild.c:2111:3: error: =E2=80=98for=
=E2=80=99 loop
> > initial declarations are only allowed in C99 mode) which I have fixed.
> > See attached, unless there are major comments/suggestions, I am
> > planning to push this day after tomorrow (by Wednesday) after another
> > pass.
>
>
> +       {
> +               int                     save_errno =3D errno;
> +
> +               CloseTransientFile(fd);
> +
> +               if (readBytes < 0)
> +               {
> +                       errno =3D save_errno;
> +                       ereport(ERROR,
>
> Do we need the CloseTransientFile(fd) there?  This call requires errno
> to be remembered but anyway OpenTransientFile'd files are to be close
> at transaction end.  Actually CloseTransientFile() is not called
> before error'ing-out at error in other places.
>

But this part of the code is just a copy of the existing code. See:

- if (readBytes !=3D sizeof(SnapBuild))
- {
- int save_errno =3D errno;
-
- CloseTransientFile(fd);
-
- if (readBytes < 0)
- {
- errno =3D save_errno;
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\": %m", path)));
- }
- else
- ereport(ERROR,
- (errcode(ERRCODE_DATA_CORRUPTED),
- errmsg("could not read file \"%s\": read %d of %zu",
- path, readBytes, sizeof(SnapBuild))));
- }

We just moved it to a separate function as the same code is being
duplicated to multiple places.

--=20
With Regards,
Amit Kapila.