Re: PGdoc: add missing ID attribute to create_subscription.sgml

Firstly, +1 for this patch. Directly jumping to the subscription
options makes it much easier to navigate in the documentation instead
of scrolling up and done in CREATE SUBSCRIPTION page looking for each
parameter. Already (just experimenting with this patch) it is
noticeably better.

~~

Anyway, here are my review comments for patch 0001

======
General

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

Rule 2 – Try to keep consistent phrasing like "XXX option" or "XXX
parameter" (whatever is appropriate for the neighbouring text)

~~~

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.

======
Commit message

3.
Currently, there is nothing.

======
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.

======
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 ...

~~~

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.

~~~

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_error</literal></link>

======
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>.

~~~

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.

~

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 ...

~~~

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>synchronous_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_error</literal></link>,
and
<link linkend="sql-createsubscription-with-origin"><literal>origin</literal></link>.

======
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).

~~~

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

======
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

------
Kind Regards,
Peter Smith.
Fujitsu Australia