Hi, On Wed, Dec 7, 2022 at 9:20 PM Peter Smith <smithpb2...@gmail.com> wrote:
> 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. > 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 >
