Re: Add lookup table for replication slot invalidation causes - Mailing list pgsql-hackers

On Thu, Feb 22, 2024 at 5:56 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Feb 22, 2024 at 05:30:08PM +1100, Peter Smith wrote:
> > Oops. Perhaps I meant more like below -- in any case, the point was
> > the same -- to ensure RS_INVAL_NONE is what returns if something
> > unexpected happens.
>
> You are right that this could be a bit confusing, even if we should
> never reach this state.  How about avoiding to return the index of the
> loop as result, as of the attached?  Would you find that cleaner?
> --

Hi, yes, it should never happen, but thanks for making the changes.

I would've just removed every local variable instead of adding more of
them. I also felt the iteration starting from RS_INVAL_NONE instead of
0 is asserting RS_INVAL_NONE must always be the first enum and can't
be rearranged. Probably it will never happen, but why require it?

------
ReplicationSlotInvalidationCause
GetSlotInvalidationCause(const char *conflict_reason)
{
    for (ReplicationSlotInvalidationCause cause = 0; cause <=
RS_INVAL_MAX_CAUSES; cause++)
        if (strcmp(SlotInvalidationCauses[cause], conflict_reason) == 0)
            return cause;

    Assert(0);
    return RS_INVAL_NONE;
}
------

But maybe those nits are a matter of personal choice. Your patch code
addressed my main concern, so it LGTM.

----------
Kind Regards,
Peter Smith.
Fujitsu Australia