On Mon, Apr 3, 2023 at 8:51 PM Drouvot, Bertrand <[email protected]> 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.
