On Wed, Feb 21, 2024 at 5:04 AM Michael Paquier <michael@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