Hi,
TBH, I don't like the state of 0001 much. I'm working on polishing it now.
A lot of the new functions in slot.h don't seem right to me:
- ObsoleteSlotIsInvalid() - isn't an obsolete slot by definition invalid?
- Why does ObsoleteSlotIsInvalid() sometime check invalidated_at and sometimes
not?
- DoNotInvalidateSlot() seems too generic a name for a function exposed to the
outside of slot.c
- TransactionIdIsValidPrecedesOrEquals() shouldn't be defined in slot.h -
also, it's not actually clear what semantics it's trying to have.
- there's no commonality in naming between the functions used to test if a
slot needs to be invalidated (SlotIsFreshEnough() vs
LogicalSlotIsNotConflicting()).
Leaving naming etc aside, most of these don't seem to belong in slot.h, but
should just be in slot.c - there aren't conceivable users from outside slot.c.
Independent of this patch: What's the point of invalidated_at? The only reads
of it are done like
invalidated = (!XLogRecPtrIsInvalid(s->data.invalidated_at) &&
XLogRecPtrIsInvalid(s->data.restart_lsn));
i.e. the actual LSN is not used.
ISTM that we should just have it be a boolean, and that it should be used by
the different kinds of invalidating a slot.
Greetings,
Andres Freund
