Обсуждение: New standby_slot_names GUC in PG 17

The release notes have this item:

    Allow specification of physical standbys that must be synchronized
    before they are visible to subscribers (Hou Zhijie, Shveta Malik)

    The new server variable is standby_slot_names. 

Is standby_slot_names an accurate name for this GUC?  It seems too
generic.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Fri, Jun 21, 2024 at 11:37:54AM -0400, Bruce Momjian wrote:
> The release notes have this item:
> 
>     Allow specification of physical standbys that must be synchronized
>     before they are visible to subscribers (Hou Zhijie, Shveta Malik)
> 
>     The new server variable is standby_slot_names. 
> 
> Is standby_slot_names an accurate name for this GUC?  It seems too
> generic.

+1, I was considering bringing this up, too.  I'm still thinking of
alternate names to propose, though.

-- 
nathan

Muhammad Ikram <mmikram@gmail.com> writes:
> A humble input, as on primary we have #primary_slot_name = ''  then should
> not it be okay to have standby_slot_names or standby_slot_name ? It seems
> consistent with the Guc on primary.
> Another suggestion is *standby_replication_slots*.

IIUC, Bruce's complaint is that the name is too generic (which I agree
with).  Given the stated functionality:

>>>> Allow specification of physical standbys that must be synchronized
>>>> before they are visible to subscribers (Hou Zhijie, Shveta Malik)

it seems like the name ought to have some connection to
synchronization.  Perhaps something like "synchronized_standby_slots"?

I haven't read the patch, so I don't know if this name is especially
on-point.  But "standby_slot_names" seems completely unhelpful, as
a server could well have slots that are for standbys but are not to
be included in this list.

            regards, tom lane

On Fri, Jun 21, 2024 at 03:50:00PM -0400, Tom Lane wrote:
>>>>> Allow specification of physical standbys that must be synchronized
>>>>> before they are visible to subscribers (Hou Zhijie, Shveta Malik)
> 
> it seems like the name ought to have some connection to
> synchronization.  Perhaps something like "synchronized_standby_slots"?

IMHO that might be a bit too close to synchronous_standby_names.  But the
name might not be the only issue, as there is a separate proposal [0] to
add _another_ GUC to tie standby_slot_names to synchronous replication.  I
wonder if this could just be a Boolean parameter or if folks really have
use-cases for both a list of synchronous standbys and a separate list of
synchronous standbys for failover slots.

[0] https://postgr.es/m/CA%2B-JvFtq6f7%2BwAwSdud-x0yMTeMejUhpkyid1Xa_VNpRd_-oPw%40mail.gmail.com

-- 
nathan

On Sat, Jun 22, 2024 at 1:49 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Fri, Jun 21, 2024 at 03:50:00PM -0400, Tom Lane wrote:
> >>>>> Allow specification of physical standbys that must be synchronized
> >>>>> before they are visible to subscribers (Hou Zhijie, Shveta Malik)
> >
> > it seems like the name ought to have some connection to
> > synchronization.  Perhaps something like "synchronized_standby_slots"?
>
> IMHO that might be a bit too close to synchronous_standby_names.  But the
> name might not be the only issue, as there is a separate proposal [0] to
> add _another_ GUC to tie standby_slot_names to synchronous replication.  I
> wonder if this could just be a Boolean parameter or if folks really have
> use-cases for both a list of synchronous standbys and a separate list of
> synchronous standbys for failover slots.
>

Both have separate functionalities. We need to wait for the standby's
in synchronous_standby_names to be synced at the commit time whereas
the standby's in the standby_slot_names doesn't have such a
requirement. The standby's in the standby_slot_names are used by
logical WAL senders such that they will send decoded changes to
plugins only after the specified replication slots confirm receiving
WAL. So, combining them doesn't sound advisable.

--
With Regards,
Amit Kapila.

On Sat, Jun 22, 2024 at 1:49 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Fri, Jun 21, 2024 at 03:50:00PM -0400, Tom Lane wrote:
> >>>>> Allow specification of physical standbys that must be synchronized
> >>>>> before they are visible to subscribers (Hou Zhijie, Shveta Malik)
> >
> > it seems like the name ought to have some connection to
> > synchronization.  Perhaps something like "synchronized_standby_slots"?
>
> IMHO that might be a bit too close to synchronous_standby_names.
>

Right, but better than the current one. The other possibility could be
wait_for_standby_slots.

--
With Regards,
Amit Kapila.

On Sat, Jun 22, 2024 at 03:17:03PM +0530, Amit Kapila wrote:
> On Sat, Jun 22, 2024 at 1:49 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
> >
> > On Fri, Jun 21, 2024 at 03:50:00PM -0400, Tom Lane wrote:
> > >>>>> Allow specification of physical standbys that must be synchronized
> > >>>>> before they are visible to subscribers (Hou Zhijie, Shveta Malik)
> > >
> > > it seems like the name ought to have some connection to
> > > synchronization.  Perhaps something like "synchronized_standby_slots"?
> >
> > IMHO that might be a bit too close to synchronous_standby_names.
> >
> 
> Right, but better than the current one. The other possibility could be
> wait_for_standby_slots.

FYI, changing this GUC name could force an initdb because
postgresql.conf would have the old name and removing the comment to
change it would cause an error.  Therefore, we should change it ASAP.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

Bruce Momjian <bruce@momjian.us> writes:
> FYI, changing this GUC name could force an initdb because
> postgresql.conf would have the old name and removing the comment to
> change it would cause an error.  Therefore, we should change it ASAP.

That's not reason for a forced initdb IMO.  It's easily fixed by
hand.

At this point we're into the release freeze for beta2, so even
if we had consensus on a new name it should wait till after.
So I see no particular urgency to make a decision.

            regards, tom lane

On Saturday, June 22, 2024 5:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Sat, Jun 22, 2024 at 1:49 AM Nathan Bossart
> <nathandbossart@gmail.com> wrote:
> >
> > On Fri, Jun 21, 2024 at 03:50:00PM -0400, Tom Lane wrote:
> > >>>>> Allow specification of physical standbys that must be
> > >>>>> synchronized before they are visible to subscribers (Hou Zhijie,
> > >>>>> Shveta Malik)
> > >
> > > it seems like the name ought to have some connection to
> > > synchronization.  Perhaps something like "synchronized_standby_slots"?
> >
> > IMHO that might be a bit too close to synchronous_standby_names.
> >
> 
> Right, but better than the current one. The other possibility could be
> wait_for_standby_slots.

I agree the current name seems too generic and the suggested ' synchronized_standby_slots '
is better than the current one.

Some other ideas could be:

synchronize_slots_on_standbys: it indicates that the standbys that enabled
slot sync should be listed in this GUC.

logical_replication_wait_slots: it means the logical replication(logical
Walsender process) will wait for these slots to advance the confirm flush
lsn before proceeding.

Best Regards,
Hou zj

On Tue, Jun 25, 2024 at 11:21 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Saturday, June 22, 2024 5:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sat, Jun 22, 2024 at 1:49 AM Nathan Bossart
> > <nathandbossart@gmail.com> wrote:
> > >
> > > On Fri, Jun 21, 2024 at 03:50:00PM -0400, Tom Lane wrote:
> > > >>>>> Allow specification of physical standbys that must be
> > > >>>>> synchronized before they are visible to subscribers (Hou Zhijie,
> > > >>>>> Shveta Malik)
> > > >
> > > > it seems like the name ought to have some connection to
> > > > synchronization.  Perhaps something like "synchronized_standby_slots"?
> > >
> > > IMHO that might be a bit too close to synchronous_standby_names.
> > >
> >
> > Right, but better than the current one. The other possibility could be
> > wait_for_standby_slots.
>
> I agree the current name seems too generic and the suggested ' synchronized_standby_slots '
> is better than the current one.
>
> Some other ideas could be:
>
> synchronize_slots_on_standbys: it indicates that the standbys that enabled
> slot sync should be listed in this GUC.
>
> logical_replication_wait_slots: it means the logical replication(logical
> Walsender process) will wait for these slots to advance the confirm flush
> lsn before proceeding.

I feel that the name that has some connection to "logical replication"
also sounds good. Let me add some ideas:

- logical_replication_synchronous_standby_slots (might be too long)
- logical_replication_synchronous_slots

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue, Jun 25, 2024 at 8:20 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Jun 25, 2024 at 11:21 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > I agree the current name seems too generic and the suggested ' synchronized_standby_slots '
> > is better than the current one.
> >
> > Some other ideas could be:
> >
> > synchronize_slots_on_standbys: it indicates that the standbys that enabled
> > slot sync should be listed in this GUC.
> >
> > logical_replication_wait_slots: it means the logical replication(logical
> > Walsender process) will wait for these slots to advance the confirm flush
> > lsn before proceeding.
>
> I feel that the name that has some connection to "logical replication"
> also sounds good. Let me add some ideas:
>
> - logical_replication_synchronous_standby_slots (might be too long)
> - logical_replication_synchronous_slots
>

I see your point about keeping logical_replication in the name but
that could also lead one to think that this list can contain logical
slots. OTOH, there is some value in keeping '_standby_' in the name as
that is more closely associated with physical standby's and this list
contains physical slots corresponding to physical standby's. So, my
preference is in order as follows: synchronized_standby_slots,
wait_for_standby_slots, logical_replication_wait_slots,
logical_replication_synchronous_slots, and
logical_replication_synchronous_standby_slots.

--
With Regards,
Amit Kapila.

Hi,

On Tue, Jun 25, 2024 at 10:24:41AM +0530, Amit Kapila wrote:
> On Tue, Jun 25, 2024 at 8:20 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Jun 25, 2024 at 11:21 AM Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > I agree the current name seems too generic and the suggested ' synchronized_standby_slots '
> > > is better than the current one.
> > >
> > > Some other ideas could be:
> > >
> > > synchronize_slots_on_standbys: it indicates that the standbys that enabled
> > > slot sync should be listed in this GUC.
> > >
> > > logical_replication_wait_slots: it means the logical replication(logical
> > > Walsender process) will wait for these slots to advance the confirm flush
> > > lsn before proceeding.
> >
> > I feel that the name that has some connection to "logical replication"
> > also sounds good. Let me add some ideas:
> >
> > - logical_replication_synchronous_standby_slots (might be too long)
> > - logical_replication_synchronous_slots
> >
> 
> I see your point about keeping logical_replication in the name but
> that could also lead one to think that this list can contain logical
> slots.

Agree, and we may add the same functionality for physical replication slots
in the future too (it has been discussed in the thread [1]). So I don't think
"logical" should be part of the name.

> OTOH, there is some value in keeping '_standby_' in the name as
> that is more closely associated with physical standby's and this list
> contains physical slots corresponding to physical standby's. So, my
> preference is in order as follows: synchronized_standby_slots,
> wait_for_standby_slots, logical_replication_wait_slots,
> logical_replication_synchronous_slots, and
> logical_replication_synchronous_standby_slots.
> 

I like the idea of having "synchronize[d]" in the name as it makes think of 
the feature it is linked to [2]. The slots mentioned in this parameter are
linked to the "primary_slot_name" parameter on the standby, so what about?

synchronized_primary_slot_names 

It makes clear it is somehow linked to "primary_slot_name" and that we want them
to be in sync.

So I'd vote for (in that order);

synchronized_primary_slot_names, synchronized_standby_slots

[1]: https://www.postgresql.org/message-id/bb437218-73bc-34c3-b8fb-8c1be4ddaec9%40enterprisedb.com
[2]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=93db6cbda037f1be9544932bd9a785dabf3ff712

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Tue, Jun 25, 2024 at 1:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jun 25, 2024 at 8:20 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Jun 25, 2024 at 11:21 AM Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > I agree the current name seems too generic and the suggested ' synchronized_standby_slots '
> > > is better than the current one.
> > >
> > > Some other ideas could be:
> > >
> > > synchronize_slots_on_standbys: it indicates that the standbys that enabled
> > > slot sync should be listed in this GUC.
> > >
> > > logical_replication_wait_slots: it means the logical replication(logical
> > > Walsender process) will wait for these slots to advance the confirm flush
> > > lsn before proceeding.
> >
> > I feel that the name that has some connection to "logical replication"
> > also sounds good. Let me add some ideas:
> >
> > - logical_replication_synchronous_standby_slots (might be too long)
> > - logical_replication_synchronous_slots
> >
>
> I see your point about keeping logical_replication in the name but
> that could also lead one to think that this list can contain logical
> slots.

Right.

>  OTOH, there is some value in keeping '_standby_' in the name as
> that is more closely associated with physical standby's and this list
> contains physical slots corresponding to physical standby's.

Agreed.

> So, my
> preference is in order as follows: synchronized_standby_slots,
> wait_for_standby_slots, logical_replication_wait_slots,
> logical_replication_synchronous_slots, and
> logical_replication_synchronous_standby_slots.

I also prefer synchronized_standby_slots.

From a different angle just for discussion, is it worth considering
the term 'failover' since the purpose of this feature is to ensure a
standby to be ready for failover in terms of logical replication? For
example, failover_standby_slot_names?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue, Jun 25, 2024 at 12:30 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Jun 25, 2024 at 1:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
>
> > So, my
> > preference is in order as follows: synchronized_standby_slots,
> > wait_for_standby_slots, logical_replication_wait_slots,
> > logical_replication_synchronous_slots, and
> > logical_replication_synchronous_standby_slots.
>
> I also prefer synchronized_standby_slots.
>
> From a different angle just for discussion, is it worth considering
> the term 'failover' since the purpose of this feature is to ensure a
> standby to be ready for failover in terms of logical replication? For
> example, failover_standby_slot_names?
>

I feel synchronized better indicates the purpose because we ensure
such slots are synchronized before we process changes for logical
failover slots. We already have a 'failover' option for logical slots
which could make things confusing if we add 'failover' where physical
slots need to be specified.

--
With Regards,
Amit Kapila.

On Tue, Jun 25, 2024 at 02:02:09PM +0530, Amit Kapila wrote:
> On Tue, Jun 25, 2024 at 12:30 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> On Tue, Jun 25, 2024 at 1:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> > So, my
>> > preference is in order as follows: synchronized_standby_slots,
>> > wait_for_standby_slots, logical_replication_wait_slots,
>> > logical_replication_synchronous_slots, and
>> > logical_replication_synchronous_standby_slots.
>>
>> I also prefer synchronized_standby_slots.
>>
>> From a different angle just for discussion, is it worth considering
>> the term 'failover' since the purpose of this feature is to ensure a
>> standby to be ready for failover in terms of logical replication? For
>> example, failover_standby_slot_names?
> 
> I feel synchronized better indicates the purpose because we ensure
> such slots are synchronized before we process changes for logical
> failover slots. We already have a 'failover' option for logical slots
> which could make things confusing if we add 'failover' where physical
> slots need to be specified.

I'm fine with synchronized_standby_slots.

-- 
nathan

On Tue, Jun 25, 2024 at 5:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jun 25, 2024 at 12:30 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Jun 25, 2024 at 1:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> >
> > > So, my
> > > preference is in order as follows: synchronized_standby_slots,
> > > wait_for_standby_slots, logical_replication_wait_slots,
> > > logical_replication_synchronous_slots, and
> > > logical_replication_synchronous_standby_slots.
> >
> > I also prefer synchronized_standby_slots.
> >
> > From a different angle just for discussion, is it worth considering
> > the term 'failover' since the purpose of this feature is to ensure a
> > standby to be ready for failover in terms of logical replication? For
> > example, failover_standby_slot_names?
> >
>
> I feel synchronized better indicates the purpose because we ensure
> such slots are synchronized before we process changes for logical
> failover slots. We already have a 'failover' option for logical slots
> which could make things confusing if we add 'failover' where physical
> slots need to be specified.

Agreed. So +1 for synchronized_stnadby_slots.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Wednesday, June 26, 2024 9:40 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> 
> On Tue, Jun 25, 2024 at 5:32 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Tue, Jun 25, 2024 at 12:30 PM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> > >
> > > On Tue, Jun 25, 2024 at 1:54 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > > >
> > >
> > > > So, my
> > > > preference is in order as follows: synchronized_standby_slots,
> > > > wait_for_standby_slots, logical_replication_wait_slots,
> > > > logical_replication_synchronous_slots, and
> > > > logical_replication_synchronous_standby_slots.
> > >
> > > I also prefer synchronized_standby_slots.
> > >
> > > From a different angle just for discussion, is it worth considering
> > > the term 'failover' since the purpose of this feature is to ensure a
> > > standby to be ready for failover in terms of logical replication?
> > > For example, failover_standby_slot_names?
> > >
> >
> > I feel synchronized better indicates the purpose because we ensure
> > such slots are synchronized before we process changes for logical
> > failover slots. We already have a 'failover' option for logical slots
> > which could make things confusing if we add 'failover' where physical
> > slots need to be specified.
> 
> Agreed. So +1 for synchronized_stnadby_slots.

+1.

Since there is a consensus on this name, I am attaching the patch to rename
the GUC to synchronized_stnadby_slots. I have confirmed that the regression
tests and pgindent passed for the patch.

Best Regards,
Hou zj

Best Regards,
Hou zj

Hi,

On Wed, Jun 26, 2024 at 04:17:45AM +0000, Zhijie Hou (Fujitsu) wrote:
> On Wednesday, June 26, 2024 9:40 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > 
> > On Tue, Jun 25, 2024 at 5:32 PM Amit Kapila <amit.kapila16@gmail.com>
> > wrote:
> > >
> > > I feel synchronized better indicates the purpose because we ensure
> > > such slots are synchronized before we process changes for logical
> > > failover slots. We already have a 'failover' option for logical slots
> > > which could make things confusing if we add 'failover' where physical
> > > slots need to be specified.
> > 
> > Agreed. So +1 for synchronized_stnadby_slots.
> 
> +1.
> 
> Since there is a consensus on this name, I am attaching the patch to rename
> the GUC to synchronized_stnadby_slots. I have confirmed that the regression
> tests and pgindent passed for the patch.
> 

Thanks for the patch!

A few comments:

1 ====

In the commit message:

"
The standby_slot_names GUC is intended to allow specification of physical
    standby slots that must be synchronized before they are visible to
    subscribers
"

Not sure that wording is correct, if we feel the need to explain the GUC,
maybe repeat some wording from bf279ddd1c?

2 ====

Should we rename StandbySlotNamesConfigData too?

3 ====

Should we rename SlotExistsInStandbySlotNames too?

4 ====

Should we rename validate_standby_slots() too?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Jun 26, 2024 at 10:19 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
>
> 2 ====
>
> Should we rename StandbySlotNamesConfigData too?
>

How about SyncStandbySlotsConfigData?

> 3 ====
>
> Should we rename SlotExistsInStandbySlotNames too?
>

Similarly SlotExistsInSyncStandbySlots?

> 4 ====
>
> Should we rename validate_standby_slots() too?
>

And validate_sync_standby_slots()?

--- a/doc/src/sgml/release-17.sgml
+++ b/doc/src/sgml/release-17.sgml
@@ -1325,7 +1325,7 @@ Author: Michael Paquier <michael@paquier.xyz>

 <!--
 Author: Amit Kapila <akapila@postgresql.org>
-2024-03-08 [bf279ddd1] Introduce a new GUC 'standby_slot_names'.
+2024-03-08 [bf279ddd1] Introduce a new GUC 'synchronized_standby_slots'.

I am not sure if it is a good idea to change release notes in the same
commit as the code change. I would prefer to do it in a separate
commit.

--
With Regards,
Amit Kapila.

On Wed, Jun 26, 2024 at 11:39:45AM +0530, Amit Kapila wrote:
> --- a/doc/src/sgml/release-17.sgml
> +++ b/doc/src/sgml/release-17.sgml
> @@ -1325,7 +1325,7 @@ Author: Michael Paquier <michael@paquier.xyz>
>
>  <!--
>  Author: Amit Kapila <akapila@postgresql.org>
> -2024-03-08 [bf279ddd1] Introduce a new GUC 'standby_slot_names'.
> +2024-03-08 [bf279ddd1] Introduce a new GUC 'synchronized_standby_slots'.
>
> I am not sure if it is a good idea to change release notes in the same
> commit as the code change. I would prefer to do it in a separate
> commit.

The existing commits referenced cannot change, but it's surely OK to
add a reference to the commit doing the rename for this item in the
release notes, and update the release notes to reflect the new GUC
name.  Using two separate commits ensures that the correct reference
about the rename is added to the release notes, so that's the correct
thing to do, IMHO.
--
Michael

Hi,

On Wed, Jun 26, 2024 at 11:39:45AM +0530, Amit Kapila wrote:
> On Wed, Jun 26, 2024 at 10:19 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> >
> > 2 ====
> >
> > Should we rename StandbySlotNamesConfigData too?
> >
> 
> How about SyncStandbySlotsConfigData?
> 
> > 3 ====
> >
> > Should we rename SlotExistsInStandbySlotNames too?
> >
> 
> Similarly SlotExistsInSyncStandbySlots?
> 
> > 4 ====
> >
> > Should we rename validate_standby_slots() too?
> >
> 
> And validate_sync_standby_slots()?
> 

Thanks!

All of the above proposal sound good to me.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wednesday, June 26, 2024 12:49 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> 
> Hi,
> 
> On Wed, Jun 26, 2024 at 04:17:45AM +0000, Zhijie Hou (Fujitsu) wrote:
> > On Wednesday, June 26, 2024 9:40 AM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> > >
> > > On Tue, Jun 25, 2024 at 5:32 PM Amit Kapila
> > > <amit.kapila16@gmail.com>
> > > wrote:
> > > >
> > > > I feel synchronized better indicates the purpose because we ensure
> > > > such slots are synchronized before we process changes for logical
> > > > failover slots. We already have a 'failover' option for logical
> > > > slots which could make things confusing if we add 'failover' where
> > > > physical slots need to be specified.
> > >
> > > Agreed. So +1 for synchronized_stnadby_slots.
> >
> > +1.
> >
> > Since there is a consensus on this name, I am attaching the patch to
> > rename the GUC to synchronized_stnadby_slots. I have confirmed that
> > the regression tests and pgindent passed for the patch.
> A few comments:

Thanks for the comments!

> 1 ====
> 
> In the commit message:
> 
> "
> The standby_slot_names GUC is intended to allow specification of physical
>     standby slots that must be synchronized before they are visible to
>     subscribers
> "
> 
> Not sure that wording is correct, if we feel the need to explain the GUC, maybe
> repeat some wording from bf279ddd1c?

I intentionally copied some words from release note of this GUC which was
also part of the content in the initial email of this thread. I think it
would be easy to understand than the original commit msg. But others may
have different opinion, so I would leave the decision to the committer. (I adjusted
a bit the word in this version).

> 
> 2 ====
> 
> Should we rename StandbySlotNamesConfigData too?
> 
> 3 ====
> 
> Should we rename SlotExistsInStandbySlotNames too?
> 
> 4 ====
> 
> Should we rename validate_standby_slots() too?
> 

Renamed these to the names suggested by Amit.

Attach the v2 patch set which addressed above and removed
the changes in release-17.sgml according to the comment from Amit.

Best Regards,
Hou zj

Hi,

On Wed, Jun 26, 2024 at 09:15:48AM +0000, Zhijie Hou (Fujitsu) wrote:
> Renamed these to the names suggested by Amit.
> 
> Attach the v2 patch set which addressed above and removed
> the changes in release-17.sgml according to the comment from Amit.
> 

Thanks! LGTM.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Jun 26, 2024 at 6:00 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> On Wed, Jun 26, 2024 at 09:15:48AM +0000, Zhijie Hou (Fujitsu) wrote:
> > Renamed these to the names suggested by Amit.
> >
> > Attach the v2 patch set which addressed above and removed
> > the changes in release-17.sgml according to the comment from Amit.
> >
>
> Thanks! LGTM.
>

As per my reading of this thread, we have an agreement on changing the
GUC name standby_slot_names to synchronized_standby_slots. I'll wait
for a day and push the change unless someone thinks otherwise.

--
With Regards,
Amit Kapila.

On 21.06.24 17:37, Bruce Momjian wrote:
> The release notes have this item:
> 
>     Allow specification of physical standbys that must be synchronized
>     before they are visible to subscribers (Hou Zhijie, Shveta Malik)
> 
>     The new server variable is standby_slot_names.
> 
> Is standby_slot_names an accurate name for this GUC?  It seems too
> generic.

This was possibly inspired by pg_failover_slots.standby_slot_names 
(which in turn came from pglogical.standby_slot_names).  In those cases, 
you have some more context from the extension prefix.

The new suggested names sound good to me.

On Wed, Jun 26, 2024 at 6:15 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Wednesday, June 26, 2024 12:49 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> >
> > Hi,
> >
> > On Wed, Jun 26, 2024 at 04:17:45AM +0000, Zhijie Hou (Fujitsu) wrote:
> > > On Wednesday, June 26, 2024 9:40 AM Masahiko Sawada
> > <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Tue, Jun 25, 2024 at 5:32 PM Amit Kapila
> > > > <amit.kapila16@gmail.com>
> > > > wrote:
> > > > >
> > > > > I feel synchronized better indicates the purpose because we ensure
> > > > > such slots are synchronized before we process changes for logical
> > > > > failover slots. We already have a 'failover' option for logical
> > > > > slots which could make things confusing if we add 'failover' where
> > > > > physical slots need to be specified.
> > > >
> > > > Agreed. So +1 for synchronized_stnadby_slots.
> > >
> > > +1.
> > >
> > > Since there is a consensus on this name, I am attaching the patch to
> > > rename the GUC to synchronized_stnadby_slots. I have confirmed that
> > > the regression tests and pgindent passed for the patch.
> > A few comments:
>
> Thanks for the comments!
>
> > 1 ====
> >
> > In the commit message:
> >
> > "
> > The standby_slot_names GUC is intended to allow specification of physical
> >     standby slots that must be synchronized before they are visible to
> >     subscribers
> > "
> >
> > Not sure that wording is correct, if we feel the need to explain the GUC, maybe
> > repeat some wording from bf279ddd1c?
>
> I intentionally copied some words from release note of this GUC which was
> also part of the content in the initial email of this thread. I think it
> would be easy to understand than the original commit msg. But others may
> have different opinion, so I would leave the decision to the committer. (I adjusted
> a bit the word in this version).
>
> >
> > 2 ====
> >
> > Should we rename StandbySlotNamesConfigData too?
> >
> > 3 ====
> >
> > Should we rename SlotExistsInStandbySlotNames too?
> >
> > 4 ====
> >
> > Should we rename validate_standby_slots() too?
> >
>
> Renamed these to the names suggested by Amit.
>
> Attach the v2 patch set which addressed above and removed
> the changes in release-17.sgml according to the comment from Amit.
>

Thank you for updating the patch. The v2 patch looks good to me.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com