Re: PGDOCS - Logical replication GUCs - added some xrefs

Hi,

On Wed, Dec 7, 2022 at 9:20 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:

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

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
>