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

From Michael Paquier
Subject Re: Add lookup table for replication slot invalidation causes
Date
Msg-id ZdU3CHqza9XJw4P-@paquier.xyz
Whole thread Raw
In response to Re: Add lookup table for replication slot invalidation causes  (Jelte Fennema-Nio <postgres@jeltef.nl>)
Responses Re: Add lookup table for replication slot invalidation causes
List pgsql-hackers
On Tue, Feb 20, 2024 at 05:53:03PM +0100, Jelte Fennema-Nio wrote:
> Seems like a good improvement overall. But I'd prefer the definition
> of the lookup table to use this syntax:
>
> const char *const SlotInvalidationCauses[] = {
>     [RS_INVAL_NONE] = "none",
>     [RS_INVAL_WAL_REMOVED] = "wal_removed",
>     [RS_INVAL_HORIZON] = "rows_removed",
>     [RS_INVAL_WAL_LEVEL] = "wal_level_sufficient",
> };

+1.

> Regarding the actual patch:
>
> -   Assert(conflict_reason);
>
> Probably we should keep this Assert. As well as the Assert(0)

The assert(0) at the end of the routine, likely so.  I don't see a
huge point for the assert on conflict_reason as we'd crash anyway  on
strcmp, no?

> +   for (cause = RS_INVAL_NONE; cause <= RS_INVAL_MAX_CAUSES; cause++)
>
> Strictly speaking this is a slight change in behaviour, since now
> "none" is also parsed. That seems fine to me though.

Yep.  This does not strike me as an issue.  We only use
GetSlotInvalidationCause() in synchronize_slots(), mapping to NULL in
the case of "none".

Agreed that this is an improvement.

+/* Maximum number of invalidation causes */
+#define    RS_INVAL_MAX_CAUSES RS_INVAL_WAL_LEVEL

There is no need to add that to slot.h: it is only used in slot.c.
--
Michael

Attachment

pgsql-hackers by date:

Previous
From: Michail Nikolaev
Date:
Subject: Re: Revisiting {CREATE INDEX, REINDEX} CONCURRENTLY improvements
Next
From: Quan Zongliang
Date:
Subject: Re: Change the bool member of the Query structure to bits