Thread: Re: Separate GUC for replication origins

On 10.12.24 19:41, Euler Taveira wrote:
> I'm attaching a patch that adds max_replication_origins. It basically 
> replaces
> all of the points that refers to max_replication_slots on the subscriber. It
> uses the same default value as max_replication_slots (10). I did nothing to
> keep the backward compatibility. I mean has a special value (like -1) that
> means same value as max_replication_slots. Is it worth it? (If not, it 
> should
> be mentioned in the commit message.)

I think some backward compatibility tweak like that would be useful. 
Otherwise, the net effect of this is that most people will have to do 
one more thing than before to keep their systems working.  Also, all 
deployment and automation scripts would have to be updated for this.

On Wed, Jan 8, 2025 at 7:39 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Dec 19, 2024, at 10:31 AM, Peter Eisentraut wrote:
>
> On 10.12.24 19:41, Euler Taveira wrote:
> > I'm attaching a patch that adds max_replication_origins. It basically
> > replaces
> > all of the points that refers to max_replication_slots on the subscriber. It
> > uses the same default value as max_replication_slots (10). I did nothing to
> > keep the backward compatibility. I mean has a special value (like -1) that
> > means same value as max_replication_slots. Is it worth it? (If not, it
> > should
> > be mentioned in the commit message.)
>
> I think some backward compatibility tweak like that would be useful.
> Otherwise, the net effect of this is that most people will have to do
> one more thing than before to keep their systems working.  Also, all
> deployment and automation scripts would have to be updated for this.
>
> This new patch includes the special value (-1) that uses max_replication_slots
> instead.

Thank you for working on this. The proposed idea makes sense to me.

Here are some comments on v2 patch:

---
        /* Report this after the initial starting message for consistency. */
-       if (max_replication_slots == 0)
+       if (max_replication_origins == 0)
                ereport(ERROR,
                                (errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
                                 errmsg("cannot start logical
replication workers when \"max_replication_slots\"=0")));

Need to update the error message too.

---
+               {"max_replication_origins",
+                       PGC_POSTMASTER,
+                       REPLICATION_SUBSCRIBERS,
+                       gettext_noop("Sets the maximum number of
simultaneously defined replication origins."),
+                       NULL
+               },

I think the description is not accurate; this GUC controls the maximum
number of simultaneous replication origins that can be setup.

---
Given that max_replication_origins doesn't control the maximum number
of replication origins that can be defined, probably we need to find a
better name. As Kuroda-san already mentioned some proposed names,
max_tracked_replication_origins or max_replication_origin_states seem
reasonable to me.

---
+#include "utils/guc_hooks.h"

I think #include'ing guc.h would be more appropriate.

------
        ereport(PANIC,
                (errcode(ERRCODE_DATA_CORRUPTED),
                 errmsg("replication slot checkpoint has wrong
checksum %u, expected %u",
                        crc, file_crc)));

I don't understand why we use the term "replication slot checkpoint"
in an error message for the replication origin code. It could be fixed
in a separate patch for back-patching.

---
It's not related to the proposed patch but I found a suspicious
behavior; when the server startup we try to restore the
replorigin_checkpoint file only when max_replication_slots (or
max_replication_origins with the patch) > 0. Therefore, we don't get
the error message "could not find free replication state, increase
\"max_replication_slots\"" when it's set to 0, even if we have some
replication states in the replorigin_checkpoint file. Which seems
quite confusing.

Regards,

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

On Wed, Feb 5, 2025 at 8:17 AM Euler Taveira <euler@eulerto.com> wrote:

>

> Under reflection, an accurate name is max_replication_origin_session_setup. A

> counter argument is that it is a long name (top-5 length).

>

> postgres=# select n, length(n) from (values('max_replication_origins'),

> ('max_tracked_replication_origins'),('max_replication_origin_states'),

> ('max_replication_origin_session_setup')) as gucs(n);

>                   n                   | length

> --------------------------------------+--------

> max_replication_origins              |     23

> max_tracked_replication_origins      |     31

> max_replication_origin_states        |     29

> max_replication_origin_session_setup |     36

> (4 rows)

>

The other possibility is max_replication_origin_sessions.

On Wed, Feb 5, 2025 at 4:39 PM Euler Taveira <euler@eulerto.com> wrote:
>
> On Wed, Feb 5, 2025, at 1:56 AM, Amit Kapila wrote:
>
> On Wed, Feb 5, 2025 at 8:17 AM Euler Taveira <euler@eulerto.com> wrote:
> >
> > Under reflection, an accurate name is max_replication_origin_session_setup. A
> > counter argument is that it is a long name (top-5 length).
> >
> > postgres=# select n, length(n) from (values('max_replication_origins'),
> > ('max_tracked_replication_origins'),('max_replication_origin_states'),
> > ('max_replication_origin_session_setup')) as gucs(n);
> >                   n                   | length
> > --------------------------------------+--------
> > max_replication_origins              |     23
> > max_tracked_replication_origins      |     31
> > max_replication_origin_states        |     29
> > max_replication_origin_session_setup |     36
> > (4 rows)
> >
>
> The other possibility is max_replication_origin_sessions.
>
>
> Looks reasonable to me.
>
> Opinions?

+1

Regards,

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

On Tue, Feb 11, 2025 at 12:27 PM Euler Taveira <euler@eulerto.com> wrote:
>
> On Wed, Feb 5, 2025, at 9:49 PM, Masahiko Sawada wrote:
>
> On Wed, Feb 5, 2025 at 4:39 PM Euler Taveira <euler@eulerto.com> wrote:
> >
> > On Wed, Feb 5, 2025, at 1:56 AM, Amit Kapila wrote:
> >
> > On Wed, Feb 5, 2025 at 8:17 AM Euler Taveira <euler@eulerto.com> wrote:
> > >
> > > Under reflection, an accurate name is max_replication_origin_session_setup. A
> > > counter argument is that it is a long name (top-5 length).
> > >
> > > postgres=# select n, length(n) from (values('max_replication_origins'),
> > > ('max_tracked_replication_origins'),('max_replication_origin_states'),
> > > ('max_replication_origin_session_setup')) as gucs(n);
> > >                   n                   | length
> > > --------------------------------------+--------
> > > max_replication_origins              |     23
> > > max_tracked_replication_origins      |     31
> > > max_replication_origin_states        |     29
> > > max_replication_origin_session_setup |     36
> > > (4 rows)
> > >
> >
> > The other possibility is max_replication_origin_sessions.
> >
> >
> > Looks reasonable to me.
> >
> > Opinions?
>
> +1
>
>
> Here is another patch that only changes the GUC name to
> max_replication_origin_sessions.

Thank you for updating the patch! The patch looks mostly good to me.
Here is one minor point:

In logical-replication.sgml there is another place we need to update
(in 29.13.2. Prepare for subscriber upgrades):

    <listitem>
     <para>
      The new cluster must have
      <link linkend="guc-max-replication-slots"><varname>max_replication_slots</varname></link>
      configured to a value greater than or equal to the number of
      subscriptions present in the old cluster.
     </para>
    </listitem>
   </itemizedlist>

Regards,

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

On Thu, Feb 13, 2025 at 6:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Thank you for updating the patch! The patch looks mostly good to me.
>

+ /*
+ * Prior to PostgreSQL 18, max_replication_slots was used to set the
+ * number of replication origins. For backward compatibility, -1 indicates
+ * to use the fallback value (max_replication_slots).
+ */
+ if (max_replication_origin_sessions == -1)

Shouldn't we let users use max_replication_origin_sessions for
subscribers? Maintaining this mapping for a long time can create
confusion such that some users will keep using max_replication_slots
for origins, and others will start using
max_replication_origin_sessions. Such a mapping would be useful if we
were doing something like this in back-branches, but doing it in a
major version appears to be more of a maintenance burden.

--
With Regards,
Amit Kapila.

On Thu, Feb 13, 2025 at 6:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

>

> Thank you for updating the patch! The patch looks mostly good to me.

>

+ /*

+ * Prior to PostgreSQL 18, max_replication_slots was used to set the

+ * number of replication origins. For backward compatibility, -1 indicates

+ * to use the fallback value (max_replication_slots).

+ */

+ if (max_replication_origin_sessions == -1)

Shouldn't we let users use max_replication_origin_sessions for

subscribers? Maintaining this mapping for a long time can create

confusion such that some users will keep using max_replication_slots

for origins, and others will start using

max_replication_origin_sessions. Such a mapping would be useful if we

were doing something like this in back-branches, but doing it in a

major version appears to be more of a maintenance burden.

On Wed, Mar 5, 2025 at 6:24 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Sat, Mar 1, 2025, at 10:08 AM, Amit Kapila wrote:
>
> On Thu, Feb 13, 2025 at 6:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Thank you for updating the patch! The patch looks mostly good to me.
> >
>
> + /*
> + * Prior to PostgreSQL 18, max_replication_slots was used to set the
> + * number of replication origins. For backward compatibility, -1 indicates
> + * to use the fallback value (max_replication_slots).
> + */
> + if (max_replication_origin_sessions == -1)
>
> Shouldn't we let users use max_replication_origin_sessions for
> subscribers? Maintaining this mapping for a long time can create
> confusion such that some users will keep using max_replication_slots
> for origins, and others will start using
> max_replication_origin_sessions. Such a mapping would be useful if we
> were doing something like this in back-branches, but doing it in a
> major version appears to be more of a maintenance burden.
>
>
> It was my initial proposal. Of course, it breaks compatibility but it
> forces the users to adopt this new GUC. It will be a smooth adoption
> because if we use the same value as max_replication_slots it means
> most of the scenarios will be covered (10 is a good start point for the
> majority of replication scenarios in my experience).
>
> However, Peter E suggested that it would be a good idea to have a
> backward compatibility so I added it.
>
> We need to decide which option we want:
>
> 1. no backward compatibility and max_replication_origin_sessions = 10 as
>   default value. It might break scenarios that have the number of
>   subscriptions greater than the default value.
>

To avoid breakage, can we add a check during the upgrade to ensure
that users have set max_replication_origin_sessions appropriately? See
the current check in function
check_new_cluster_subscription_configuration(). It uses
max_replication_slots, we can change it to use
max_replication_origin_sessions for versions greater than or equal to
18. Will that address this concern?

> 2. backward compatibility for a certain number of releases until the
>    tools can be adjusted. However, the applications that use it were
>    adjusted as part of this patch. The drawback here is that once we
>    accept -1, we cannot remove it without breaking compatibility.
>

Right, I find that path will add more maintenance burden in terms of
remembering this and finding a way to deprecate such a check.

> My preference was 1 but I'm fine with 2 too. Since it is a major
> release, it is natural to introduce features that break things.
>

+1.

Does anyone else have an opinion on this?

--
With Regards,
Amit Kapila.

On Tue, Mar 4, 2025 at 10:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Mar 5, 2025 at 6:24 AM Euler Taveira <euler@eulerto.com> wrote:
> >
> > On Sat, Mar 1, 2025, at 10:08 AM, Amit Kapila wrote:
> >
> > On Thu, Feb 13, 2025 at 6:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > Thank you for updating the patch! The patch looks mostly good to me.
> > >
> >
> > + /*
> > + * Prior to PostgreSQL 18, max_replication_slots was used to set the
> > + * number of replication origins. For backward compatibility, -1 indicates
> > + * to use the fallback value (max_replication_slots).
> > + */
> > + if (max_replication_origin_sessions == -1)
> >
> > Shouldn't we let users use max_replication_origin_sessions for
> > subscribers? Maintaining this mapping for a long time can create
> > confusion such that some users will keep using max_replication_slots
> > for origins, and others will start using
> > max_replication_origin_sessions. Such a mapping would be useful if we
> > were doing something like this in back-branches, but doing it in a
> > major version appears to be more of a maintenance burden.
> >
> >
> > It was my initial proposal. Of course, it breaks compatibility but it
> > forces the users to adopt this new GUC. It will be a smooth adoption
> > because if we use the same value as max_replication_slots it means
> > most of the scenarios will be covered (10 is a good start point for the
> > majority of replication scenarios in my experience).
> >
> > However, Peter E suggested that it would be a good idea to have a
> > backward compatibility so I added it.
> >
> > We need to decide which option we want:
> >
> > 1. no backward compatibility and max_replication_origin_sessions = 10 as
> >   default value. It might break scenarios that have the number of
> >   subscriptions greater than the default value.
> >
>
> To avoid breakage, can we add a check during the upgrade to ensure
> that users have set max_replication_origin_sessions appropriately? See
> the current check in function
> check_new_cluster_subscription_configuration(). It uses
> max_replication_slots, we can change it to use
> max_replication_origin_sessions for versions greater than or equal to
> 18. Will that address this concern?

I think that the patch already replaced max_replication_slots with
max_replication_origin_sessions in that function.

>
> > 2. backward compatibility for a certain number of releases until the
> >    tools can be adjusted. However, the applications that use it were
> >    adjusted as part of this patch. The drawback here is that once we
> >    accept -1, we cannot remove it without breaking compatibility.
> >
>
> Right, I find that path will add more maintenance burden in terms of
> remembering this and finding a way to deprecate such a check.
>
> > My preference was 1 but I'm fine with 2 too. Since it is a major
> > release, it is natural to introduce features that break things.
> >
>
> +1.

+1

A major release would be a good timing to break off the relationship
between the number of origins and the number of replication slots.

Regards,

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

On Wed, Mar 5, 2025 at 12:42 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Mar 4, 2025 at 10:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Mar 5, 2025 at 6:24 AM Euler Taveira <euler@eulerto.com> wrote:
> > >
> > > On Sat, Mar 1, 2025, at 10:08 AM, Amit Kapila wrote:
> > >
> > > On Thu, Feb 13, 2025 at 6:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > Thank you for updating the patch! The patch looks mostly good to me.
> > > >
> > >
> > > + /*
> > > + * Prior to PostgreSQL 18, max_replication_slots was used to set the
> > > + * number of replication origins. For backward compatibility, -1 indicates
> > > + * to use the fallback value (max_replication_slots).
> > > + */
> > > + if (max_replication_origin_sessions == -1)
> > >
> > > Shouldn't we let users use max_replication_origin_sessions for
> > > subscribers? Maintaining this mapping for a long time can create
> > > confusion such that some users will keep using max_replication_slots
> > > for origins, and others will start using
> > > max_replication_origin_sessions. Such a mapping would be useful if we
> > > were doing something like this in back-branches, but doing it in a
> > > major version appears to be more of a maintenance burden.
> > >
> > >
> > > It was my initial proposal. Of course, it breaks compatibility but it
> > > forces the users to adopt this new GUC. It will be a smooth adoption
> > > because if we use the same value as max_replication_slots it means
> > > most of the scenarios will be covered (10 is a good start point for the
> > > majority of replication scenarios in my experience).
> > >
> > > However, Peter E suggested that it would be a good idea to have a
> > > backward compatibility so I added it.
> > >
> > > We need to decide which option we want:
> > >
> > > 1. no backward compatibility and max_replication_origin_sessions = 10 as
> > >   default value. It might break scenarios that have the number of
> > >   subscriptions greater than the default value.
> > >
> >
> > To avoid breakage, can we add a check during the upgrade to ensure
> > that users have set max_replication_origin_sessions appropriately? See
> > the current check in function
> > check_new_cluster_subscription_configuration(). It uses
> > max_replication_slots, we can change it to use
> > max_replication_origin_sessions for versions greater than or equal to
> > 18. Will that address this concern?
>
> I think that the patch already replaced max_replication_slots with
> max_replication_origin_sessions in that function.
>

Then, that should be sufficient to avoid upgrade related risks.

--
With Regards,
Amit Kapila.

On 11.02.25 21:25, Euler Taveira wrote:
> Here is another patch that only changes the GUC name to
> max_replication_origin_sessions.

I think the naming and description of this is still confusing.

What does this name mean?  There is (I think) no such thing as a 
"replication origin session".  So why would there be a maximum of those?

The description in the documentation, after the patch, says "Specifies 
how many replication origins (see Chapter 48) can be tracked 
simultaneously".  But Chapter 48 does not say anything about what it 
means for a replication slot to be tracked.  The only use of the word 
tracked in that chapter is to say that replication slots do the tracking 
of replication progress.

Both of these are confusing independently, but moreover we are now using 
two different words ("sessions" and "tracked") for apparently the same 
thing, but neither of which is adequately documented in those terms 
anywhere else.

I agree that the originally proposed name max_replication_origins is not 
good, because you can "create" (using pg_replication_origin_create()) 
more than the configured maximum.  What is the term for what the setting 
actually controls?  How many are "active"?  "In use"?  Per session?  etc.

On Wed, Mar 5, 2025 at 4:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 11.02.25 21:25, Euler Taveira wrote:
> > Here is another patch that only changes the GUC name to
> > max_replication_origin_sessions.
>
> I think the naming and description of this is still confusing.
>
...
...
>
> I agree that the originally proposed name max_replication_origins is not
> good, because you can "create" (using pg_replication_origin_create())
> more than the configured maximum.  What is the term for what the setting
> actually controls?  How many are "active"?  "In use"?  Per session?  etc.
>

It controls the number of active sessions using origin. The idea is
that to track replication progress via replication_origin we need to
do replorigin_session_setup(). If you look in the code, we have used
the term replorigin_session* in many places, so we thought of naming
this as max_replication_origin_sessions. But the other options could
be max_active_replication_origins or max_replication_origins_in_use.

--
With Regards,
Amit Kapila.

On Wed, Mar 5, 2025 at 4:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:

>

> On 11.02.25 21:25, Euler Taveira wrote:

> > Here is another patch that only changes the GUC name to

> > max_replication_origin_sessions.

>

> I think the naming and description of this is still confusing.

>

...

...

>

> I agree that the originally proposed name max_replication_origins is not

> good, because you can "create" (using pg_replication_origin_create())

> more than the configured maximum.  What is the term for what the setting

> actually controls?  How many are "active"?  "In use"?  Per session?  etc.

>

It controls the number of active sessions using origin. The idea is

that to track replication progress via replication_origin we need to

do replorigin_session_setup(). If you look in the code, we have used

the term replorigin_session* in many places, so we thought of naming

this as max_replication_origin_sessions. But the other options could

be max_active_replication_origins or max_replication_origins_in_use.

On Thu, Mar 6, 2025 at 6:36 PM Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Mar 6, 2025, at 6:55 AM, Amit Kapila wrote:
>
> On Wed, Mar 5, 2025 at 4:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> >
> > On 11.02.25 21:25, Euler Taveira wrote:
> > > Here is another patch that only changes the GUC name to
> > > max_replication_origin_sessions.
> >
> > I think the naming and description of this is still confusing.
> >
> ...
> ...
> >
> > I agree that the originally proposed name max_replication_origins is not
> > good, because you can "create" (using pg_replication_origin_create())
> > more than the configured maximum.  What is the term for what the setting
> > actually controls?  How many are "active"?  "In use"?  Per session?  etc.
> >
>
> It controls the number of active sessions using origin. The idea is
> that to track replication progress via replication_origin we need to
> do replorigin_session_setup(). If you look in the code, we have used
> the term replorigin_session* in many places, so we thought of naming
> this as max_replication_origin_sessions. But the other options could
> be max_active_replication_origins or max_replication_origins_in_use.
>
>
> The word "session" is correlated to "replication origin" but requires some
> knowledge to know the replication progress tracking design. The word "active"
> can express the fact that it was setup and is currently in use. I vote for
> max_active_replication_origins.
>

Sounds reasonable. Let's go with max_active_replication_origins then,
unless people think otherwise.

--
With Regards,
Amit Kapila.

On 07.03.25 04:51, Amit Kapila wrote:
>>> I agree that the originally proposed name max_replication_origins is not
>>> good, because you can "create" (using pg_replication_origin_create())
>>> more than the configured maximum.  What is the term for what the setting
>>> actually controls?  How many are "active"?  "In use"?  Per session?  etc.
>>>
>> It controls the number of active sessions using origin. The idea is
>> that to track replication progress via replication_origin we need to
>> do replorigin_session_setup(). If you look in the code, we have used
>> the term replorigin_session* in many places, so we thought of naming
>> this as max_replication_origin_sessions. But the other options could
>> be max_active_replication_origins or max_replication_origins_in_use.
>>
>>
>> The word "session" is correlated to "replication origin" but requires some
>> knowledge to know the replication progress tracking design. The word "active"
>> can express the fact that it was setup and is currently in use. I vote for
>> max_active_replication_origins.
>>
> Sounds reasonable. Let's go with max_active_replication_origins then,
> unless people think otherwise.

Is that maximum active for the whole system, or maximum active per 
session, or maximum active per created origin, or some combination of these?

Is that maximum active for the whole system, or maximum active per 

session, or maximum active per created origin, or some combination of these?

On Fri, 7 Mar 2025 at 21:21, Euler Taveira <euler@eulerto.com> wrote:
>
> On Fri, Mar 7, 2025, at 11:47 AM, Peter Eisentraut wrote:
>
> Is that maximum active for the whole system, or maximum active per
> session, or maximum active per created origin, or some combination of these?
>
>
> It is a cluster-wide setting. Similar to max_replication_slots. I will make
> sure the GUC description is clear about it. It seems the Replication Progress
> Tracking chapter needs an update to specify this information too.

I reviewed the discussion on this thread and believe we now have an
agreement on the design and GUC names. However, the patch still needs
updates and extensive testing, especially considering its impact on
backward compatibility. I'm unsure if this feature can be committed in
the current CommitFest. If you're okay with it, we can move it to the
next CommitFest. However, if you prefer to keep it, we can do our best
to complete it and make a final decision at the end of the CommitFest.

Regards,
Vignesh

I reviewed the discussion on this thread and believe we now have an

agreement on the design and GUC names. However, the patch still needs

updates and extensive testing, especially considering its impact on

backward compatibility. I'm unsure if this feature can be committed in

the current CommitFest. If you're okay with it, we can move it to the

next CommitFest. However, if you prefer to keep it, we can do our best

to complete it and make a final decision at the end of the CommitFest.

On Thu, 13 Mar 2025 at 05:59, Euler Taveira <euler@eulerto.com> wrote:
>
> On Wed, Mar 12, 2025, at 8:47 AM, vignesh C wrote:
>
> I reviewed the discussion on this thread and believe we now have an
> agreement on the design and GUC names. However, the patch still needs
> updates and extensive testing, especially considering its impact on
> backward compatibility. I'm unsure if this feature can be committed in
> the current CommitFest. If you're okay with it, we can move it to the
> next CommitFest. However, if you prefer to keep it, we can do our best
> to complete it and make a final decision at the end of the CommitFest.
>
>
> This is a mechanical patch. I was waiting if someone would object or suggest a
> better GUC name. It seems to me it isn't. The pre existing GUC
> (max_replication_slots) already has coverage. I don't know what additional
> tests you have in mind. Could you elaborate?

I was considering any potential impact on pg_upgrade and
pg_createsubscriber. I will run tests with the latest posted patch to
verify.

> I'm biased to say that it is one of the easiest patches to commit because it is
> just assigning a new GUC name for a pre existing functionality. If there is no
> interested in it, it will be moved to the next CF.

Sounds like a plan! Let's verify it and work towards getting it committed.

Regards,
Vignesh

On Thu, 13 Mar 2025 at 05:59, Euler Taveira <euler@eulerto.com> wrote:
>
> On Wed, Mar 12, 2025, at 8:47 AM, vignesh C wrote:
>
> I reviewed the discussion on this thread and believe we now have an
> agreement on the design and GUC names. However, the patch still needs
> updates and extensive testing, especially considering its impact on
> backward compatibility. I'm unsure if this feature can be committed in
> the current CommitFest. If you're okay with it, we can move it to the
> next CommitFest. However, if you prefer to keep it, we can do our best
> to complete it and make a final decision at the end of the CommitFest.
>
>
> This is a mechanical patch. I was waiting if someone would object or suggest a
> better GUC name. It seems to me it isn't. The pre existing GUC
> (max_replication_slots) already has coverage. I don't know what additional
> tests you have in mind. Could you elaborate?
>
> I'm biased to say that it is one of the easiest patches to commit because it is
> just assigning a new GUC name for a pre existing functionality. If there is no
> interested in it, it will be moved to the next CF.
>
> I'm adding 2 patches. The 0001 contains the same version as the previous one
> but I renamed the GUC name to max_active_replication_origins. I'm also
> attaching 0002 if we decide that backward compatibility is not a concern so it
> removes it and assign 10 as the default value. I'm adding an additional suffix
> so CF bot doesn't grab 0002.

Few comments:
1) After selecting max_active_replication_origins setting in the
SELECT query having order by, the first record returned will be the
one with max_active_replication_origins, rather than the second
record, because max_active_replication_origins appears before
max_logical_replication_workers in the order.
        res = PQexec(conn,
                                 "SELECT setting FROM
pg_catalog.pg_settings WHERE name IN ("
                                 "'max_logical_replication_workers', "
-                                "'max_replication_slots', "
+                                "'max_active_replication_origins', "
                                 "'max_worker_processes', "
                                 "'primary_slot_name') "
                                 "ORDER BY name");

The code below in the function should be modified accordingly:
max_lrworkers = atoi(PQgetvalue(res, 0, 0));
max_reporigins = atoi(PQgetvalue(res, 1, 0));
max_wprocs = atoi(PQgetvalue(res, 2, 0));

2) I felt max_replication_slots must be max_active_replication_origins
here in logical-replication.sgml:
      The new cluster must have
      <link linkend= guc-max-replication-slots
><varname>max_replication_slots</varname></link>
      configured to a value greater than or equal to the number of
      subscriptions present in the old cluster.

Regards,
Vignesh

Few comments:

1) After selecting max_active_replication_origins setting in the

SELECT query having order by, the first record returned will be the

one with max_active_replication_origins, rather than the second

record, because max_active_replication_origins appears before

max_logical_replication_workers in the order.

2) I felt max_replication_slots must be max_active_replication_origins

here in logical-replication.sgml:

On Fri, 14 Mar 2025 at 06:25, Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Mar 13, 2025, at 11:10 AM, vignesh C wrote:
>
> Few comments:
>
>
> Thanks for taking a look.
>
> 1) After selecting max_active_replication_origins setting in the
> SELECT query having order by, the first record returned will be the
> one with max_active_replication_origins, rather than the second
> record, because max_active_replication_origins appears before
> max_logical_replication_workers in the order.
>
>
> Fixed.
>
> 2) I felt max_replication_slots must be max_active_replication_origins
> here in logical-replication.sgml:
>
>
> Fixed.

Few comments:
1) Should we add a test case to verify that if
max_active_replication_origins is set to -1, it will use
max_replication_slots value:
+        * Prior to PostgreSQL 18, max_replication_slots was used to set the
+        * number of replication origins. For backward compatibility,
-1 indicates
+        * to use the fallback value (max_replication_slots).
+        */
+       if (max_active_replication_origins == -1)

Maybe since the default is -1, we can just add the verification in one
of the tests.

2) Should we consider about the origin's created by user using the
replication managment function at [1] or is it intentionally left out:
-    <link linkend="guc-max-replication-slots-subscriber"><varname>max_replication_slots</varname></link>
+    <link linkend="guc-max-active-replication-origins"><varname>max_active_replication_origins</varname></link>
     must be set to at least the number of subscriptions that will be added to
     the subscriber, plus some reserve for table synchronization.

[1] - https://www.postgresql.org/docs/current/replication-origins.html

Regards,
Vignesh

1) Should we add a test case to verify that if

max_active_replication_origins is set to -1, it will use

max_replication_slots value:

2) Should we consider about the origin's created by user using the

replication managment function at [1] or is it intentionally left out:

-    <link linkend="guc-max-replication-slots-subscriber"><varname>max_replication_slots</varname></link>

+    <link linkend="guc-max-active-replication-origins"><varname>max_active_replication_origins</varname></link>

     must be set to at least the number of subscriptions that will be added to

     the subscriber, plus some reserve for table synchronization.

On Thu, Mar 13, 2025 at 5:55 PM Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Mar 13, 2025, at 11:10 AM, vignesh C wrote:
>
> Few comments:
>
>
> Thanks for taking a look.
>
> 1) After selecting max_active_replication_origins setting in the
> SELECT query having order by, the first record returned will be the
> one with max_active_replication_origins, rather than the second
> record, because max_active_replication_origins appears before
> max_logical_replication_workers in the order.
>
>
> Fixed.
>
> 2) I felt max_replication_slots must be max_active_replication_origins
> here in logical-replication.sgml:
>
>
> Fixed.

Thank you for updating the patch. I have one comment:

 #max_logical_replication_workers = 4   # taken from max_worker_processes
                    # (change requires restart)
+#max_active_replication_origins = 10   # maximum number of active
replication origins
+                   # (change requires restart)
 #max_sync_workers_per_subscription = 2 # taken from
max_logical_replication_workers
 #max_parallel_apply_workers_per_subscription = 2   # taken from
max_logical_replication_workers

I would suggest putting the new max_active_replication_origins after
max_parallel_apply_workers_per_subscription as both
max_sync_workers_per_subscription and
max_parallel_apply_workers_per_subscription are related to
max_logical_replication_workers.

The rest looks good to me.

Regards,

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

I would suggest putting the new max_active_replication_origins after

max_parallel_apply_workers_per_subscription as both

max_sync_workers_per_subscription and

max_parallel_apply_workers_per_subscription are related to

max_logical_replication_workers.

On Mon, Mar 17, 2025 at 6:05 PM Euler Taveira <euler@eulerto.com> wrote:
>
> On Mon, Mar 17, 2025, at 8:44 PM, Masahiko Sawada wrote:
>
> I would suggest putting the new max_active_replication_origins after
> max_parallel_apply_workers_per_subscription as both
> max_sync_workers_per_subscription and
> max_parallel_apply_workers_per_subscription are related to
> max_logical_replication_workers.
>
>
> Good point. Looking at the documentation, the old max_replication_slots
> parameter was the first one in that section so I decided to use the same order
> for the postgresql.conf.sample.

Thank you for updating the patch!

The patch looks good to me. I've updated the commit message and
squashed the second patch as we agreed that we don't need to have the
codes for supporting backward compatibility IIUC. I'm going to push
the patch barring any objections and further comments.

Regards,

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

On Wed, Mar 19, 2025 at 10:43 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Mar 17, 2025 at 6:05 PM Euler Taveira <euler@eulerto.com> wrote:
> >
> > On Mon, Mar 17, 2025, at 8:44 PM, Masahiko Sawada wrote:
> >
> > I would suggest putting the new max_active_replication_origins after
> > max_parallel_apply_workers_per_subscription as both
> > max_sync_workers_per_subscription and
> > max_parallel_apply_workers_per_subscription are related to
> > max_logical_replication_workers.
> >
> >
> > Good point. Looking at the documentation, the old max_replication_slots
> > parameter was the first one in that section so I decided to use the same order
> > for the postgresql.conf.sample.
>
> Thank you for updating the patch!
>

*
   <para>
    Logical replication requires several configuration options to be set. Most
-   options are relevant only on one side of the replication. However,
-   <varname>max_replication_slots</varname> is used on both the publisher and
-   the subscriber, but it has a different meaning for each.
+   options are relevant only on one side of the replication.
   </para>

In this para, after removing the content about max_replication_slots,
the other line: "Most options are relevant only on one side of the
replication." doesn't make sense because there is no other option that
applies to both sides and if there is one then we should mention about
that.

> The patch looks good to me.
>

Other than the above, the patch looks good to me as well.

--
With Regards,
Amit Kapila.

On Wed, Mar 19, 2025 at 8:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Mar 19, 2025 at 10:43 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Mar 17, 2025 at 6:05 PM Euler Taveira <euler@eulerto.com> wrote:
> > >
> > > On Mon, Mar 17, 2025, at 8:44 PM, Masahiko Sawada wrote:
> > >
> > > I would suggest putting the new max_active_replication_origins after
> > > max_parallel_apply_workers_per_subscription as both
> > > max_sync_workers_per_subscription and
> > > max_parallel_apply_workers_per_subscription are related to
> > > max_logical_replication_workers.
> > >
> > >
> > > Good point. Looking at the documentation, the old max_replication_slots
> > > parameter was the first one in that section so I decided to use the same order
> > > for the postgresql.conf.sample.
> >
> > Thank you for updating the patch!
> >
>
> *
>    <para>
>     Logical replication requires several configuration options to be set. Most
> -   options are relevant only on one side of the replication. However,
> -   <varname>max_replication_slots</varname> is used on both the publisher and
> -   the subscriber, but it has a different meaning for each.
> +   options are relevant only on one side of the replication.
>    </para>
>
> In this para, after removing the content about max_replication_slots,
> the other line: "Most options are relevant only on one side of the
> replication." doesn't make sense because there is no other option that
> applies to both sides and if there is one then we should mention about
> that.

Good point. How about the following change?

   <para>
-   Logical replication requires several configuration options to be set. Most
+   Logical replication requires several configuration options to be set. These
    options are relevant only on one side of the replication.
   </para>

Regards,

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

On Thu, Mar 20, 2025 at 10:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Mar 19, 2025 at 8:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 10:43 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Mon, Mar 17, 2025 at 6:05 PM Euler Taveira <euler@eulerto.com> wrote:
> > > >
> > > > On Mon, Mar 17, 2025, at 8:44 PM, Masahiko Sawada wrote:
> > > >
> > > > I would suggest putting the new max_active_replication_origins after
> > > > max_parallel_apply_workers_per_subscription as both
> > > > max_sync_workers_per_subscription and
> > > > max_parallel_apply_workers_per_subscription are related to
> > > > max_logical_replication_workers.
> > > >
> > > >
> > > > Good point. Looking at the documentation, the old max_replication_slots
> > > > parameter was the first one in that section so I decided to use the same order
> > > > for the postgresql.conf.sample.
> > >
> > > Thank you for updating the patch!
> > >
> >
> > *
> >    <para>
> >     Logical replication requires several configuration options to be set. Most
> > -   options are relevant only on one side of the replication. However,
> > -   <varname>max_replication_slots</varname> is used on both the publisher and
> > -   the subscriber, but it has a different meaning for each.
> > +   options are relevant only on one side of the replication.
> >    </para>
> >
> > In this para, after removing the content about max_replication_slots,
> > the other line: "Most options are relevant only on one side of the
> > replication." doesn't make sense because there is no other option that
> > applies to both sides and if there is one then we should mention about
> > that.
>
> Good point. How about the following change?
>
>    <para>
> -   Logical replication requires several configuration options to be set. Most
> +   Logical replication requires several configuration options to be set. These
>     options are relevant only on one side of the replication.
>    </para>
>

LGTM.

--
With Regards,
Amit Kapila.

On Thu, Mar 20, 2025 at 8:38 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Mar 20, 2025 at 10:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Mar 19, 2025 at 8:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Mar 19, 2025 at 10:43 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Mon, Mar 17, 2025 at 6:05 PM Euler Taveira <euler@eulerto.com> wrote:
> > > > >
> > > > > On Mon, Mar 17, 2025, at 8:44 PM, Masahiko Sawada wrote:
> > > > >
> > > > > I would suggest putting the new max_active_replication_origins after
> > > > > max_parallel_apply_workers_per_subscription as both
> > > > > max_sync_workers_per_subscription and
> > > > > max_parallel_apply_workers_per_subscription are related to
> > > > > max_logical_replication_workers.
> > > > >
> > > > >
> > > > > Good point. Looking at the documentation, the old max_replication_slots
> > > > > parameter was the first one in that section so I decided to use the same order
> > > > > for the postgresql.conf.sample.
> > > >
> > > > Thank you for updating the patch!
> > > >
> > >
> > > *
> > >    <para>
> > >     Logical replication requires several configuration options to be set. Most
> > > -   options are relevant only on one side of the replication. However,
> > > -   <varname>max_replication_slots</varname> is used on both the publisher and
> > > -   the subscriber, but it has a different meaning for each.
> > > +   options are relevant only on one side of the replication.
> > >    </para>
> > >
> > > In this para, after removing the content about max_replication_slots,
> > > the other line: "Most options are relevant only on one side of the
> > > replication." doesn't make sense because there is no other option that
> > > applies to both sides and if there is one then we should mention about
> > > that.
> >
> > Good point. How about the following change?
> >
> >    <para>
> > -   Logical replication requires several configuration options to be set. Most
> > +   Logical replication requires several configuration options to be set. These
> >     options are relevant only on one side of the replication.
> >    </para>
> >
>
> LGTM.

Thank you for reviewing it. I've committed the patch.


Regards,

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