Dear Hou,

Thanks for updating the patch. Further comments.

01.
```
+#include "access/xlog.h"
```

I could build without the inclusion because "replication/logical.h" already
includes it. Can we remove or we should retain?

02.
Should we increase checkpoint_timeout for stabilizing tests?

03.
To confirm, you've removed the logic that checks the oldest segment and try
reserving, but it can be removed same as ReplicationSlotReserveWal(), right?
XLogGetOldestSegno() is also not needed anymore because race can't happen if 
standby have never discarded.

04.
```
$primary->psql('postgres',
        q{SELECT pg_create_logical_replication_slot('failover_slot', 
'test_decoding', false, false, true);
         SELECT pg_create_physical_replication_slot('phys_slot');}
);
...
$primary->psql('postgres', "CHECKPOINT");
```

I found two lines use `psql()`, but should be `safe_psql()`.

05.
Per my tests, the issue exists till PG17 and your patch can be backpatched till
it, right?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Reply via email to