Dear Peter, Thank you for reviewing! PSA new patch set.
> 1. > It will be better if all the references follow a consistent pattern: > > Rule 1 - IMO it is quite important/necessary for these option name > “XXX” (see below) to be rendered using <literal> markup rather than > just plain text font. Unfortunately, I don't know how to do that using > xref labels. If you can figure out some way to do it then great, > otherwise I feel it is better just remove all those xreflabels and > instead create the links like this: > > <link > linkend="sql-createsubscription-with-XXX"><literal>XXX</literal></link> > option I have not known the better way, so I followed that. > Rule 2 – Try to keep consistent phrasing like "XXX option" or "XXX > parameter" (whatever is appropriate for the neighbouring text) Better suggestion. > 2. > I think you can extend this patch similarly to add IDs for the WITH > parameters of CREATE PUBLICATION. For example, I saw a couple of > places where referencing the 'publish' parameter might be useful. This suggestions exceeds initial motivation, but I made a patch. See 0002. > ====== > Commit message > > 3. > Currently, there is nothing. Added. > ====== > doc/src/sgml/config.sgml > > 4. (Section 20.17 Developer Options -- logical_replication_mode) > > - <literal>streaming</literal> option (see optional parameters set by > - <link linkend="sql-createsubscription"><command>CREATE > SUBSCRIPTION</command></link>) > + <xref linkend="sql-createsubscription-with-streaming"/> option > + (see optional parameters set by <link > linkend="sql-createsubscription"><command>CREATE > SUBSCRIPTION</command></link>) > > Since we now have a direct link to the option, I think the rest of > that sentence can now be a bit simpler. YMMV. > > SUGGESTION (per my general comment about links/fonts) > ... if the <link > linkend="sql-createsubscription-with-streaming"><literal>streaming</literal> > </link> > option of <link linkend="sql-createsubscription"><command>CREATE > SUBSCRIPTION</command></link> is enabled, otherwise, serialize each > change. Changed. Moreover, I reworded from "option" to "parameter" because It has already been used in the file. > ====== > doc/src/sgml/logical-replication. > > 5. (Section 31.2 Subscription) > > - <literal>streaming</literal> option (see optional parameters set by > - <link linkend="sql-createsubscription"><command>CREATE > SUBSCRIPTION</command></link>) > + <xref linkend="sql-createsubscription-with-streaming"/> option > + (see optional parameters set by <link > linkend="sql-createsubscription"><command>CREATE > SUBSCRIPTION</command></link>) > > For consistency with everything else, I think only the word “binary > should be the link. > > SUGGESTION > See the <link > linkend="sql-createsubscription-with-binary"><literal>binary</literal></link> > option ... You seemed to copy wrong diffs, but your point was right, fixed. > 6. (Section 31.2.3 Examples) > > - restrictive. See the <link > linkend="sql-createsubscription-binary"><literal>binary</literal> > + restrictive. See the <link > linkend="sql-createsubscription-with-binary"><literal>binary</literal> > > SUGGESTION (per my general comment about links/fonts, and also added > word "option") > <link > linkend="sql-createsubscription-with-slot-name"><literal>slot_name</literal> > </link> > option. You seemed to copy wrong diffs, but I fixed. > 7. (Section 31.5 Conflicts) > > - subscription can be used with the > <literal>disable_on_error</literal> option. > - Then, you can use > <function>pg_replication_origin_advance()</function> function > - with the <parameter>node_name</parameter> (i.e., > <literal>pg_16395</literal>) > + subscription can be used with the <xref > linkend="sql-createsubscription-with-disable-on-error"/> > + option. Then, you can use > <function>pg_replication_origin_advance()</function> > + function with the <parameter>node_name</parameter> (i.e., > <literal>pg_16395</literal>) > > SUGGESTION (per my general comment about links/fonts) > <link > linkend="sql-createsubscription-with-disable-on-error"><literal>disable_on_er > ror</literal></link> Fixed. > doc/src/sgml/ref/alter_subscription.sgml > > > 8. (Description) > > - <literal>two_phase</literal> commit enabled, > - unless <literal>copy_data</literal> is <literal>false</literal>. > + <link linkend="sql-createsubscription-with-two-phase"> commit > enabled</link>, > + unless <xref linkend="sql-createsubscription-with-copy-data"/> is > <literal>false</literal>. > > I think the "two_phase" was rendering wrongly because there was a > mixup of link/xref. Suggest fix it like below: > > SUGGESTION (per my general comment about links/fonts) > <link > linkend="sql-createsubscription-with-two-phase"><literal>two_phase</literal > ></link> > commit enabled, unless <link > linkend="sql-createsubscription-with-copy-data"><literal>copy_data</literal> > </link> > is <literal>false</literal>. Good detection. Fixed. > 9. (copy_data) > > - <literal>origin</literal> parameter. > + <xref linkend="sql-createsubscription-with-origin"/> parameter. > > SUGGESTION (per my general comment about links/fonts) > <link > linkend="sql-createsubscription-with-origin"><literal>origin</literal></link> > parameter. Fixed. > 10. > <para> > - See the <link > linkend="sql-createsubscription-binary"><literal>binary</literal> > + See the <link > linkend="sql-createsubscription-with-binary"><literal>binary</literal> > > Everything nearby was called a "parameter" so I recommend to change > "binary option" to "binary parameter" here too and move that word > outside the link. > > SUGGESTION (per my general comment about links/fonts) > See the <link > linkend="sql-createsubscription-with-binary"><literal>binary</literal></link> > parameter of ... Fixed. > 11 (SET) > > - are <literal>slot_name</literal>, > - <literal>synchronous_commit</literal>, > - <literal>binary</literal>, <literal>streaming</literal>, > - <literal>disable_on_error</literal>, and > + are <xref linkend="sql-createsubscription-with-slot-name"/>, > + <xref linkend="sql-createsubscription-with-synchronous-commit"/>, > + <literal>binary</literal>, <xref > linkend="sql-createsubscription-with-streaming"/>, > + <xref linkend="sql-createsubscription-with-disable-on-error"/>, and > > Modify so all the fonts are <literal>. Also, the binary link and > origin links were added. I know you said you chose to do that because > they are already linked previously on this page, but in practice, it > looked strange when rendered where only those ones were missing as > links from this long list. > > SUGGESTION (per my general comment about links/fonts) > The parameters that can be altered are > <link > linkend="sql-createsubscription-with-slot-name"><literal>slot_name</literal> > </link>, > <link > linkend="sql-createsubscription-with-synchronous-commit"><literal>synchron > ous_commit</literal></link>, > <link > linkend="sql-createsubscription-with-binary"><literal>binary</literal></link> > , > <link > linkend="sql-createsubscription-with-streaming"><literal>streaming</literal> > </link>, > <link > linkend="sql-createsubscription-with-disable-on-error"><literal>disable_on_er > ror</literal></link>, > and > <link > linkend="sql-createsubscription-with-origin"><literal>origin</literal></link>. Fixed. > doc/src/sgml/ref/create_subscription.sgml > > 12. > I think all those xreflabels can be removed. As per my general > comment, the references to the WITH option should use a <literal> font > for the option name, but then I was unable to get that working using > xreflabels. So AFAIK those xreflabels are unused (unless they have > some other purpose that I don't know about). They are no longer used, so removed. > 13. > Sometimes the WITH parameters reference to each other on this page. I > wasn’t sure if we should cross-reference within the same page. What do > you think? It might be useful, or OTOH it might be overkill to have > too many links. > > e.g. connect refers to -- create_slot, enabled, copy_data > > e.g. a lot_name refers to -- create_slot, enabled > > e.g. binary refers to -- copy_data > > e.g. copy_data refers to -- origin > > e.g. origin refers to -- copy_data I have not added links because it was in the same page and I thought it was overkill. I checked a few reference pages, e.g. create_table.sgml and create_type.sgml, but I could not find any links that refer varlistentry in the same page (except links for <sectN>). So I kept them. > doc/src/sgml/ref/pg_dump.sgml > > 14. (Section II. PG client applications -- pg_dump) > > - <literal>two_phase</literal> option will be automatically enabled by the > - subscriber if the subscription had been originally created with > - <literal>two_phase = true</literal> option. > + <xref linkend="sql-createsubscription-with-two-phase"/> option will be > + automatically enabled by the subscriber if the subscription had been > + originally created with <literal>two_phase = true</literal> option. > > SUGGESTION (per my general comment about links/fonts) > <link > linkend="sql-createsubscription-with-two-phase"><literal>two_phase</literal > ></link> > option Fixed. Besides, I have added a missing reference related with "CONNECTION". Best Regards, Hayato Kuroda FUJITSU LIMITED
v2-0001-Add-XML-ID-attributes-to-create_subscription.sgml.patch
Description: v2-0001-Add-XML-ID-attributes-to-create_subscription.sgml.patch
v2-0002-Add-XML-ID-attributes-to-create_publication.sgml.patch
Description: v2-0002-Add-XML-ID-attributes-to-create_publication.sgml.patch
