RE: PGdoc: add missing ID attribute to create_subscription.sgml - Mailing list 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