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/
