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
