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@alvh.no-ip.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