Re: Add lookup table for replication slot invalidation causes

2024-02-23 Thread Michael Paquier
On Fri, Feb 23, 2024 at 09:04:04AM +1100, Peter Smith wrote: > I would've just removed every local variable instead of adding more of > them. I also felt the iteration starting from RS_INVAL_NONE instead of > 0 is asserting RS_INVAL_NONE must always be the first enum and can't > be rearranged. Prob

Re: Add lookup table for replication slot invalidation causes

2024-02-22 Thread Peter Smith
On Thu, Feb 22, 2024 at 5:56 PM Michael Paquier wrote: > > On Thu, Feb 22, 2024 at 05:30:08PM +1100, Peter Smith wrote: > > Oops. Perhaps I meant more like below -- in any case, the point was > > the same -- to ensure RS_INVAL_NONE is what returns if something > > unexpected happens. > > You are r

Re: Add lookup table for replication slot invalidation causes

2024-02-21 Thread Bharath Rupireddy
On Thu, Feb 22, 2024 at 12:26 PM Michael Paquier wrote: > > On Thu, Feb 22, 2024 at 05:30:08PM +1100, Peter Smith wrote: > > Oops. Perhaps I meant more like below -- in any case, the point was > > the same -- to ensure RS_INVAL_NONE is what returns if something > > unexpected happens. > > You are

Re: Add lookup table for replication slot invalidation causes

2024-02-21 Thread Michael Paquier
On Thu, Feb 22, 2024 at 05:30:08PM +1100, Peter Smith wrote: > Oops. Perhaps I meant more like below -- in any case, the point was > the same -- to ensure RS_INVAL_NONE is what returns if something > unexpected happens. You are right that this could be a bit confusing, even if we should never reac

Re: Add lookup table for replication slot invalidation causes

2024-02-21 Thread Peter Smith
On Thu, Feb 22, 2024 at 5:19 PM Peter Smith wrote: > > Hi, Sorry for the late comment but isn't the pushed logic now > different to what it was there before? > > IIUC previously (in a non-debug build) if the specified > conflict_reason was not found, it returned RS_INVAL_NONE -- now it > seems to

Re: Add lookup table for replication slot invalidation causes

2024-02-21 Thread Peter Smith
Hi, Sorry for the late comment but isn't the pushed logic now different to what it was there before? IIUC previously (in a non-debug build) if the specified conflict_reason was not found, it returned RS_INVAL_NONE -- now it seems to return whatever enum happens to be last. How about something mor

Re: Add lookup table for replication slot invalidation causes

2024-02-21 Thread Michael Paquier
On Wed, Feb 21, 2024 at 12:50:00PM +0530, Bharath Rupireddy wrote: > Please see the attached v3 patch. Seems globally OK, so applied. I've simplified a bit the comments, painted some extra const, and kept variable name as conflict_reason as the other routines of slot.h use "name" already to refe

Re: Add lookup table for replication slot invalidation causes

2024-02-20 Thread Bharath Rupireddy
On Wed, Feb 21, 2024 at 11:56 AM Michael Paquier wrote: > > Note the recent commit 74a730631065 where Alvaro has changed for the > lwlock tranche names. That's quite elegant. Yes, that's absolutely neat. FWIW, designated initializer syntax can be used in a few more places though. I'm not sure ho

Re: Add lookup table for replication slot invalidation causes

2024-02-20 Thread Michael Paquier
On Wed, Feb 21, 2024 at 09:49:37AM +0530, Bharath Rupireddy wrote: > On Wed, Feb 21, 2024 at 5:04 AM Michael Paquier wrote: > 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/

Re: Add lookup table for replication slot invalidation causes

2024-02-20 Thread Bharath Rupireddy
On Wed, Feb 21, 2024 at 5:04 AM Michael Paquier 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[] = {

Re: Add lookup table for replication slot invalidation causes

2024-02-20 Thread Michael Paquier
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

Re: Add lookup table for replication slot invalidation causes

2024-02-20 Thread Jelte Fennema-Nio
On Tue, 20 Feb 2024 at 12:11, Bharath Rupireddy wrote: > Thoughts? 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",