On Mon, Nov 21, 2022 at 5:39 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > > 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.) >
Maybe. In that case pg_get_replication_slots() should be changed. We should use the same criteria to decide whether a slot is invalidated or not at all the places. 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. -- Best Wishes, Ashutosh Bapat