Re: Introduce XID age and inactive timeout based replication slot invalidation

Hi Nisha,

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.

Anyway, YMMV.

======

Other review comments follow ...

src/backend/replication/slot.c

InvalidatePossiblyObsoleteSlot:

1.
Having all those 'gotos' seems like something best avoided. Can you
try removing them to see if it improves this function? IIUC you maybe
can try rid all of them using logic like:

- assign invalidation_cause = NONE outside the loop
- loop until invalidation_cause != NONE
- include 'invalidation_cause == NONE' condition with all the bit flag checks
- reassign invalidation_cause = NONE in the racy place where you want
to continue the loop.

and instead just keep looping and checking while the
'invalidation_cause' remains NONE.

======
Kind Regards,
Peter Smith.
Fujitsu Australia