Hi Fujii-san,
Thanks for the review.
Attached v3. Only the test changed.
> How about configuring sync replication at startup instead?
Done. synchronous_standby_names = '*' and synchronous_commit = local
are now set in the primary's initial configuration, so there is no reload and
no window where the standby appears synchronous before the backends are
aware of it. Only the test session uses remote_apply.
> Also, do we really need to pause replay?
No, it is not needed. Thank you for pointing that out. The test now runs the
INSERT with query_safe() and then checks that the row is visible on the standby.
Without the fix the INSERT remains blocked, and the test fails on timeout.
I ran it a few times with the fix and it passes. With the walreceiver.c change
reverted, it fails on a timeout.
Thanks again for your time and for the review.
Regards,
Vaibhav
On Tue, Oct 6, 2026 at 9:31 PM Vaibhav Dalvi
<vaibhav.dalvi@enterprisedb.com> wrote:
> The walreceiver.c change is the same as in v1. Only the test changed.
> The v2 patch is attached.
Thanks for the patch! The walreceiver change looks correct to me. I have a
couple of comments on the test.
+$primary->safe_psql('postgres',
+ "ALTER SYSTEM SET synchronous_standby_names = '*'");
+$primary->reload;
+$primary->poll_query_until(
+ 'postgres',
+ "SELECT count(*) = 1 FROM pg_stat_replication
+ WHERE state = 'streaming' AND sync_state = 'sync'
+ AND flush_lsn IS NOT NULL"
There seems to be another race in the test. After the configuration reload,
the checkpointer needs to read the new synchronous_standby_names setting
and update the shared SYNC_STANDBY_DEFINED flag before backends can wait
for sync replication. But, walsender could read the new setting first,
causing sync_state to show sync before the checkpointer updates that flag.
If the INSERT commits during this window, it could skip the SyncRep wait
and cause the test to fail.
How about configuring sync replication at startup instead?
$primary->append_conf(
'postgresql.conf', qq(
wal_sender_timeout = 0
synchronous_standby_names = '*'
synchronous_commit = local
));
Using local allows setup to proceed before the standby is available. Only
the test session needs to switch to remote_apply:
$bg->query_safe('SET synchronous_commit = remote_apply');
Also, do we really need to pause replay? Isn't it sufficient to wait
directly for the INSERT to complete using query_safe() in a background psql
session with a timeout, and then check that the row is visible on the standby?
Without the fix, the INSERT would remain blocked and the test would fail on
timeout. With the fix, it would complete and the test would pass. This would
simplify the test.
Regards,
--
Fujii Masao
