MIME-Version: 1.0
References: <CA+-JvFtq6f7+wAwSdud-x0yMTeMejUhpkyid1Xa_VNpRd_-oPw@mail.gmail.com>
 <CAA4eK1+5H4wUCg3afO=Jc_mHCCwGCadH0SxxXR=SQAPCBGcRuA@mail.gmail.com> <CA+-JvFsn_FMKVcbztr2LiVKzwjz6uQTNKtakR47mXg+78Nt2iA@mail.gmail.com>
In-Reply-To: <CA+-JvFsn_FMKVcbztr2LiVKzwjz6uQTNKtakR47mXg+78Nt2iA@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Thu, 18 Jul 2024 09:25:37 +0530
Message-ID: <CAJpy0uCZ04ZQFHs-tV5LprkYtSSwtBtUJW4O=0S01yc+TRw7EQ@mail.gmail.com>
Subject: Re: Allow logical failover slots to wait on synchronous replication
To: John H <johnhyvr@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, 
	"bertranddrouvot.pg@gmail.com" <bertranddrouvot.pg@gmail.com>, pgsql-hackers@postgresql.org, 
	shveta malik <shveta.malik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAJpy0uCZ04ZQFHs-tV5LprkYtSSwtBtUJW4O%3D0S01yc%2BTRw7EQ%40mail.gmail.com>
Precedence: bulk

On Tue, Jul 9, 2024 at 12:42=E2=80=AFAM John H <johnhyvr@gmail.com> wrote:
>
>
> > Can we make it a default
> > behavior that logical slots marked with a failover option will wait
> > for 'synchronous_standby_names' as per your patch's idea unless
> > 'standby_slot_names' is specified? I don't know if there is any value
> > in setting the 'failover' option for a slot without specifying
> > 'standby_slot_names', so was wondering if we can additionally tie it
> > to 'synchronous_standby_names'. Any better ideas?
> >
>
> No, I think that works pretty cleanly actually. Reserving some special
> keyword isn't great
> which is the only other thing I can think of. I've updated the patch
> and tests to reflect that.
>
> Attached the patch that addresses these changes.

Thank You for the patch. I like the overall idea, it is a useful one
and will make user experience better. Please find few comments.

1)
I agree with the idea that instead of introducing new GUC, we can make
failover logical slots wait on 'synchronous_standby_names', but this
will leave user no option to unlink failover-enabled logical
subscribers from the wait on synchronous standbys. We provide user a
way to switch off automatic slot-sync by disabling
'sync_replication_slots' and we also provide user a way to manually
sync the slots using function 'pg_sync_replication_slots()' and
nowhere we make it mandatory to set 'synchronized_standby_slots'; in
fact in docs, it is a recommended setting and not a mandatory one.
User can always create failover slots, switch off automatic slot sync
and disable wait on standbys by not specifying
'synchronized_standby_slots' and do the slot-sync and consistency
checks manually when needed. I feel, for worst case scenarios, we
should provide user an option to delink failover-enabled logical
subscriptions from  'synchronous_standby_names'.

We can have 'synchronized_standby_slots' (standby_slot_names) to
accept one such option as in 'SAME_AS_SYNCREP_STANDBYS' or  say
'DEFAULT'. So when 'synchronous_standby_names' is comma separated
list, we pick those slots; if it is empty, then no wait on standbys,
and if its value is 'DEFAULT' as configured by user, then go with
'synchronous_standby_names'. Thoughts?


2)
When  'synchronized_standby_slots' is configured but standby named in
it is down blocking logical replication, then we get a WARNING in
subscriber's log file:

WARNING:  replication slot "standby_2" specified in parameter
synchronized_standby_slots does not have active_pid.
DETAIL:  Logical replication is waiting on the standby associated with
"standby_2".
HINT:  Consider starting standby associated with "standby_2" or amend
parameter synchronized_standby_slots.

But OTOH, when  'synchronous_standby_names' is configured instead of
'synchronized_standby_slots' and any of the standbys listed is down
blocking logical replication, we do not get any sort of warning. It is
inconsistent behavior. Also user might be left clueless on why
subscribers are not getting changes.

thanks
Shveta