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