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
signature.asc
Description: PGP signature