MIME-Version: 1.0
References: <514f6f2f-6833-4539-39f1-96cd1e011f23@enterprisedb.com>
 <18dbc929-a281-8552-4f1d-7e4d0e4eedba@enterprisedb.com> <646acd8d-9896-ed8d-2ab4-0bf11b3285d4@enterprisedb.com>
 <20220205195944.drwc3psrb5udt2cw@alap3.anarazel.de> <CAE9k0PnGmQ97ikLy2oBDoom72FsV_x+H9JuptJ4ysx=CFM-+kQ@mail.gmail.com>
 <20220207203222.22aktwxrt3fcllru@alap3.anarazel.de> <CAE9k0Pn80FMhkUZAPyPCLWDYnOcTmxKN3Q3TNNv1m4V99qG-Ew@mail.gmail.com>
 <YgWH4/4tK1AMIOYG@momjian.us> <ac40e8f8-46f5-7bb4-c08b-df3c9e6f1599@enterprisedb.com>
 <aa626ffb-2cb1-8683-5188-dad6031a4623@gmail.com> <b5cc7fd2-ebbc-512f-40fc-5c7ce93b8bdf@gmail.com>
 <aeab9f12-93a6-743d-4ac6-5f2c73f9bfea@gmail.com> <CAA4eK1LrZuDLGu6saZ8RDs7gKLM0dmafq+9oqRPKKN1cEEao_Q@mail.gmail.com>
 <CALj2ACW5z=QYNabSEW-iqJ0oZUyTNztPBXzCoAiDtWFWYMcuag@mail.gmail.com>
In-Reply-To: <CALj2ACW5z=QYNabSEW-iqJ0oZUyTNztPBXzCoAiDtWFWYMcuag@mail.gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Mon, 10 Jul 2023 09:06:22 +0530
Message-ID: <CAA4eK1KAC1TPZStUXjMi0gd-OCRTY04JgY3cyKUXyq3Ndt32CQ@mail.gmail.com>
Subject: Re: Synchronizing slots from primary to standby
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>, 
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Bruce Momjian <bruce@momjian.us>, 
	Ashutosh Sharma <ashu.coek88@gmail.com>, Andres Freund <andres@anarazel.de>, 
	pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1KAC1TPZStUXjMi0gd-OCRTY04JgY3cyKUXyq3Ndt32CQ%40mail.gmail.com>
Precedence: bulk

On Sun, Jul 9, 2023 at 1:01=E2=80=AFPM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Jun 16, 2023 at 3:26=E2=80=AFPM Amit Kapila <amit.kapila16@gmail.=
com> wrote:
> >
> >
> > 1. Can you please try to explain the functionality of the overall
> > patch somewhere in the form of comments and or commit message?
>
> IIUC, there are 2 core ideas of the feature:
>
> 1) It will never let the logical replication subscribers go ahead of
> physical replication standbys specified in standby_slot_names. It
> implements this by delaying decoding of commit records on the
> walsenders corresponding to logical replication subscribers on the
> primary until all the specified standbys confirm receiving the commit
> LSN.
>
> 2) The physical replication standbys will synchronize data of the
> specified logical replication slots (in synchronize_slot_names) from
> the primary, creating the logical replication slots if necessary.
> Since the logical replication subscribers will never go out of
> physical replication standbys, the standbys can safely synchronize the
> slots and keep the data necessary for subscribers to connect to it and
> work seamlessly even after a failover.
>
> If my understanding is right,
>

This matches my understanding as well.

> I have few thoughts here:
>
> 1. All the logical walsenders are delayed on the primary - per
> wait_for_standby_confirmation() despite the user being interested in
> only a few of them via synchronize_slot_names. Shouldn't the delay be
> for just the slots specified in synchronize_slot_names?
> 2. I think we can split the patch like this - 0001 can be the logical
> walsenders delaying decoding on the primary unless standbys confirm,
> 0002 standby synchronizing the logical slots.
>

Agreed with the above two points.

> 3. I think we need to change the GUC standby_slot_names to better
> reflect what it is used for - wait_for_replication_slot_names or
> wait_for_
>

I feel at this stage we can focus on getting the design and
implementation correct. We can improve GUC names later once we are
confident that the functionality is correct.

> 4. It allows specifying logical slots in standby_slot_names, meaning,
> it can disallow logical slots getting ahead of other logical slots
> specified in standby_slot_names. Should we allow this case with the
> thinking that if there's anyone using logical replication for failover
> (well, will anybody do that in production?).
>

I think on the contrary we should prohibit this case. We can always
extend this functionality later.

> 5. Similar to above, it allows specifying physical slots in
> synchronize_slot_names. Should we disallow?
>

We should prohibit that as well.

--=20
With Regards,
Amit Kapila.