Thread: Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns

At Tue, 2 Aug 2022 13:54:43 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in 
> On Tue, Aug 2, 2022 at 12:00 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > +       {
> > +               int                     save_errno = errno;
> > +
> > +               CloseTransientFile(fd);
> > +
> > +               if (readBytes < 0)
> > +               {
> > +                       errno = 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.
..
> We just moved it to a separate function as the same code is being
> duplicated to multiple places.

There are code paths that doesn't CloseTransientFile() explicitly,
too.  If there were no need of save_errno there, that'd be fine.  But
otherwise I guess we prefer to let the orphan fds closed by ERROR and
I don't think we need to preserve the less-preferred code pattern (if
we actually prefer not to have the explicit call).

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Aug 3, 2022 at 10:20 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Tue, 2 Aug 2022 13:54:43 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
> > On Tue, Aug 2, 2022 at 12:00 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > >
> > > +       {
> > > +               int                     save_errno = errno;
> > > +
> > > +               CloseTransientFile(fd);
> > > +
> > > +               if (readBytes < 0)
> > > +               {
> > > +                       errno = 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.
> ..
> > We just moved it to a separate function as the same code is being
> > duplicated to multiple places.
>
> There are code paths that doesn't CloseTransientFile() explicitly,
> too.  If there were no need of save_errno there, that'd be fine.  But
> otherwise I guess we prefer to let the orphan fds closed by ERROR and
> I don't think we need to preserve the less-preferred code pattern (if
> we actually prefer not to have the explicit call).

Looking at other codes in snapbuild.c, we call CloseTransientFile()
before erroring out in SnapBuildSerialize(). I think it's better to
keep it consistent with nearby codes in this patch. I think if we
prefer the style of closing the file by ereport(ERROR), it should be
done for all of them in a separate patch.

Regards,

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

On Wed, Aug 3, 2022 at 7:05 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Aug 3, 2022 at 10:20 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > At Tue, 2 Aug 2022 13:54:43 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
> > > On Tue, Aug 2, 2022 at 12:00 PM Kyotaro Horiguchi
> > > <horikyota.ntt@gmail.com> wrote:
> > > >
> > > > +       {
> > > > +               int                     save_errno = errno;
> > > > +
> > > > +               CloseTransientFile(fd);
> > > > +
> > > > +               if (readBytes < 0)
> > > > +               {
> > > > +                       errno = 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.
> > ..
> > > We just moved it to a separate function as the same code is being
> > > duplicated to multiple places.
> >
> > There are code paths that doesn't CloseTransientFile() explicitly,
> > too.  If there were no need of save_errno there, that'd be fine.  But
> > otherwise I guess we prefer to let the orphan fds closed by ERROR and
> > I don't think we need to preserve the less-preferred code pattern (if
> > we actually prefer not to have the explicit call).
>
> Looking at other codes in snapbuild.c, we call CloseTransientFile()
> before erroring out in SnapBuildSerialize(). I think it's better to
> keep it consistent with nearby codes in this patch. I think if we
> prefer the style of closing the file by ereport(ERROR), it should be
> done for all of them in a separate patch.
>

+1. I also feel it is better to change it in a separate patch as this
is not a pattern introduced by this patch.

-- 
With Regards,
Amit Kapila.