On Mon, Feb 10, 2025 at 11:33 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Some review comments for v72-0001. > > ====== > GENERAL > > My preference was to just keep the enum as per v70 for the *actual* > cause, and introduce a separate set of bit flags for *possible* causes > to be checked. This creates a clear code separation between the actual > and possible. It also eliminates the need to jump through hoops just > to map a cause to its name. > > You wrote: > > > OTOH, keeping the enums as they are in v70, and defining new macros > for the very similar purpose could add unnecessary complexity to code > management. > > Since both the enum and the bit flags would be defined in slot.h > adjacent to each other I don't foresee much complexity. I concede, a > dev might write code and accidentally muddle the enum instead of the > flag or vice versa but that's an example of sloppiness, not > complexity. Certainly there would be fewer necessary changes than what > are in the v72 patch due to all the cause/causename mappings. For > example, > > slot.h - Now, introduces a NEW typedef SlotInvalidationCauseMap > slot.h - Now, need extern for NEW function GetSlotInvalidationCauseName > > slot.c - Now, needed minor rewrite of GetSlotInvalidationCause instead > of leaving it as-is > slot.c - Now, needs a whole NEW looping function > GetSlotInvalidationCauseName instead of direct array index. > > Several place now must call to the GetSlotInvalidationCauseName where > previously a simple direct array lookup was done > slot.c - NEW call in ReplicationSlotAcquire > slotfuncs.c - NEW call in pg_get_replication_slots > > ~ > > FWIW, I've attached a topup patch using my idea just to see what it > might look like. The result was 20 lines less code. >
I don't like the idea of maintaining the same information in two different ways (as enum and bit flags). We already have a few cases of defining bit flags as part of enums like ScanOptions and relopt_kind, so I feel following that model would be a better approach. -- With Regards, Amit Kapila.
