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.) -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/