Re: promotion related handling in pg_sync_replication_slots()

On Tue, Apr 16, 2024 at 1:55 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Tuesday, April 16, 2024 2:52 PM Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
>
> Hi,
>
> > On Tue, Apr 16, 2024 at 10:00:04AM +0530, shveta malik wrote:
> > > Please find v5 addressing above comments.
> >
> > Thanks!
> >
> > @@ -1634,9 +1677,14 @@ SyncReplicationSlots(WalReceiverConn *wrconn) {
> > PG_ENSURE_ERROR_CLEANUP(slotsync_failure_callback,
> > PointerGetDatum(wrconn));
> > {
> > + check_flags_and_set_sync_info(InvalidPid);
> > +
> >
> > Given the fact that if the sync worker is running it won't be possible to trigger a
> > manual sync with pg_sync_replication_slots(), what about also checking the
> > "sync_replication_slots" value at the start of SyncReplicationSlots() and emmit
> > an error if sync_replication_slots is set to on? (The message could explicitly
> > states that it's not possible to use the function if sync_replication_slots is set to
> > on).
>
> I personally feel adding the additional check for sync_replication_slots may
> not improve the situation here. Because the GUC sync_replication_slots can
> change at any point, the GUC could be false when performing this addition check
> and is set to true immediately after the check, so It could not simplify the logic
> anyway.

+1.
I feel doc and "cannot synchronize replication slots concurrently"
check should suffice.

In the scenario which Hou-San pointed out, if after performing the
GUC check in SQL function, this GUC is enabled immediately and say
worker is started sooner than the function could get chance to sync,
in that case as well, SQL function will ultimately get error "cannot
synchronize replication slots concurrently", even though GUC is
enabled. Thus, I feel we should stick with samer error in all
scenarios.

thanks
Shveta