Re: Improve error handling for invalid slots and ensure a same 'inactive_since' time for inactive slots - Mailing list pgsql-hackers

On Wed, Jan 29, 2025 at 3:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>

+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("can no longer get changes from replication slot \"%s\"",
+    NameStr(slot->data.name)),
+ errdetail_internal("%s", err_detail->data));

ERROR message appears to be specific to logical slots, it will occur
for physical slots as well, so I suggest changing from "can no longer
get changes from replication slot" to "can no longer access from
replication slot"

The errdetail should be used as we have in ReplicationSlotAlter.
errdetail("This replication slot has been invalidated due to \"%s\".",
SlotInvalidationCauses[MyReplicationSlot->data.invalidated]));

You have given the following reason in another thread for not giving a
message during acquire in ReplicationSlotAlter: "Because
ReplicationSlotAlter() already handles errors immediately after
acquiring the slot. It raises errors for invalidated slots and also
raises a different error message if the slot is a physical one. So, In
case of ALTER, I feel it is okay to acquire the slot first without
raising errors and then handle errors in the pre-defined way. Similar
immediate error handling is not available at other places."

It is better to unify the error in one place rather than at different
places. One benefit is error message contents will be the same which
is currently not the case.

BTW, in the commit message, you only mention the place where we get
the message without a patch during logical replication, it is better
to mention the timing for physical replication as well.

--
With Regards,
Amit Kapila.