Re: Non-reserved replication slots and slot advancing

On 2018-07-09 15:48:33 +0900, Michael Paquier wrote:
> On Mon, Jul 09, 2018 at 03:13:04PM +0900, Kyotaro HORIGUCHI wrote:
> > Looking the attached patch, I noticed that both "WAL" and "wal"
> > are used in similar ERROR messages. Grepping the source tree
> > showed me that it is always in upper case letters except in the
> > case it is a part of other words like variable/column/function
> > names or "walsender". This is the same with the word "lsn".
> 
> Thanks for the lookup.
> 
> I see.  Indeed, let's fix at the same time the error message close by.
> xlog.c uses "WAL location (LSN)" for the same thing, so I am sticking
> with that as per the attached.  I'll go commit that if there are no
> objections.  If you see any others which you would like to fix, please
> feel free to send a patch.

I object to this wording change - it doesn't seem to add anything but
confusion.

> diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
> index 2806e1076c..b55dadfe2f 100644
> --- a/src/backend/replication/slotfuncs.c
> +++ b/src/backend/replication/slotfuncs.c
> @@ -465,7 +465,7 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS)
>  
>  	if (XLogRecPtrIsInvalid(moveto))
>  		ereport(ERROR,
> -				(errmsg("invalid target wal lsn")));
> +				(errmsg("invalid target WAL location (LSN)")));

and it seems to at least bend the rules of the style guide a bit.


>  	/* Build a tuple descriptor for our result type */
>  	if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
> @@ -483,6 +483,15 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS)
>  	/* Acquire the slot so we "own" it */
>  	ReplicationSlotAcquire(NameStr(*slotname), true);
>  
> +	/* A slot whose restart_lsn has never been reserved cannot be advanced */
> +	if (XLogRecPtrIsInvalid(MyReplicationSlot->data.restart_lsn))
> +	{
> +		ReplicationSlotRelease();
> +		ereport(ERROR,
> +				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +				 errmsg("cannot move slot not reserving WAL")));
> +	}
> +

I don't like this error message. It's unclear what refers to exactly
what.  Nor is "move" a terminology used otherwise.  How about:
"cannot advance replication slot that has not previously reserved WAL"
or something similar?

Also, ERRCODE_FEATURE_NOT_SUPPORTED doesn't quite seem right. How about
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE?

Greetings,

Andres Freund