Re: Standby server with cascade logical replication could not be properly stopped under load

On Wed, May 28, 2025 at 07:12:25PM +0300, Alexey Makhmutov wrote:
> Well, this is a great question, because it made me thinking whether we need
> to change behavior of THIS function at all. Extending
> 'GetStandbyFlushRecPtr' to properly support logical replication will just
> make it equal to the 'GetXLogReplayRecPtr'. So, basically we just need to
> replace invocation of 'GetStandbyFlushRecPtr' with 'GetXLogReplayRecPtr' and
> this should solve the problem.

Exactly. I don't want to assume that all the callers are OK with this
forced changed in the context of a logical WAL sender inside the
function GetStandbyFlushRecPtr(), especially since the change can be
isolated where it only makes sense, aka inside the logical sender
loop.

> This leaves us with only single function
> to consider: 'IdentifySystem'. This function could be called both in case of
> physical and logical replication, but I assume that we need to return
> flushed WAL position in it as well, as it's described in the documentation
> for this command. This also seems to be the right behavior, as primary goal
> for 'identify' command is to inform client about the potential replication
> start point and we definitely can start replication from the flushed
> position (as we already has this record), just with some delay (once our
> recovery get us to this point). But maybe somebody with more knowledge of
> the logical replication expectation could confirm this.

I don't think that there is any need to care about IDENTIFY_SYSTEM.
It would be a short-lived command. Note that we document that the LSN
returned should be the current flush LSN, that could be used as a
starting point for START_REPLICATION. START_REPLICATION has different
properties and assumptions; it's expected to last with its inner logic
required for the shutdown sequence order with WAL senders.

> The fix seems to be very simple in this case (assuming the logic above is
> correct): we only replace 'GetStandbyFlushRecPtr' call in 'XLogSendLogical'
> with the 'GetXLogReplayRecPtr' and that's it. Attached is the modified
> patch. It could be applied on top of 16, 17 and master versions.

Yeah, I think that this is a sensible move. Documenting the reason
why GetXLogReplayRecPtr() is called is important, but the comment you
are suggesting could be better, IMO, say:
"For cascading logical WAL senders, we use the replay LSN rather than
the flush LSN, as decoding would only happen with records already
replayed. This distinction is important especially during the
shutdown sequence, as cascading logical WAL senders can only catch up
with records that have been already replayed, not flushed."

That feels that I'm repeating myself a bit twice if formulated this
way. If you have a better suggestion, feel free..

> As for the test: do you want this case with shutdown to be added to the
> existing test plan for standby logical recovery? The condition to trigger
> the problem is just to have a flushed and replayed positions pointing to
> different values during standby shutdown. I could try to add it to the test
> suit, but it could take some time.

Anything I could think of for the moment would require the release of
an injection point, which is not something we can do during a shutdown
sequence with the existing facility as we cannot connect to the server
while it's in this late in its shutdown sequence: new connections are
prohibited once we expect the WAL senders to catch up. We could go
without that for now to get a fix in, your scripts are enough to
push all the logical WAL senders on the physical standby in an
infinite loop once its shutdown sequence initiates.

Bertrand and others, what do you think?
--
Michael