Re: Catalog_xmin is not advanced when a logical slot is lost

Hi Sirisha,

Thanks for identifying the bug and the solution. Some review comments inlined.

On Mon, Nov 21, 2022 at 2:49 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2022-Nov-20, sirisha chamarthi wrote:
>
> > Hi Hackers,
> >
> > forking this thread from the discussion [1] as suggested by Amit.
> >
> > Catalog_xmin is not advanced when a logical slot is invalidated (lost)
> > until the invalidated slot is dropped. This patch ignores invalidated slots
> > while computing the oldest xmin. Attached a small patch to address this and
> > the output after the patch is as shown below.
>
> Oh wow, that's bad :-( I'll get it patched immediately.

+ /* ignore invalid slots while computing the oldest xmin */
+ if (TransactionIdIsValid(invalidated_at_lsn))
+ continue;

I think the condition should be

if (!XLogRecPtrIsInvalid(invalidated_at_lsn)) LSN and XID are
different data types.

and to be inline with pg_get_replication_slots()
361 if (XLogRecPtrIsInvalid(slot_contents.data.restart_lsn) &&
362 !XLogRecPtrIsInvalid(slot_contents.data.invalidated_at))
363 walstate = WALAVAIL_REMOVED;

we should also check restart_lsn.

I would write this as

bool invalidated_slot = false;

then under spinlock
invalidated_slot = XLogRecPtrIsInvalid(s->data.restart_lsn) &&
!XLogRecPtrIsInvalid(s->data.invalidated_at);

if (invalidated_slot)
continue.

--
Best Wishes,
Ashutosh Bapat