Re: Minimal logical decoding on standbys

On Mon, Apr 3, 2023 at 8:51 PM Drouvot, Bertrand
<bertranddrouvot.pg@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.