Re: PGDOCS - Logical replication GUCs - added some xrefs

On Thu, Dec 8, 2022 at 10:49 AM samay sharma <smilingsamay(at)gmail(dot)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.

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