Re: Catalog_xmin is not advanced when a logical slot is lost - Mailing list pgsql-hackers

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/