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/