Re: Separate GUC for replication origins

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?

> 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.