Re: Improve error handling for invalid slots and ensure a same 'inactive_since' time for inactive slots

On Thu, Jan 30, 2025 at 12:00 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> Removed patch v1-0001. Please find the attached version 2 of 0002,
> which is now v2-0001.
>
> In v2, I have addressed all comments till now from [1] , [2] and [3].
> - With the proposed errdetail message in [3], the new function
> RaiseSlotInvalidationError() is no longer required.
>

I have made minor changes in the attached. The main change is to raise
an ERROR before we broadcast to let everybody know we've modified this
slot. There is no point in broadcasting if the slot is unusable.

> - Updated the test files to match the new error message.
>

- /ERROR: cannot alter invalid replication slot "vacuum_full_inactiveslot"/
+ /ERROR: can no longer access replication slot "vacuum_full_inactiveslot"/

- "can no longer get changes from replication slot
\"shared_row_removal_activeslot\""
+ "can no longer access replication slot \"shared_row_removal_activeslot\""

With the above changes, we made the ERROR message generic which was
required to raise it from the central place. This looks reasonable to
me. The other option that occurred to me is to write it as: "can no
longer use replication slot "vacuum_full_inactiveslot" to access WAL
or modify it" but I find the one used currently in the patch better as
this is longer and may need modification in future if we start using
slot for something else.

--
With Regards,
Amit Kapila.