Re: Copy function for logical replication slots

On Tue, Nov 27, 2018 at 3:46 AM Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
>
> Hi,
>
> On 26/11/2018 01:29, Masahiko Sawada wrote:
> > On Sun, Nov 25, 2018 at 12:27 AM Petr Jelinek
> > <petr.jelinek@2ndquadrant.com> wrote:
> >>
> >> The more serious thing is:
> >>
> >>> +     if (MyReplicationSlot)
> >>> +             ReplicationSlotRelease();
> >>> +
> >>> +     /* Release the saved slot if exist while preventing double releasing */
> >>> +     if (savedslot && savedslot != MyReplicationSlot)
> >>
> >> This won't work as intended as the ReplicationSlotRelease() will set
> >> MyReplicationSlot to NULL, you might need to set aside MyReplicationSlot
> >> to yet another temp variable inside this function prior to releasing it.
> >>
> >
> > You're right. I've fixed it by checking if we need to release the
> > saved slot before releasing the origin slot. Is that right?
> > Attached an updated patch.
> >
>
> Sounds good.
>
> I do have one more minor gripe after reading though again:
>

Thank you for the review comment and sorry for the late response.

> > +
> > +               /*
> > +                * The requested wal lsn is no longer available. We don't want to retry
> > +                * it, so raise an error.
> > +                */
> > +               if (!XLogRecPtrIsInvalid(requested_lsn))
> > +               {
> > +                       char filename[MAXFNAMELEN];
> > +
> > +                       XLogFileName(filename, ThisTimeLineID, segno, wal_segment_size);
> > +                       ereport(ERROR,
> > +                                       (errmsg("could not reserve WAL segment %s", filename)));
> > +               }
>
> I would reword the comment to something like "The caller has requested a
> specific wal lsn which we failed to reserve. We can't retry here as the
> requested wal is no longer available." (It took me a while to understand
> this part).
>
> Also the ereport should have errcode as it's going to be thrown to user
> sessions and it might be better if the error itself used same wording as
> CheckXLogRemoved() and XLogRead() for consistency. What do you think?
>

I agreed your both comments. I've changed the above comment and
ereport. Attached the updated version patch.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center