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

Dear hackers, (CC: reviewers of copy-binary patch)

This is an follow-up thread of [1]. PSA patch that adds an attributes.

By ecb6965, an XML ID attribute is added only one varlistentry in create_subscription.sgml.
But according to the commit 78ee60 and related discussions [2], [3], it is worth
adding ID attribute to other entries. This patch adds them.

Moreover, I have added some references to parameters from pre-existing documents.
Only entries that are referred from other files have XREFLABEL attribute.

Basically I detected the to-be-added position by:

1. Grepped subscription options, e.g. <literal>two_phase</literal>
2. Found a first place of above detection in each sgml files.
3. Replaced them link, e.g. <xref linkend="sql-createsubscription-with-two-phase"/>.

"XXX = YYY" style was not replaced because there are few links of its style for now.

[1]: https://www.postgresql.org/message-id/flat/CAGPVpCQvAziCLknEnygY0v1-KBtg+Om-9JHJYZOnNPKFJPompw@mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAB8KJ=jpuQU9QJe4+RgWENrK5g9jhoysMw2nvTN_esoOU0=a_w@mail.gmail.com
[3]: https://www.postgresql.org/message-id/3bac458c-b121-1b20-8dea-0665986faa40@gmx.de

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

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

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

Here are review comments for v2-0001

======
Commit Message

1.
In commit ecb696, an XML ID attribute was added to only one varlistentry,
creating inconsistency with the commit 78ee60. This commit adds XML ID
attributes to all varlistentries in create_subscritpion.sgml for consistency.
Additionally, links are added to refer subscription options, enhancing the
readability of documents.

~

1a.
Typo: create_subscritpion.sgml

~

1b.
"to refer subscription options" --> "to refer to the subscription options"

======
doc/src/sgml/config.sgml

2.
-        <literal>streaming</literal> option (see optional parameters set by
-        <link linkend="sql-createsubscription"><command>CREATE
SUBSCRIPTION</command></link>)
+        <link linkend="sql-createsubscription-with-streaming"><literal>streaming</literal></link>
+        parameter of <link
linkend="sql-createsubscription"><command>CREATE
SUBSCRIPTION</command></link>

Now, this link says "streaming parameter", but the very next paragraph
refers to "streaming option". I think it is better to keep them the
same (e.g. both say "streaming option").

======
doc/src/sgml/ref/alter_subscription.sgml

The SKIP part says "... enabling two_phase on subscriber.". I thought
there could be a link for "two_phase" here (also "on subscriber" -->
"on the subscriber").

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

Here are review comments for v2-0002

======
doc/src/sgml/logical-replication.sgml

1.
I am not sure your convention to only give the link to the FIRST
reference on a page is good in all case. Maybe that rule is OK for
multiple references all in the same sub-section but when they are in
different sub-sections (even on one page) I think it would be better
to include the extra links.

1a.
For example, Section 33.3 (Row Filter) refers to
"publish_via_partition_root" lots of times across multiple subsections
– So it is not convenient to have to scroll around looking in
different sections for the topmost reference which has the link.

1b.
Also in Section 33.3 (Row Filter), there are a couple of places you
could link to "publish" parameter on this page.

~~~

2.
I thought was a missing link in 31.7.1 (Architecture/Initial Snapshot)
which could've linked to the "publish" parameter.


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

Dear Peter,

Thank you for reviewing! New patch set will be attached in later mail.

> ======
> Commit Message
> 
> 1.
> In commit ecb696, an XML ID attribute was added to only one varlistentry,
> creating inconsistency with the commit 78ee60. This commit adds XML ID
> attributes to all varlistentries in create_subscritpion.sgml for consistency.
> Additionally, links are added to refer subscription options, enhancing the
> readability of documents.
> 
> ~
> 
> 1a.
> Typo: create_subscritpion.sgml

Fixed.

> 1b.
> "to refer subscription options" --> "to refer to the subscription options"

Fixed.

> 2.
> -        <literal>streaming</literal> option (see optional parameters set by
> -        <link linkend="sql-createsubscription"><command>CREATE
> SUBSCRIPTION</command></link>)
> +        <link
> linkend="sql-createsubscription-with-streaming"><literal>streaming</literal>
> </link>
> +        parameter of <link
> linkend="sql-createsubscription"><command>CREATE
> SUBSCRIPTION</command></link>
> 
> Now, this link says "streaming parameter", but the very next paragraph
> refers to "streaming option". I think it is better to keep them the
> same (e.g. both say "streaming option").

I missed just next paragraph, I thought :-(.
Reverted the change, now it is called as "streaming option"

> doc/src/sgml/ref/alter_subscription.sgml
> 
> The SKIP part says "... enabling two_phase on subscriber.". I thought
> there could be a link for "two_phase" here (also "on subscriber" -->
> "on the subscriber").

Added.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Peter,

Thank you for reviewing! PSA new version.

> doc/src/sgml/logical-replication.sgml
> 
> 1.
> I am not sure your convention to only give the link to the FIRST
> reference on a page is good in all case. Maybe that rule is OK for
> multiple references all in the same sub-section but when they are in
> different sub-sections (even on one page) I think it would be better
> to include the extra links.

Sounds better for readability.

> 1a.
> For example, Section 33.3 (Row Filter) refers to
> "publish_via_partition_root" lots of times across multiple subsections
> – So it is not convenient to have to scroll around looking in
> different sections for the topmost reference which has the link.

Added only two links because almost lines were in the same sub-section(Examples).
Did it match with your expectation?

> 1b.
> Also in Section 33.3 (Row Filter), there are a couple of places you
> could link to "publish" parameter on this page.

IIUC there was only one point to add the link, but added.

Also, I have added further links for "FOR ALL TABLES" and "FOR TABLES IN SCHEMA" clauses.

> 2.
> I thought was a missing link in 31.7.1 (Architecture/Initial Snapshot)
> which could've linked to the "publish" parameter.
>

Added.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Hi Kuroda-san. Here are my review comments for both patches v3-0001 and v3-0002.


////////
v3-0001
////////

This patch looks good, but I think there are a couple of other places
where you could add links:

~~~

1.1 doc/src/sgml/logical-replication.sgml (31.5 Conflicts)

"When the streaming mode is parallel, the finish LSN ..."

Maybe you can add a "streaming" link there.

~~~

1.2. doc/src/sgml/logical-replication.sgml (31.5 31.8. Monitoring)

"Moreover, if the streaming transaction is applied in parallel, there
may be additional parallel apply workers."

Maybe you can add a "streaming" link there.


////////
v3-0002
////////

There is one bug, and I think there are a couple of other places where
you could add links:

~~~

2.1 doc/src/sgml/logical-replication.sgml (31.4. Column Lists blurb)

For partitioned tables, the publication parameter
publish_via_partition_root determines which column list is used.

~

Maybe you can add a "publish_via_partition_root" link there.

~~~

2.2 doc/src/sgml/logical-replication.sgml (31.6. Restrictions)

Publications can also specify that changes are to be replicated using
the identity and schema of the partitioned root table instead of that
of the individual leaf partitions in which the changes actually
originate (see CREATE PUBLICATION).

~

Maybe that text can be changed now to say something like "(see
publish_via_partition_root parameter of CREATE PUBLICATION)” -- so
only the parameter part has the link, not the CREATE PUBLICATION part.

~~~

2.3 doc/src/sgml/logical-replication.sgml (31.9. Security)

+   subscription <link
linkend="sql-createpublication-for-all-tables"><literal>FOR ALL
TABLES</literal></link>
+   or <link linkend="sql-createpublication-for-tables-in-schema"><literal>FOR
TABLES IN SCHEMA</literal></link><literal>FOR TABLES IN
SCHEMA</literal>
+   only when superusers trust every user permitted to create a non-temp table
+   on the publisher or the subscriber.

There is a cut/paste typo here -- it renders badly with "FOR TABLES IN
SCHEMA" appearing 2x.

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

Dear Peter,

Thank you for reviewing. PSA new version.

> ////////
> v3-0001
> ////////
> 
> This patch looks good, but I think there are a couple of other places
> where you could add links:
> 
> ~~~
> 
> 1.1 doc/src/sgml/logical-replication.sgml (31.5 Conflicts)
> 
> "When the streaming mode is parallel, the finish LSN ..."
> 
> Maybe you can add a "streaming" link there.

Added. It could not be detected because this is not tagged as <literal>.

> 1.2. doc/src/sgml/logical-replication.sgml (31.5 31.8. Monitoring)
> 
> "Moreover, if the streaming transaction is applied in parallel, there
> may be additional parallel apply workers."
> 
> Maybe you can add a "streaming" link there.

Added.

> ////////
> v3-0002
> ////////
> 
> There is one bug, and I think there are a couple of other places where
> you could add links:
> 
> ~~~
> 
> 2.1 doc/src/sgml/logical-replication.sgml (31.4. Column Lists blurb)
> 
> For partitioned tables, the publication parameter
> publish_via_partition_root determines which column list is used.
> 
> ~
> 
> Maybe you can add a "publish_via_partition_root" link there.

Added. I'm not sure why I missed it...

> 2.2 doc/src/sgml/logical-replication.sgml (31.6. Restrictions)
> 
> Publications can also specify that changes are to be replicated using
> the identity and schema of the partitioned root table instead of that
> of the individual leaf partitions in which the changes actually
> originate (see CREATE PUBLICATION).
> 
> ~
> 
> Maybe that text can be changed now to say something like "(see
> publish_via_partition_root parameter of CREATE PUBLICATION)” -- so
> only the parameter part has the link, not the CREATE PUBLICATION part.

Seems better, added.

> 2.3 doc/src/sgml/logical-replication.sgml (31.9. Security)
> 
> +   subscription <link
> linkend="sql-createpublication-for-all-tables"><literal>FOR ALL
> TABLES</literal></link>
> +   or <link
> linkend="sql-createpublication-for-tables-in-schema"><literal>FOR
> TABLES IN SCHEMA</literal></link><literal>FOR TABLES IN
> SCHEMA</literal>
> +   only when superusers trust every user permitted to create a non-temp table
> +   on the publisher or the subscriber.
> 
> There is a cut/paste typo here -- it renders badly with "FOR TABLES IN
> SCHEMA" appearing 2x.
>

That's my fault, fixed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, Mar 28, 2023 at 2:04 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Peter,
>
> Thank you for reviewing. PSA new version.
>

v4-0001 LGTM

>
> > ////////
> > v3-0002
> > ////////
> >
>
> > 2.2 doc/src/sgml/logical-replication.sgml (31.6. Restrictions)
> >
> > Publications can also specify that changes are to be replicated using
> > the identity and schema of the partitioned root table instead of that
> > of the individual leaf partitions in which the changes actually
> > originate (see CREATE PUBLICATION).
> >
> > ~
> >
> > Maybe that text can be changed now to say something like "(see
> > publish_via_partition_root parameter of CREATE PUBLICATION)” -- so
> > only the parameter part has the link, not the CREATE PUBLICATION part.
>
> Seems better, added.
>

-     originate (see <link
linkend="sql-createpublication"><command>CREATE
PUBLICATION</command></link>).
+     originate (see <link
linkend="sql-createpublication-with-publish-via-partition-root"><literal>publish_via_partition_root</literal></link>
+     of <command>CREATE PUBLICATION</command>).

Hmm, my above-suggested wording was “publish_via_partition_root
parameter “ but it seems you (accidentally?) omitted the word
“parameter”.

Otherwise, the patch v4-0002 also LGTM

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

Dear Peter,

Thank you for prompt reply!

> Hmm, my above-suggested wording was “publish_via_partition_root
> parameter “ but it seems you (accidentally?) omitted the word
> “parameter”.

It is my carelessness, sorry for inconvenience. PSA new ones.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Thanks for this patch.

v5-0001 looks good to me.

v5-0002 looks good to me.

I've marked the CF entry [1] as "ready for committer".

------
[1] https://commitfest.postgresql.org/43/4256/

Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Mar 28, 2023 at 9:49 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Thank you for prompt reply!
>
> > Hmm, my above-suggested wording was “publish_via_partition_root
> > parameter “ but it seems you (accidentally?) omitted the word
> > “parameter”.
>
> It is my carelessness, sorry for inconvenience. PSA new ones.
>

In 0001, patch, I see a lot of long lines like below:
-   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 <link
linkend="sql-createsubscription-with-disable-on-error"><literal>disable_on_error</literal></link>

Isn't it better to move the link-related part to the next line
wherever possible? Currently, it looks bit odd.

Why 0002 patch is part of this thread? I thought here we want to add
'ids' to entries corresponding to Create Subscription as we have added
the one in commit ecb696.

--
With Regards,
Amit Kapila.

Dear Amit,

Thank you for reviewing! PSA new version.

> Isn't it better to move the link-related part to the next line
> wherever possible? Currently, it looks bit odd.

Previously I preferred not to add a new line inside the <link> tag, but it caused
long-line. So I adjusted them not to be too short/long length.

> Why 0002 patch is part of this thread? I thought here we want to add
> 'ids' to entries corresponding to Create Subscription as we have added
> the one in commit ecb696.
>

0002 was motivated by Peter's comment [1]. This exceeds the initial intention of
the patch, so I removed once.

[1]: https://www.postgresql.org/message-id/CAHut%2BPu%2B-OocYYhW9E0gxxqgfUb1yJ8jVQ4AZ0v-ud00s7TxEA%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, Mar 28, 2023 at 1:00 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Amit,
>
> Thank you for reviewing! PSA new version.
>
> > Isn't it better to move the link-related part to the next line
> > wherever possible? Currently, it looks bit odd.
>
> Previously I preferred not to add a new line inside the <link> tag, but it caused
> long-line. So I adjusted them not to be too short/long length.
>

There is no need to break the link line. See attached.

--
With Regards,
Amit Kapila.

Dear Amit-san,

> There is no need to break the link line. See attached.

I understood your saying. I think it's better.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, Mar 29, 2023 at 6:31 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Amit-san,
>
> > There is no need to break the link line. See attached.
>
> I understood your saying. I think it's better.
>

Pushed.

--
With Regards,
Amit Kapila.