Re: [HACKERS] Restricting maximum keep segments by repslots

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