On Tue, Oct 28, 2025 at 11:53:26AM -0700, Masahiko Sawada wrote:
> On Tue, Oct 28, 2025 at 1:58 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
Apologies for not following closely what has been happening on this
thread. I have just not been paying much attention, and Sawada-san
has just poked at me about its existence today, also regarding the
fact that my fingerprints are all over the place (implying that I'm an
owner here, even in light of 105b2cb3361 that has introduced the trick
to bypass the creation of the annoying standby snapshot records that
distabilized the tests).
>> I see, you mean that the tests are stable now (thanks to 105b2cb3361) but
>> that we should still do something for "production" cases? (i.e not making use
>> of injection points).
>
> Yes. While it seems we might want to review the past discussion, if
> we've concluded such behavior is problematic behavior and could
> confuse users, we can do something like improving the
> invalidation/termination reports. Or we can do nothing if the current
> reporting is fine.
What do you mean exactly here? An improvement in the reports done
when the invalidations are kicked sounds separate to me, that may not
be something to touch when it comes to the back-branches.
Anyway, coming back to the patch, the argument of relying on the
slot's restart_lsn and the two xmins to restore the pre-818fefd8fd4
sounds good to me in light of 105b2cb3361.
Extra argument: Is the fact that RS_INVAL_WAL_REMOVED (and optionally
RS_INVAL_IDLE_TIMEOUT) is used only by the checkpointer something that
we may want to document in the shape of an assertion at the beginning
of InvalidateObsoleteReplicationSlots() with an extra condition based
on the backend type? Based on the fact that restart_lsn could be
updated across two loops of the invalidation logic done by the
checkpointer, the answer to my own question is "no" and we may want to
trigger this reason from other contexts. Just wondering if that's
worth doing, as it has been mentioned upthread.
--
Michael
