Re: [PATCH] Fix PITR pause bypass when initial XLOG_RUNNING_XACTS has subxid overflow

Hi Fujii-san,

Thanks for reviewing!

On 29.10.26 03:41, Fujii Masao wrote:

> + if (!LocalHotStandbyActive && !LocalPromoteIsTriggered)
>
> Do we need to use !CheckForStandbyTrigger() instead of !LocalPromoteIsTriggered?
> This would also handle the case where a promotion request has arrived but
> LocalPromoteIsTriggered has not yet been set.

Right, I missed that case. Changed to CheckForStandbyTrigger() as suggested.

> + errdetail("The standby snapshots stayed overflowed and the
> transactions responsible did not finish, so hot standby never
> started."),
> + errhint("Set \"recovery_target_action\" to \"promote\", or choose a
> recovery target at which hot standby can be enabled.")));
>
> Are overflowed standby snapshots the only cause of this situation?
> If not, mentioning them in the detail message could be misleading.

Good point. It can also happen, for example, in single-user mode,
where hot standby is never enabled. So I removed the errdetail; the
message and hint hold for all cases.

> We should clarify that this applies only when recovery has already reached
> a consistent state? If the recovery target is reached before that,
> the preceding check raises FATAL regardless of recovery_target_action.
> As currently worded, this could be read as covering that case too.
>
> For example, how about the following description?
>
> Likewise, if recovery has reached a consistent state but hot standby
> has not yet become active when the recovery target is reached,
> a setting of pause will act the same as shutdown. This can happen,
> for example, when the replayed standby snapshots remain overflowed.

Yes, that makes it clearer. I have adjusted the documentation as proposed.

> +# to pause and shut down instead of promoting. The server stops on its own, so
> +# we drive it with pg_ctl directly (a regular ->start() would error out because
> +# the server never becomes ready to accept connections).
> +run_log(
> + [
> + 'pg_ctl',
> + '--pgdata' => $node_standby->data_dir,
> + '--log' => $node_standby->logfile,
> + 'start',
> + ]);
>
> How about using $node_standby->start(fail_ok => 1) instead of invoking pg_ctl
> directly? This would allow the expected startup failure while tracking
> the postmaster PID, so teardown_node() could stop the server
> if it unexpectedly promotes. If we do this, the comment needs to be updated
> accordingly.

Unfortunately, I couldn't get it to work with
$node_standby->start(fail_ok => 1). When the server shuts down at the
recovery target, pg_ctl start reports "server shut down because of
recovery target settings" and exits with 0, so Cluster::start() takes
the success path, finds no postmaster.pid and BAIL_OUTs.

You're right that if a standby unexpectedly promotes, it would keep
running because Cluster.pm doesn't track its PID. So I kept starting it
with pg_ctl directly, updated the comment, and now I also stop it with
pg_ctl at the end of the test.

> +my $until_lsn =
> + $node_primary->safe_psql('postgres', 'SELECT pg_current_wal_lsn()');
>
> We can simplify this to "my $until_lsn = $node_primary->lsn('write');".

Good point, changed as suggested.

Version 6 of the patch with all of these changes is attached.

Thanks again for the careful review.

Best regards
Jan

--
Jan Nidzwetzki
PlanetScale Postgres Core Team