Thank you for polishing and committing this.
At Tue, 28 Apr 2020 20:47:10 -0400, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in
> I pushed this one. Some closing remarks:
>
> On 2020-Apr-28, Alvaro Herrera wrote:
>
> > On 2020-Apr-28, Kyotaro Horiguchi wrote:
>
> > > Agreed to describe what is failed rather than the cause. However,
> > > logical replications slots are always "previously reserved" at
> > > creation.
> >
> > Bah, of course. I was thinking in making the equivalent messages all
> > identical in all callsites, but maybe they should be different when
> > slots are logical. I'll go over them again.
>
> I changed the ones that can only be logical slots so that they no longer
> say "previously reserved WAL". The one in
> pg_replication_slot_advance still uses that wording, because I didn't
> think it was worth creating two separate error paths.
Agreed.
> > > ERROR: replication slot "repl" is not usable to get changes
> >
> > That wording seems okay, but my specific point for this error message is
> > that we were trying to use a physical slot to get logical changes; so
> > the fact that the slot has been invalidated is secondary and we should
> > complain about the *type* of slot rather than the restart_lsn.
>
> I moved the check for validity to after CreateDecodingContext, so the
> other errors are reported preferently. I also chose a different wording:
Yes. It is what I had in my mind. The function checks invariable
properties of the slot, then the following code checks a variable
state of the same.
> /*
> * After the sanity checks in CreateDecodingContext, make sure the
> * restart_lsn is valid. Avoid "cannot get changes" wording in this
> * errmsg because that'd be confusingly ambiguous about no changes
> * being available.
> */
> if (XLogRecPtrIsInvalid(MyReplicationSlot->data.restart_lsn))
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> errmsg("can no longer get changes from replication slot \"%s\"",
> NameStr(*name)),
> errdetail("This slot has never previously reserved WAL, or has been invalidated.")));
>
> I hope this is sufficiently clear, but if not, feel free to nudge me and
> we can discuss it further.
That somewhat sounds odd that 'we "no longer" get changes from "never
previously reserved" slots'. More than that, I think we don't reach
there for physical slots, since CreateDecodingContext doesn't accept a
physical slot and ERRORs out. (That is the reason for the location of
the checking.)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center