On Mon, Apr 3, 2023 at 8:51 PM Drouvot, Bertrand
<bertranddrouvot...@gmail.com> wrote:
>
> On 4/2/23 10:10 PM, Andres Freund wrote:
> > Hi,
> >> restart_lsn = s->data.restart_lsn;
> >> -
> >> - /*
> >> - * If the slot is already invalid or is fresh enough, we
> >> don't need to
> >> - * do anything.
> >> - */
> >> - if (XLogRecPtrIsInvalid(restart_lsn) || restart_lsn >=
> >> oldestLSN)
> >> + slot_xmin = s->data.xmin;
> >> + slot_catalog_xmin = s->data.catalog_xmin;
> >> +
> >> + /* the slot has been invalidated (logical decoding conflict
> >> case) */
> >> + if ((xid && ((LogicalReplicationSlotIsInvalid(s)) ||
> >> + /* or the xid is valid and this is a non conflicting slot */
> >> + (TransactionIdIsValid(*xid) &&
> >> !(LogicalReplicationSlotXidsConflict(slot_xmin, slot_catalog_xmin,
> >> *xid))))) ||
> >> + /* or the slot has been invalidated (obsolete LSN case) */
> >> + (!xid && (XLogRecPtrIsInvalid(restart_lsn) ||
> >> restart_lsn >= oldestLSN)))
> >> {
> >
> > This still looks nearly unreadable. I suggest moving comments outside of the
> > if (), remove redundant parentheses, use a function to detect if the slot
> > has
> > been invalidated.
> >
>
> I made it as simple as:
>
> /*
> * If the slot is already invalid or is a non conflicting slot, we
> don't
> * need to do anything.
> */
> islogical = xid ? true : false;
>
> if (SlotIsInvalid(s, islogical) || SlotIsNotConflicting(s,
> islogical, xid, &oldestLSN))
>
> in V56 attached.
>
Here the variable 'islogical' doesn't seem to convey its actual
meaning because one can imagine that it indicates whether the slot is
logical which I don't think is the actual intent. One idea to simplify
this is to introduce a single function CanInvalidateSlot() or
something like that and move the logic from both the functions
SlotIsInvalid() and SlotIsNotConflicting() into the new function.
--
With Regards,
Amit Kapila.
