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

On Mon, Nov 21, 2022 at 5:39 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2022-Nov-21, Ashutosh Bapat wrote:
>
> > I think the condition should be
> >
> > if (!XLogRecPtrIsInvalid(invalidated_at_lsn)) LSN and XID are
> > different data types.
>
> Yeah, this bit is wrong. I agree with your suggestion to just keep a
> boolean flag, as we don't need more than that.
>
> > 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.
>
> Hmm, I'm not sure about this one. I'm not sure why we check both in
> pg_get_replication_slots. I suppose we didn't want to ignore a slot
> only if it had a non-zero invalidated_at in case it was accidental (say,
> we initialize a slot as valid, but forget to zero-out the invalidated_at
> value); but I think that's pretty much useless. This is only changed
> with the spinlock held, so it's not like you can see partially-set
> state.
>
> In fact, as I recall we could replace invalidated_at in
> ReplicationSlotPersistentData with a boolean "invalidated" flag, and
> leave restart_lsn alone when invalidated. IIRC the only reason we
> didn't do it that way was that we feared some code might observe some
> valid value in restart_lsn without noticing that it belonged to an
> invalidate slot. (Which is exactly what happened now, except with a
> different field.)
>

Maybe. In that case pg_get_replication_slots() should be changed. We
should use the same criteria to decide whether a slot is invalidated
or not at all the places.
I am a fan of stricter, all-assumption-covering conditions. In case we
don't want to check restart_lsn, an Assert might be useful to validate
our assumption.

--
Best Wishes,
Ashutosh Bapat