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

On 2022-Nov-21, sirisha chamarthi wrote:

> > > 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.
> >
> > Agreed. I'll throw in an assert.
>
> Changed this in the patch to throw an assert.

Thank you. I had pushed mine for CirrusCI to test, and it failed the
assert I added in slot.c:
https://cirrus-ci.com/build/4786354503548928
Not yet sure why, looking into it.

You didn't add any asserts to the slot.c code.

In slotfuncs.c, I'm not sure I want to assert anything about restart_lsn
in any cases other than when invalidated_at is set. In other words, I
prefer this coding in pg_get_replication_slots:

if (!XLogRecPtrIsInvalid(slot_contents.data.invalidated_at))
{
Assert(XLogRecPtrIsInvalid(slot_contents.data.restart_lsn));
walstate = WALAVAIL_REMOVED;
}
else
walstate = GetWALAvailability(slot_contents.data.restart_lsn);

Your proposal is doing this:

switch (walstate)
{
[...]
case WALAVAIL_REMOVED:

if (!XLogRecPtrIsInvalid(slot_contents.data.restart_lsn))
{
[...]
if (pid != 0)
[...] break;
}
Assert(XLogRecPtrIsInvalid(slot_contents.data.restart_lsn));

which sounds like it could be hit if the replica is connected to the
slot.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/