On Wed, Feb 21, 2024 at 5:04 AM Michael Paquier <mich...@paquier.xyz> wrote:
>
> 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.Done that way. I'm fine with the designated initialization [1] that an ISO C99 compliant compiler offers. PostgreSQL installation guide https://www.postgresql.org/docs/current/install-requirements.html says that we need an at least C99-compliant ISO/ANSI C compiler. [1] https://open-std.org/JTC1/SC22/WG14/www/docs/n494.pdf https://en.cppreference.com/w/c/99 https://www.ibm.com/docs/en/zos/2.4.0?topic=initializers-designated-aggregate-types-c-only > > 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? Right, but an assertion isn't a bad idea there as it can generate a backtrace as opposed to the crash generating just SEGV note (and perhaps a crash dump) in server logs. With these two asserts, the behavior (asserts on null and non-existent inputs) is the same as what GetSlotInvalidationCause has right now. > +/* 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. Right, but it needs to be updated whenever a new cause is added to enum ReplicationSlotInvalidationCause. Therefore, I think it's better to be closer there in slot.h. Please see the attached v2 patch. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
v2-0001-Add-lookup-table-for-replication-slot-invalidatio.patch
Description: Binary data
