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/