Re: Separate GUC for replication origins

On Tue, Mar 4, 2025 at 10:42 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Mar 5, 2025 at 6:24 AM Euler Taveira <euler(at)eulerto(dot)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(dot)mshk(at)gmail(dot)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