Re: Minimal logical decoding on standbys - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Minimal logical decoding on standbys
Date
Msg-id CAA4eK1LzWv2p+97vsHekhftSuMR0m-qVEdoCpASUApN6pXtZpQ@mail.gmail.com
Whole thread Raw
In response to Re: Minimal logical decoding on standbys  ("Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>)
Responses Re: Minimal logical decoding on standbys
List pgsql-hackers
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.



pgsql-hackers by date:

Previous
From: tender wang
Date:
Subject: Re: same query but different result on pg16devel and pg15.2
Next
From: Richard Guo
Date:
Subject: Re: same query but different result on pg16devel and pg15.2