On Tue, Mar 4, 2025 at 10:42 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Mar 5, 2025 at 6:24 AM Euler Taveira <eu...@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.m...@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
