On Thu, Dec 8, 2022 at 10:49 AM samay sharma <smilingsa...@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. > > + <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
v7-0001-Logical-replication-GUCs-links-and-tidy.patch
Description: Binary data
