Thread: PGDOCS - Logical replication GUCs - added some xrefs

PGDOCS - Logical replication GUCs - added some xrefs

From
Peter Smith
Date:
Hi hackers.

There is a docs Logical Replication section "31.10 Configuration
Settings" [1] which describes some logical replication GUCs, and
details on how they interact with each other and how to take that into
account when setting their values.

There is another docs Server Configuration section "20.6 Replication"
[2] which lists the replication-related GUC parameters, and what they
are for.

Currently AFAIK those two pages are unconnected, but I felt it might
be helpful if some of the parameters in the list [2] had xref links to
the additional logical replication configuration information [1]. PSA
a patch to do that.

~~

Meanwhile, I also suspect that the main blurb top of [1] is not
entirely correct... it says "These settings control the behaviour of
the built-in streaming replication feature", although some of the GUCs
mentioned later in this section are clearly for "logical replication".

Thoughts?

------
[1] 31.10 Configuration Settings -
https://www.postgresql.org/docs/current/logical-replication-config.html
[2] 20.6 Replication -
https://www.postgresql.org/docs/current/runtime-config-replication.html

Kind Regards,
Peter Smith.
Fujitsu Australia

Attachment

Re: PGDOCS - Logical replication GUCs - added some xrefs

From
vignesh C
Date:
On Mon, 24 Oct 2022 at 13:15, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi hackers.
>
> There is a docs Logical Replication section "31.10 Configuration
> Settings" [1] which describes some logical replication GUCs, and
> details on how they interact with each other and how to take that into
> account when setting their values.
>
> There is another docs Server Configuration section "20.6 Replication"
> [2] which lists the replication-related GUC parameters, and what they
> are for.
>
> Currently AFAIK those two pages are unconnected, but I felt it might
> be helpful if some of the parameters in the list [2] had xref links to
> the additional logical replication configuration information [1]. PSA
> a patch to do that.
>
> ~~
>
> Meanwhile, I also suspect that the main blurb top of [1] is not
> entirely correct... it says "These settings control the behaviour of
> the built-in streaming replication feature", although some of the GUCs
> mentioned later in this section are clearly for "logical replication".

The introduction mainly talks about streaming replication and the page
[1] subsection "Subscribers" clearly mentions that these
configurations are for logical replication. As we already have a
separate page [2] to detail about logical replication configurations,
it might be better to move the "subscribers" section from [1] to [2].

[1] 20.6 Replication -
https://www.postgresql.org/docs/current/runtime-config-replication.html
[2] 31.10 Configuration Settings -
https://www.postgresql.org/docs/current/logical-replication-config.html

Regards,
Vignesh



Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Peter Smith
Date:
On Sun, Nov 13, 2022 at 11:47 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 24 Oct 2022 at 13:15, Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi hackers.
> >
> > There is a docs Logical Replication section "31.10 Configuration
> > Settings" [1] which describes some logical replication GUCs, and
> > details on how they interact with each other and how to take that into
> > account when setting their values.
> >
> > There is another docs Server Configuration section "20.6 Replication"
> > [2] which lists the replication-related GUC parameters, and what they
> > are for.
> >
> > Currently AFAIK those two pages are unconnected, but I felt it might
> > be helpful if some of the parameters in the list [2] had xref links to
> > the additional logical replication configuration information [1]. PSA
> > a patch to do that.
> >
> > ~~
> >
> > Meanwhile, I also suspect that the main blurb top of [1] is not
> > entirely correct... it says "These settings control the behaviour of
> > the built-in streaming replication feature", although some of the GUCs
> > mentioned later in this section are clearly for "logical replication".
>
> The introduction mainly talks about streaming replication and the page
> [1] subsection "Subscribers" clearly mentions that these
> configurations are for logical replication. As we already have a
> separate page [2] to detail about logical replication configurations,
> it might be better to move the "subscribers" section from [1] to [2].
>
> [1] 20.6 Replication -
> https://www.postgresql.org/docs/current/runtime-config-replication.html
> [2] 31.10 Configuration Settings -
> https://www.postgresql.org/docs/current/logical-replication-config.html
>

Thanks, Vignesh. Your suggestion (to move that "Subscribers" section)
seemed like a good idea to me, so PSA my patch v2 to implement that.

Now, on the Streaming Replication page
- the blurb has a reference to information about logical replication config
- the "Subscribers" section was relocated to the other page

Now, on the Logical Replication "Configuration Settings" page
- there are new subsections for "Publishers", "Subscribers" (copied), "Notes"
- some wording is rearranged but the content is basically the same as before

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Attachment

Re: PGDOCS - Logical replication GUCs - added some xrefs

From
vignesh C
Date:
On Tue, 15 Nov 2022 at 11:17, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Sun, Nov 13, 2022 at 11:47 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Mon, 24 Oct 2022 at 13:15, Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > Hi hackers.
> > >
> > > There is a docs Logical Replication section "31.10 Configuration
> > > Settings" [1] which describes some logical replication GUCs, and
> > > details on how they interact with each other and how to take that into
> > > account when setting their values.
> > >
> > > There is another docs Server Configuration section "20.6 Replication"
> > > [2] which lists the replication-related GUC parameters, and what they
> > > are for.
> > >
> > > Currently AFAIK those two pages are unconnected, but I felt it might
> > > be helpful if some of the parameters in the list [2] had xref links to
> > > the additional logical replication configuration information [1]. PSA
> > > a patch to do that.
> > >
> > > ~~
> > >
> > > Meanwhile, I also suspect that the main blurb top of [1] is not
> > > entirely correct... it says "These settings control the behaviour of
> > > the built-in streaming replication feature", although some of the GUCs
> > > mentioned later in this section are clearly for "logical replication".
> >
> > The introduction mainly talks about streaming replication and the page
> > [1] subsection "Subscribers" clearly mentions that these
> > configurations are for logical replication. As we already have a
> > separate page [2] to detail about logical replication configurations,
> > it might be better to move the "subscribers" section from [1] to [2].
> >
> > [1] 20.6 Replication -
> > https://www.postgresql.org/docs/current/runtime-config-replication.html
> > [2] 31.10 Configuration Settings -
> > https://www.postgresql.org/docs/current/logical-replication-config.html
> >
>
> Thanks, Vignesh. Your suggestion (to move that "Subscribers" section)
> seemed like a good idea to me, so PSA my patch v2 to implement that.
>
> Now, on the Streaming Replication page
> - the blurb has a reference to information about logical replication config
> - the "Subscribers" section was relocated to the other page
>
> Now, on the Logical Replication "Configuration Settings" page
> - there are new subsections for "Publishers", "Subscribers" (copied), "Notes"
> - some wording is rearranged but the content is basically the same as before

One suggestion:
The format of subscribers includes the data type and default values,
the format of publishers does not include data type and default
values. We can try to maintain the consistency for both publisher and
subscriber configurations.
+   <para>
+    <varname>wal_level</varname> must be set to <literal>logical</literal>.
+   </para>

+     <term><varname>max_logical_replication_workers</varname>
(<type>integer</type>)
+     <indexterm>
+      <primary><varname>max_logical_replication_workers</varname>
configuration parameter</primary>
+     </indexterm>
+     </term>
+     <listitem>
+      <para>
+       Specifies maximum number of logical replication workers. This
must be set
+       to at least the number of subscriptions (for apply workers), plus some
+       reserve for the table synchronization workers.
+      </para>
+      <para>

If we don't want to keep the same format, we could give a link to
runtime-config-replication where data type and default is defined for
publisher configurations max_replication_slots and max_wal_senders.

Regards,
Vignesh



Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Peter Smith
Date:
On Wed, Nov 16, 2022 at 10:24 PM vignesh C <vignesh21@gmail.com> wrote:
>
...

> One suggestion:
> The format of subscribers includes the data type and default values,
> the format of publishers does not include data type and default
> values. We can try to maintain the consistency for both publisher and
> subscriber configurations.
> +   <para>
> +    <varname>wal_level</varname> must be set to <literal>logical</literal>.
> +   </para>
>
> +     <term><varname>max_logical_replication_workers</varname>
> (<type>integer</type>)
> +     <indexterm>
> +      <primary><varname>max_logical_replication_workers</varname>
> configuration parameter</primary>
> +     </indexterm>
> +     </term>
> +     <listitem>
> +      <para>
> +       Specifies maximum number of logical replication workers. This
> must be set
> +       to at least the number of subscriptions (for apply workers), plus some
> +       reserve for the table synchronization workers.
> +      </para>
> +      <para>
>
> If we don't want to keep the same format, we could give a link to
> runtime-config-replication where data type and default is defined for
> publisher configurations max_replication_slots and max_wal_senders.
>

Thanks for your suggestions.

I have included xref links to the original definitions, rather than
defining the same GUC in multiple places.

PSA v3.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Attachment

Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Peter Smith
Date:
On Wed, Nov 23, 2022 at 9:16 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Nov 16, 2022 at 10:24 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> ...
>
> > One suggestion:
> > The format of subscribers includes the data type and default values,
> > the format of publishers does not include data type and default
> > values. We can try to maintain the consistency for both publisher and
> > subscriber configurations.
> > +   <para>
> > +    <varname>wal_level</varname> must be set to <literal>logical</literal>.
> > +   </para>
> >
> > +     <term><varname>max_logical_replication_workers</varname>
> > (<type>integer</type>)
> > +     <indexterm>
> > +      <primary><varname>max_logical_replication_workers</varname>
> > configuration parameter</primary>
> > +     </indexterm>
> > +     </term>
> > +     <listitem>
> > +      <para>
> > +       Specifies maximum number of logical replication workers. This
> > must be set
> > +       to at least the number of subscriptions (for apply workers), plus some
> > +       reserve for the table synchronization workers.
> > +      </para>
> > +      <para>
> >
> > If we don't want to keep the same format, we could give a link to
> > runtime-config-replication where data type and default is defined for
> > publisher configurations max_replication_slots and max_wal_senders.
> >
>
> Thanks for your suggestions.
>
> I have included xref links to the original definitions, rather than
> defining the same GUC in multiple places.
>
> PSA v3.
>

I updated the patch. The content is unchanged from v3 but the links
are modified so now they render with the correct <varname> format for
the GUC names.

PSA v4.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Attachment

Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Peter Eisentraut
Date:
Your patch moves the description of the subscriber-related configuration 
parameters from config.sgml to logical-replication.sgml.  But 
config.sgml is supposed to contain *all* configuration parameters.  If 
we're going to start splitting this up and moving things around then 
we'd need a more comprehensive plan than this individual patch.  (I'm 
not suggesting that we actually do this.)




Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Peter Smith
Date:
On Fri, Nov 25, 2022 at 9:23 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> Your patch moves the description of the subscriber-related configuration
> parameters from config.sgml to logical-replication.sgml.  But
> config.sgml is supposed to contain *all* configuration parameters.  If
> we're going to start splitting this up and moving things around then
> we'd need a more comprehensive plan than this individual patch.  (I'm
> not suggesting that we actually do this.)
>

OK, thanks for the information.

This v5 patch now only adds some previously missing cross-references
and tidies the Chapter 31.10 "Configuration Settings" section.
Meanwhile, the Subscriber GUC descriptions are left on the
config.sgml, where you said they are supposed to be.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Attachment

Re: PGDOCS - Logical replication GUCs - added some xrefs

From
samay sharma
Date:
Hi,

On Mon, Oct 24, 2022 at 12:45 AM Peter Smith <smithpb2250@gmail.com> wrote:
Hi hackers.

There is a docs Logical Replication section "31.10 Configuration
Settings" [1] which describes some logical replication GUCs, and
details on how they interact with each other and how to take that into
account when setting their values.

There is another docs Server Configuration section "20.6 Replication"
[2] which lists the replication-related GUC parameters, and what they
are for.

Currently AFAIK those two pages are unconnected, but I felt it might
be helpful if some of the parameters in the list [2] had xref links to
the additional logical replication configuration information [1]. PSA
a patch to do that.

+1 on the patch. Some feedback on v5 below.

> +    <para>
> +     For <firstterm>logical replication</firstterm> configuration settings refer
> +     also to <xref linkend="logical-replication-config"/>.
> +    </para>
> +

I feel the top paragraph needs to explain terminology for logical replication like it does for physical replication in addition to linking to the logical replication config page. I'm recommending this as we use terms like subscriber etc. in description of parameters without introducing them first.

As an example, something like below might work.

These settings control the behavior of the built-in streaming replication feature (see Section 27.2.5) and logical replication (link).

For physical replication, servers will be either a primary or a standby server. Primaries can send data, while standbys are always receivers of replicated data. When cascading replication (see Section 27.2.7) is used, standby servers can also be senders, as well as receivers. Parameters are mainly for sending and standby servers, though some parameters have meaning only on the primary server. Settings may vary across the cluster without problems if that is required.

For logical replication, servers will either be publishers (also called senders in the sections below) or subscribers. Publishers are ...., Subscribers are...

> +       <para>
> +         See <xref linkend="logical-replication-config"/> for more details
> +         about setting <varname>max_replication_slots</varname> for logical
> +         replication.
> +        </para>


The link doesn't add any new information regarding max_replication_slots other than "to reserve some for table sync" and has a good amount of unrelated info. I think it might be useful to just put a line here asking to reserve some for table sync instead of linking to the entire logical replication config section.

 
> -   Logical replication requires several configuration options to be set.
> +   Logical replication requires several configuration parameters to be set.

May not be needed? The docs have references to both options and parameters but I don't feel strongly about it. Feel free to use what you prefer.

I think we should add an additional line to the intro here saying that parameters are mostly relevant only one of the subscriber or publisher. Maybe a better written version of "While max_replication_slots means different things on the publisher and subscriber, all other parameters are relevant only on either the publisher or the subscriber."

> +  <sect2 id="logical-replication-config-notes">
> +   <title>Notes</title>

I don't think we need this sub-section. If I understand correctly, these parameters are effective only on the subscriber side. So, any reason to not include them in that section?

> +
> +   <para>
> +    Logical replication workers are also affected by
> +    <link linkend="guc-wal-receiver-timeout"><varname>wal_receiver_timeout</varname></link>,
> +    <link linkend="guc-wal-receiver-status-interval"><varname>wal_receiver_status_interval</varname></link> and
> +    <link linkend="guc-wal-retrieve-retry-interval"><varname>wal_receiver_retry_interval</varname></link>.
> +   </para>
> +

I like moving this; it makes more sense here. Should we remove it from config.sgml? It seems a bit out of place there as we generally talk only about individual parameters there and this line is general logical replication subscriber advise which is more suited to logical-replication.sgml

> +   <para>
> +    Configuration parameter
> +    <link linkend="guc-max-worker-processes"><varname>max_worker_processes</varname></link>
> +    may need to be adjusted to accommodate for replication workers, at least (
> +    <link linkend="guc-max-logical-replication-workers"><varname>max_logical_replication_workers</varname></link>
> +    + <literal>1</literal>). Some extensions and parallel queries also take
> +    worker slots from <varname>max_worker_processes</varname>.
> +   </para>
> +
> +  </sect2>

I think we should move this to the subscriber section as said above. It's useful to know this and people might skip over the notes.


~~

Meanwhile, I also suspect that the main blurb top of [1] is not
entirely correct... it says "These settings control the behaviour of
the built-in streaming replication feature", although some of the GUCs
mentioned later in this section are clearly for "logical replication".

Thoughts?

I shared an idea above.

Regards,
Samay
 

------
[1] 31.10 Configuration Settings -
https://www.postgresql.org/docs/current/logical-replication-config.html
[2] 20.6 Replication -
https://www.postgresql.org/docs/current/runtime-config-replication.html

Kind Regards,
Peter Smith.
Fujitsu Australia

Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Peter Smith
Date:
On Tue, Dec 6, 2022 at 5:57 AM samay sharma <smilingsamay@gmail.com> wrote:
>
> Hi,
>
> On Mon, Oct 24, 2022 at 12:45 AM Peter Smith <smithpb2250@gmail.com> wrote:
>>
>> Hi hackers.
>>
>> There is a docs Logical Replication section "31.10 Configuration
>> Settings" [1] which describes some logical replication GUCs, and
>> details on how they interact with each other and how to take that into
>> account when setting their values.
>>
>> There is another docs Server Configuration section "20.6 Replication"
>> [2] which lists the replication-related GUC parameters, and what they
>> are for.
>>
>> Currently AFAIK those two pages are unconnected, but I felt it might
>> be helpful if some of the parameters in the list [2] had xref links to
>> the additional logical replication configuration information [1]. PSA
>> a patch to do that.
>
>
> +1 on the patch. Some feedback on v5 below.
>

Thanks for your detailed review comments!

I have changed most things according to your suggestions. Please check patch v6.

> > +    <para>
> > +     For <firstterm>logical replication</firstterm> configuration settings refer
> > +     also to <xref linkend="logical-replication-config"/>.
> > +    </para>
> > +
>
> I feel the top paragraph needs to explain terminology for logical replication like it does for physical replication
inaddition to linking to the logical replication config page. I'm recommending this as we use terms like subscriber
etc.in description of parameters without introducing them first. 
>
> As an example, something like below might work.
>
> These settings control the behavior of the built-in streaming replication feature (see Section 27.2.5) and logical
replication(link). 
>
> For physical replication, servers will be either a primary or a standby server. Primaries can send data, while
standbysare always receivers of replicated data. When cascading replication (see Section 27.2.7) is used, standby
serverscan also be senders, as well as receivers. Parameters are mainly for sending and standby servers, though some
parametershave meaning only on the primary server. Settings may vary across the cluster without problems if that is
required.
>
> For logical replication, servers will either be publishers (also called senders in the sections below) or
subscribers.Publishers are ...., Subscribers are... 
>

OK. I split this blurb into 2 parts – streaming and logical
replication. The streaming replication part is the same as before. The
logical replication part is new.

> > +       <para>
> > +         See <xref linkend="logical-replication-config"/> for more details
> > +         about setting <varname>max_replication_slots</varname> for logical
> > +         replication.
> > +        </para>
>
>
> The link doesn't add any new information regarding max_replication_slots other than "to reserve some for table sync"
andhas a good amount of unrelated info. I think it might be useful to just put a line here asking to reserve some for
tablesync instead of linking to the entire logical replication config section. 
>

OK. I copied the tablesync note back to config.sgml definition of
'max_replication_slots' and removed the link as suggested. Frankly, I
also thought it is a bit strange that the max_replication_slots in the
“Sending Servers” section was describing this parameter for
“Subscribers”. OTOH, I did not want to split the definition in half so
instead, I’ve added another Subscriber <varlistentry> that just refers
back to this place. It looks like an improvement to me.

> > -   Logical replication requires several configuration options to be set.
> > +   Logical replication requires several configuration parameters to be set.
>
> May not be needed? The docs have references to both options and parameters but I don't feel strongly about it. Feel
freeto use what you prefer. 

OK. I removed this.

>
> I think we should add an additional line to the intro here saying that parameters are mostly relevant only one of the
subscriberor publisher. Maybe a better written version of "While max_replication_slots means different things on the
publisherand subscriber, all other parameters are relevant only on either the publisher or the subscriber." 
>

OK. Done but with slightly different wording to that.

> > +  <sect2 id="logical-replication-config-notes">
> > +   <title>Notes</title>
>
> I don't think we need this sub-section. If I understand correctly, these parameters are effective only on the
subscriberside. So, any reason to not include them in that section? 

OK. I moved these notes into the "Subscribers" section as suggested,
and removed "Notes".

>
> > +
> > +   <para>
> > +    Logical replication workers are also affected by
> > +    <link linkend="guc-wal-receiver-timeout"><varname>wal_receiver_timeout</varname></link>,
> > +    <link linkend="guc-wal-receiver-status-interval"><varname>wal_receiver_status_interval</varname></link> and
> > +    <link linkend="guc-wal-retrieve-retry-interval"><varname>wal_receiver_retry_interval</varname></link>.
> > +   </para>
> > +
>
> I like moving this; it makes more sense here. Should we remove it from config.sgml? It seems a bit out of place there
aswe generally talk only about individual parameters there and this line is general logical replication subscriber
advisewhich is more suited to logical-replication.sgml 

OK. I agree, it looked repetitive since the link to the
logical-replication page is nearby this information anyway, so I’ve
removed it from the config.sgml as you suggested.

>
> > +   <para>
> > +    Configuration parameter
> > +    <link linkend="guc-max-worker-processes"><varname>max_worker_processes</varname></link>
> > +    may need to be adjusted to accommodate for replication workers, at least (
> > +    <link linkend="guc-max-logical-replication-workers"><varname>max_logical_replication_workers</varname></link>
> > +    + <literal>1</literal>). Some extensions and parallel queries also take
> > +    worker slots from <varname>max_worker_processes</varname>.
> > +   </para>
> > +
> > +  </sect2>
>
> I think we should move this to the subscriber section as said above. It's useful to know this and people might skip
overthe notes. 

OK. Done.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Attachment

Re: PGDOCS - Logical replication GUCs - added some xrefs

From
samay sharma
Date:
Hi,

On Tue, Dec 6, 2022 at 11:12 PM Peter Smith <smithpb2250@gmail.com> wrote:
On Tue, Dec 6, 2022 at 5:57 AM samay sharma <smilingsamay@gmail.com> wrote:
>
> Hi,
>
> On Mon, Oct 24, 2022 at 12:45 AM Peter Smith <smithpb2250@gmail.com> wrote:
>>
>> Hi hackers.
>>
>> There is a docs Logical Replication section "31.10 Configuration
>> Settings" [1] which describes some logical replication GUCs, and
>> details on how they interact with each other and how to take that into
>> account when setting their values.
>>
>> There is another docs Server Configuration section "20.6 Replication"
>> [2] which lists the replication-related GUC parameters, and what they
>> are for.
>>
>> Currently AFAIK those two pages are unconnected, but I felt it might
>> be helpful if some of the parameters in the list [2] had xref links to
>> the additional logical replication configuration information [1]. PSA
>> a patch to do that.
>
>
> +1 on the patch. Some feedback on v5 below.
>

Thanks for your detailed review comments!

I have changed most things according to your suggestions. Please check patch v6.

Thanks for the changes. See a few points of feedback below.

> +    <para>
> +     For <emphasis>logical replication</emphasis>, <firstterm>publishers</firstterm>
> +     (servers that do <link linkend="sql-createpublication"><command>CREATE PUBLICATION</command></link>)
> +     replicate data to <firstterm>subscribers</firstterm>
> +     (servers that do <link linkend="sql-createsubscription"><command>CREATE SUBSCRIPTION</command></link>).
> +     Servers can also be publishers and subscribers at the same time. Note,
> +     the following sections refers to publishers as "senders". The parameter
> +     <literal>max_replication_slots</literal> has a different meaning for the
> +     publisher and subscriber, but all other parameters are relevant only to
> +     one side of the replication. For more details about logical replication
> +     configuration settings refer to
> +     <xref linkend="logical-replication-config"/>.
> +    </para>

The second last line seems a bit odd here. In my last round of feedback, I had meant to add the line "The parameter .... " onwards to the top of logical-replication-config.sgml.

What if we made the top of logical-replication-config.sgml like below?

Logical replication requires several configuration options to be set. Most configuration options are relevant only on one side of the replication (i.e. publisher or subscriber). However, max_replication_slots is applicable on both sides but has different meanings on each side.


> > +    <para>
> > +     For <firstterm>logical replication</firstterm> configuration settings refer
> > +     also to <xref linkend="logical-replication-config"/>.
> > +    </para>
> > +
>
> I feel the top paragraph needs to explain terminology for logical replication like it does for physical replication in addition to linking to the logical replication config page. I'm recommending this as we use terms like subscriber etc. in description of parameters without introducing them first.
>
> As an example, something like below might work.
>
> These settings control the behavior of the built-in streaming replication feature (see Section 27.2.5) and logical replication (link).
>
> For physical replication, servers will be either a primary or a standby server. Primaries can send data, while standbys are always receivers of replicated data. When cascading replication (see Section 27.2.7) is used, standby servers can also be senders, as well as receivers. Parameters are mainly for sending and standby servers, though some parameters have meaning only on the primary server. Settings may vary across the cluster without problems if that is required.
>
> For logical replication, servers will either be publishers (also called senders in the sections below) or subscribers. Publishers are ...., Subscribers are...
>

OK. I split this blurb into 2 parts – streaming and logical
replication. The streaming replication part is the same as before. The
logical replication part is new.

> > +       <para>
> > +         See <xref linkend="logical-replication-config"/> for more details
> > +         about setting <varname>max_replication_slots</varname> for logical
> > +         replication.
> > +        </para>
>
>
> The link doesn't add any new information regarding max_replication_slots other than "to reserve some for table sync" and has a good amount of unrelated info. I think it might be useful to just put a line here asking to reserve some for table sync instead of linking to the entire logical replication config section.
>

OK. I copied the tablesync note back to config.sgml definition of
'max_replication_slots' and removed the link as suggested. Frankly, I
also thought it is a bit strange that the max_replication_slots in the
“Sending Servers” section was describing this parameter for
“Subscribers”. OTOH, I did not want to split the definition in half so
instead, I’ve added another Subscriber <varlistentry> that just refers
back to this place. It looks like an improvement to me.

Hmm, I agree this is a tricky scenario. However, to me, it seems odd to mention the parameter twice as this chapter of the docs just lists each parameter and describes them. So, I'd probably remove the reference to it in the subscriber section. We should describe it's usage in different places in the logical replication part of the docs (as we do).
 

> > -   Logical replication requires several configuration options to be set.
> > +   Logical replication requires several configuration parameters to be set.
>
> May not be needed? The docs have references to both options and parameters but I don't feel strongly about it. Feel free to use what you prefer.

OK. I removed this.

>
> I think we should add an additional line to the intro here saying that parameters are mostly relevant only one of the subscriber or publisher. Maybe a better written version of "While max_replication_slots means different things on the publisher and subscriber, all other parameters are relevant only on either the publisher or the subscriber."
>

OK. Done but with slightly different wording to that.

> > +  <sect2 id="logical-replication-config-notes">
> > +   <title>Notes</title>
>
> I don't think we need this sub-section. If I understand correctly, these parameters are effective only on the subscriber side. So, any reason to not include them in that section?

OK. I moved these notes into the "Subscribers" section as suggested,
and removed "Notes".

>
> > +
> > +   <para>
> > +    Logical replication workers are also affected by
> > +    <link linkend="guc-wal-receiver-timeout"><varname>wal_receiver_timeout</varname></link>,
> > +    <link linkend="guc-wal-receiver-status-interval"><varname>wal_receiver_status_interval</varname></link> and
> > +    <link linkend="guc-wal-retrieve-retry-interval"><varname>wal_receiver_retry_interval</varname></link>.
> > +   </para>
> > +
>
> I like moving this; it makes more sense here. Should we remove it from config.sgml? It seems a bit out of place there as we generally talk only about individual parameters there and this line is general logical replication subscriber advise which is more suited to logical-replication.sgml

OK. I agree, it looked repetitive since the link to the
logical-replication page is nearby this information anyway, so I’ve
removed it from the config.sgml as you suggested.

>
> > +   <para>
> > +    Configuration parameter
> > +    <link linkend="guc-max-worker-processes"><varname>max_worker_processes</varname></link>
> > +    may need to be adjusted to accommodate for replication workers, at least (
> > +    <link linkend="guc-max-logical-replication-workers"><varname>max_logical_replication_workers</varname></link>
> > +    + <literal>1</literal>). Some extensions and parallel queries also take
> > +    worker slots from <varname>max_worker_processes</varname>.
> > +   </para>
> > +
> > +  </sect2>
>
> I think we should move this to the subscriber section as said above. It's useful to know this and people might skip over the notes.

OK. Done.

> +   <para>
> +    <link linkend="guc-max-logical-replication-workers"><varname>max_logical_replication_workers</varname></link>
> +    must be set to at least the number of subscriptions (for apply workers), plus
> +    some reserve for the table synchronization workers. Configuration parameter
> +    <link linkend="guc-max-worker-processes"><varname>max_worker_processes</varname></link>
> +    may need to be adjusted to accommodate for replication workers, at least (
> +    <link linkend="guc-max-logical-replication-workers"><varname>max_logical_replication_workers</varname></link>
> +    + <literal>1</literal>). Note, some extensions and parallel queries also
> +    take worker slots from <varname>max_worker_processes</varname>.
> +   </para>

Maybe do max_worker_processes in a new line like the rest.

Regards,
Samay
Microsoft
 

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Peter Smith
Date:
On Thu, Dec 8, 2022 at 10:49 AM samay sharma <smilingsamay@gmail.com> wrote:
>
...

> Thanks for the changes. See a few points of feedback below.
>

Patch v7 addresses this feedback. PSA.

> > +    <para>
> > +     For <emphasis>logical replication</emphasis>, <firstterm>publishers</firstterm>
> > +     (servers that do <link linkend="sql-createpublication"><command>CREATE PUBLICATION</command></link>)
> > +     replicate data to <firstterm>subscribers</firstterm>
> > +     (servers that do <link linkend="sql-createsubscription"><command>CREATE SUBSCRIPTION</command></link>).
> > +     Servers can also be publishers and subscribers at the same time. Note,
> > +     the following sections refers to publishers as "senders". The parameter
> > +     <literal>max_replication_slots</literal> has a different meaning for the
> > +     publisher and subscriber, but all other parameters are relevant only to
> > +     one side of the replication. For more details about logical replication
> > +     configuration settings refer to
> > +     <xref linkend="logical-replication-config"/>.
> > +    </para>
>
> The second last line seems a bit odd here. In my last round of feedback, I had meant to add the line "The parameter
...." onwards to the top of logical-replication-config.sgml. 
>
> What if we made the top of logical-replication-config.sgml like below?
>
> Logical replication requires several configuration options to be set. Most configuration options are relevant only on
oneside of the replication (i.e. publisher or subscriber). However, max_replication_slots is applicable on both sides
buthas different meanings on each side. 


OK. Moving this note is not quite following the same pattern as the
"streaming replication" intro blurb, but anyway it looks fine when
moved, so I've done as suggested.


>> OK. I copied the tablesync note back to config.sgml definition of
>> 'max_replication_slots' and removed the link as suggested. Frankly, I
>> also thought it is a bit strange that the max_replication_slots in the
>> “Sending Servers” section was describing this parameter for
>> “Subscribers”. OTOH, I did not want to split the definition in half so
>> instead, I’ve added another Subscriber <varlistentry> that just refers
>> back to this place. It looks like an improvement to me.
>
>
> Hmm, I agree this is a tricky scenario. However, to me, it seems odd to mention the parameter twice as this chapter
ofthe docs just lists each parameter and describes them. So, I'd probably remove the reference to it in the subscriber
section.We should describe it's usage in different places in the logical replication part of the docs (as we do). 

The 'max_replication_slots' is problematic because it is almost like
having 2 different GUCs that happen to have the same name. So I
preferred it also gets a mention in the “Subscriber” section to make
it obvious that it wears 2 hats, but IIUC you prefer that 2nd mention
is not present because typically each GUC should appear once only in
this chapter. TBH, I think both ways could be successfully argued for
or against -- so I’m just going to leave this as-is for now and let
the committer decide.

> > +   <para>
> > +    <link linkend="guc-max-logical-replication-workers"><varname>max_logical_replication_workers</varname></link>
> > +    must be set to at least the number of subscriptions (for apply workers), plus
> > +    some reserve for the table synchronization workers. Configuration parameter
> > +    <link linkend="guc-max-worker-processes"><varname>max_worker_processes</varname></link>
> > +    may need to be adjusted to accommodate for replication workers, at least (
> > +    <link linkend="guc-max-logical-replication-workers"><varname>max_logical_replication_workers</varname></link>
> > +    + <literal>1</literal>). Note, some extensions and parallel queries also
> > +    take worker slots from <varname>max_worker_processes</varname>.
> > +   </para>
>
> Maybe do max_worker_processes in a new line like the rest.

OK. Done as suggested.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Attachment

Re: PGDOCS - Logical replication GUCs - added some xrefs

From
samay sharma
Date:
Hi,

On Wed, Dec 7, 2022 at 9:20 PM Peter Smith <smithpb2250@gmail.com> wrote:
On Thu, Dec 8, 2022 at 10:49 AM samay sharma <smilingsamay@gmail.com> wrote:
>
...

> Thanks for the changes. See a few points of feedback below.
>

Patch v7 addresses this feedback. PSA.

> > +    <para>
> > +     For <emphasis>logical replication</emphasis>, <firstterm>publishers</firstterm>
> > +     (servers that do <link linkend="sql-createpublication"><command>CREATE PUBLICATION</command></link>)
> > +     replicate data to <firstterm>subscribers</firstterm>
> > +     (servers that do <link linkend="sql-createsubscription"><command>CREATE SUBSCRIPTION</command></link>).
> > +     Servers can also be publishers and subscribers at the same time. Note,
> > +     the following sections refers to publishers as "senders". The parameter
> > +     <literal>max_replication_slots</literal> has a different meaning for the
> > +     publisher and subscriber, but all other parameters are relevant only to
> > +     one side of the replication. For more details about logical replication
> > +     configuration settings refer to
> > +     <xref linkend="logical-replication-config"/>.
> > +    </para>
>
> The second last line seems a bit odd here. In my last round of feedback, I had meant to add the line "The parameter .... " onwards to the top of logical-replication-config.sgml.
>
> What if we made the top of logical-replication-config.sgml like below?
>
> Logical replication requires several configuration options to be set. Most configuration options are relevant only on one side of the replication (i.e. publisher or subscriber). However, max_replication_slots is applicable on both sides but has different meanings on each side.


OK. Moving this note is not quite following the same pattern as the
"streaming replication" intro blurb, but anyway it looks fine when
moved, so I've done as suggested.


>> OK. I copied the tablesync note back to config.sgml definition of
>> 'max_replication_slots' and removed the link as suggested. Frankly, I
>> also thought it is a bit strange that the max_replication_slots in the
>> “Sending Servers” section was describing this parameter for
>> “Subscribers”. OTOH, I did not want to split the definition in half so
>> instead, I’ve added another Subscriber <varlistentry> that just refers
>> back to this place. It looks like an improvement to me.
>
>
> Hmm, I agree this is a tricky scenario. However, to me, it seems odd to mention the parameter twice as this chapter of the docs just lists each parameter and describes them. So, I'd probably remove the reference to it in the subscriber section. We should describe it's usage in different places in the logical replication part of the docs (as we do).

The 'max_replication_slots' is problematic because it is almost like
having 2 different GUCs that happen to have the same name. So I
preferred it also gets a mention in the “Subscriber” section to make
it obvious that it wears 2 hats, but IIUC you prefer that 2nd mention
is not present because typically each GUC should appear once only in
this chapter. TBH, I think both ways could be successfully argued for
or against -- so I’m just going to leave this as-is for now and let
the committer decide.

Sounds fair.

I don't have any other feedback. This looks good to me.

Also, I don't see this patch in the 2023/01 commitfest. Might be worth moving to that one.

Regards,
Samay
Microsoft
 

> > +   <para>
> > +    <link linkend="guc-max-logical-replication-workers"><varname>max_logical_replication_workers</varname></link>
> > +    must be set to at least the number of subscriptions (for apply workers), plus
> > +    some reserve for the table synchronization workers. Configuration parameter
> > +    <link linkend="guc-max-worker-processes"><varname>max_worker_processes</varname></link>
> > +    may need to be adjusted to accommodate for replication workers, at least (
> > +    <link linkend="guc-max-logical-replication-workers"><varname>max_logical_replication_workers</varname></link>
> > +    + <literal>1</literal>). Note, some extensions and parallel queries also
> > +    take worker slots from <varname>max_worker_processes</varname>.
> > +   </para>
>
> Maybe do max_worker_processes in a new line like the rest.

OK. Done as suggested.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Peter Smith
Date:
On Sat, Dec 10, 2022 at 5:10 AM samay sharma <smilingsamay@gmail.com> wrote:
>

>
> I don't have any other feedback. This looks good to me.
>
> Also, I don't see this patch in the 2023/01 commitfest. Might be worth moving to that one.
>

Hmm, it was already recorded in the 2022-11 commitfest [1], so I
assumed it would just carry forward to the next one.

Anyway, I've added it again to 2023-01 commitfest [2]. Thanks for telling me.

------
[1] 2022-11 CF - https://commitfest.postgresql.org/40/3959/
[2] 2023-01 CF - https://commitfest.postgresql.org/41/4061/

Kind Regards,
Peter Smith.
Fujitsu Australia.



Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Tom Lane
Date:
Peter Smith <smithpb2250@gmail.com> writes:
> On Sat, Dec 10, 2022 at 5:10 AM samay sharma <smilingsamay@gmail.com> wrote:
>> Also, I don't see this patch in the 2023/01 commitfest. Might be worth moving to that one.

> Hmm, it was already recorded in the 2022-11 commitfest [1], so I
> assumed it would just carry forward to the next one.

Ian is still working on closing out the November 'fest :-(.
I suspect that in a day or so that one will get moved, and
you will have duplicate entries in the January 'fest.

            regards, tom lane



Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Alvaro Herrera
Date:
On 2022-Dec-07, samay sharma wrote:

> On Tue, Dec 6, 2022 at 11:12 PM Peter Smith <smithpb2250@gmail.com> wrote:

> > OK. I copied the tablesync note back to config.sgml definition of
> > 'max_replication_slots' and removed the link as suggested. Frankly, I
> > also thought it is a bit strange that the max_replication_slots in the
> > “Sending Servers” section was describing this parameter for
> > “Subscribers”. OTOH, I did not want to split the definition in half so
> > instead, I’ve added another Subscriber <varlistentry> that just refers
> > back to this place. It looks like an improvement to me.
> 
> Hmm, I agree this is a tricky scenario. However, to me, it seems odd to
> mention the parameter twice as this chapter of the docs just lists each
> parameter and describes them. So, I'd probably remove the reference to it
> in the subscriber section. We should describe it's usage in different
> places in the logical replication part of the docs (as we do).

I agree this is tricky.  However, because they essentially have
completely different behaviors on each side, and because we're
documenting each side separately, to me it makes more sense to document
each behavior separately, so I've split it.  I also added mention at
each side that the other one exists.  My rationale is that a user is
likely going to search for stuff to set on one side first, then for
stuff to set on the other side.  So doing it this way maximizes
helpfulness (or so I hope anyway).  I also added a separate index entry.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"I love the Postgres community. It's all about doing things _properly_. :-)"
(David Garamond)



Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Alvaro Herrera
Date:
On 2022-Dec-11, Tom Lane wrote:

> Ian is still working on closing out the November 'fest :-(.
> I suspect that in a day or so that one will get moved, and
> you will have duplicate entries in the January 'fest.

I've marked both as committed.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Here's a general engineering tip: if the non-fun part is too complex for you
to figure out, that might indicate the fun part is too ambitious." (John Naylor)
https://postgr.es/m/CAFBsxsG4OWHBbSDM%3DsSeXrQGOtkPiOEOuME4yD7Ce41NtaAD9g%40mail.gmail.com



Re: PGDOCS - Logical replication GUCs - added some xrefs

From
Peter Smith
Date:
On Tue, Dec 13, 2022 at 6:25 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Dec-07, samay sharma wrote:
>
> > On Tue, Dec 6, 2022 at 11:12 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> > > OK. I copied the tablesync note back to config.sgml definition of
> > > 'max_replication_slots' and removed the link as suggested. Frankly, I
> > > also thought it is a bit strange that the max_replication_slots in the
> > > “Sending Servers” section was describing this parameter for
> > > “Subscribers”. OTOH, I did not want to split the definition in half so
> > > instead, I’ve added another Subscriber <varlistentry> that just refers
> > > back to this place. It looks like an improvement to me.
> >
> > Hmm, I agree this is a tricky scenario. However, to me, it seems odd to
> > mention the parameter twice as this chapter of the docs just lists each
> > parameter and describes them. So, I'd probably remove the reference to it
> > in the subscriber section. We should describe it's usage in different
> > places in the logical replication part of the docs (as we do).
>
> I agree this is tricky.  However, because they essentially have
> completely different behaviors on each side, and because we're
> documenting each side separately, to me it makes more sense to document
> each behavior separately, so I've split it.  I also added mention at
> each side that the other one exists.  My rationale is that a user is
> likely going to search for stuff to set on one side first, then for
> stuff to set on the other side.  So doing it this way maximizes
> helpfulness (or so I hope anyway).  I also added a separate index entry.
>

LGTM. Thank you for pushing this.

------
Kind Regards,
Peter Smith.
Fujitsu Australia.