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