From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Peter Smith' <smithpb2250(at)gmail(dot)com> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Euler Taveira <euler(at)eulerto(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, "Takamichi Osumi (Fujitsu)" <osumi(dot)takamichi(at)fujitsu(dot)com> |
Subject: | RE: PGdoc: add missing ID attribute to create_subscription.sgml |
Date: | 2023-03-24 11:27:48 |
Message-ID: | TYAPR01MB5866087E6347D160FF5EDD90F5849@TYAPR01MB5866.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Add-XML-ID-attributes-to-create_subscription.sgml.patch | application/octet-stream | 14.7 KB |
v2-0002-Add-XML-ID-attributes-to-create_publication.sgml.patch | application/octet-stream | 11.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | houzj.fnst@fujitsu.com | 2023-03-24 11:57:00 | RE: Initial Schema Sync for Logical Replication |
Previous Message | Peter Eisentraut | 2023-03-24 10:59:23 | Re: meson documentation build open issues |