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