Re: Conflict Detection and Resolution

Here are some review comments for v14-0001.

This is a WIP, but here are my comments for all the SGML parts.

(There will be some overlap here with comments already posted by Shveta)

======
1. file modes after applying the patch

mode change 100644 => 100755 doc/src/sgml/ref/alter_subscription.sgml
mode change 100644 => 100755 doc/src/sgml/ref/create_subscription.sgml

What's going on here? Why are those SGMLs changed to executable?

======
Commit message

2.
nit - a missing period in the first sentence
nit - typo /reseting/resetting/

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

3.
- <title>Conflicts</title>
+ <title>Conflicts and conflict resolution</title>

nit - change the capitalisation to "and Conflict Resolution" to match
other titles.

~~~

4.
+ Additional logging is triggered in various conflict scenarios,
each identified as a
+ conflict type, and the conflict statistics are collected (displayed in the
+ <link linkend="monitoring-pg-stat-subscription-stats"><structname>pg_stat_subscription_stats</structname></link>
view).
+ Users have the option to configure a
<literal>conflict_resolver</literal> for each
+ <literal>conflict_type</literal> when creating a subscription.
+ For more information on the supported
<literal>conflict_types</literal> detected and
+ <literal>conflict_resolvers</literal>, refer to section
+ <link linkend="sql-createsubscription-params-with-conflict-resolver"><literal>CONFLICT
RESOLVERS</literal></link>.
+

nit - "Additional logging is triggered" sounds strange. I reworded
this in the nits attachment. Please see if you approve.
nit - The "conflict_type" and "conflict_resolver" you are referring to
here are syntax elements of the CREATE SUBSCRIPTION, so here I think
they should just be called (without the underscores) "conflict type"
and "conflict resolver".
nit - IMO this would be better split into multiple paragraphs.
nit - There is no such section called "CONFLICT RESOLVERS". I reworded
this link text.

======
doc/src/sgml/monitoring.sgml

5.
The changes here all render with the link including the type "(enum)"
displayed, which I thought it unnecessary/strange.

For example:
See insert_exists (enum) for details about this conflict.

IIUC there is no problem here, but maybe the other end of the link
needed to define xreflabels. I have made the necessary modifications
in the create_subscription.sgml.

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

6.
+ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable>
CONFLICT RESOLVER ( <replaceable
class="parameter">conflict_type</replaceable> [= <replaceable
class="parameter">conflict_resolver</replaceable>] [, ...] )

This syntax seems wrong to me.

Currently, it says:
ALTER SUBSCRIPTION name CONFLICT RESOLVER ( conflict_type [=
conflict_resolver] [, ...] )

But, shouldn't that say:
ALTER SUBSCRIPTION name CONFLICT RESOLVER ( conflict_type =
conflict_resolver [, ...] )

~~~
7.
+ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable>
RESET CONFLICT RESOLVER FOR (<replaceable
class="parameter">conflict_type</replaceable>)

I can see that this matches the implementation, but I was wondering
why don't you permit resetting multiple conflict_types at the same
time. e.g. what if I want to reset some but not ALL?

~~~

nit - there are some minor whitespace indent problems in the SGML

~~~

8.
+ <varlistentry id="sql-altersubscription-params-conflict-resolver">
+ <term><literal>CONFLICT RESOLVER ( <replaceable
class="parameter">conflict_type</replaceable> [= <replaceable
class="parameter">conflict_resolver</replaceable>] [, ... ]
)</literal></term>
+ <listitem>
+ <para>
+ This clause alters either the default conflict resolvers or
those set by <xref linkend="sql-createsubscription"/>.
+ Refer to section <link
linkend="sql-createsubscription-params-with-conflict-resolver"><literal>CONFLICT
RESOLVERS</literal></link>
+ for the details on supported <literal>conflict_types</literal>
and <literal>conflict_resolvers</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="sql-altersubscription-params-conflict-type">
+ <term><replaceable class="parameter">conflict_type</replaceable></term>
+ <listitem>
+ <para>
+ The conflict type being reset to its default resolver setting.
+ For details on conflict types and their default resolvers, refer
to section <link
linkend="sql-createsubscription-params-with-conflict-resolver"><literal>CONFLICT
RESOLVERS</literal></link>
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>

This section seems problematic:
e.g the syntax seems wrong same as before.

~
There are other nits.
(I've given a rough fix in the nits attachment. Please see it and make
it better).

nit - why do you care if it is "either the default conflict resolvers
or those set...". Why not just say "current resolver"
nit - it does not mention 'conflict_resolver' type in the normal way
nit - there is no actual section called "CONFLICT RESOLVERS"
nit - the part that says "The conflict type being reset to its default
resolver setting." is bogus for this form of the ALTER statement.

~~~

9.
There is no description for the "RESET CONFLICT RESOLVER ALL"

~~~

10.
There is no description for the "RESET CONFLICT RESOLVER FOR (conflict_type)"

======
doc/src/sgml/ref/create_subscription.sgml

11. General - Order

+ <varlistentry id="sql-createsubscription-params-with-conflict-resolver">
+ <term><literal>CONFLICT RESOLVER ( <replaceable
class="parameter">conflict_type</replaceable> = <replaceable

nit - IMO this entire new entry about "CONFLICT RESOLVER" should
appear on the page *above* the "WITH" section, because that is the
order that it is defined in the CREATE SUBSCRIPTION syntax.

~~~

12. General - whitespace

nit - Much of this new section seems to have a slightly wrong
indentation in the SGML. Mostly it is out by 1 or 2 spaces.

~~~

13. General - ordering of conflict_type.

nit - Instead of just some apparent random order, let's put each
insert/update/delete conflict type in alphabetical order, so at least
users can find them where they would expect to find them.

~~~

14.
99. General - ordering of conflict_resolver

nit - ditto. Let's name these in alphabetical order. IMO it makes more
sense than the current random ordering.

~~~

15.
+ <para>
+ This optional clause specifies options for conflict resolvers
for different conflict_types.
+ </para>

nit - IMO we don't need the words "options for" here.

~~~

16.
+ <para>
+ The <replaceable class="parameter">conflict_type</replaceable>
and their default behaviour are listed below.

nit - sounded strange to me. reworded it slightly.

~~~

17.
+ <varlistentry
id="sql-createsubscription-params-with-conflict_type-insert-exists">

nit - Here, and for all other conflict types, add "xreflabel". See my
review comment #5 for the reason why.

~~~

18.
+ <para>
+ The <replaceable
class="parameter">conflict_resolver</replaceable> and their behaviour
+ are listed below. Users can use any of the following resolvers
for automatic conflict
+ resolution.
+ <variablelist>

nit - reworded this too, to be like the previous review comment.

~~~

19. General - readability.

19a.
IMO the information about what are the default resolvers for each
conflict type, and what resolvers are allowed for each conflict type
should ideally be documented in a tabular form.

Maybe all information is already present in the current document, but
it is certainly hard to easily see it.

As an example, I have added a table in this section. Maybe it is the
best placement for this table, but I gave it mostly how you can
present the same information so it is easier to read.

~
19b.
Bug. In doing this exercise I discovered there are 2 resolvers
("error" and "apply_remote") that both claim to be defaults for the
same conflict types.

They both say:

+ It is the default resolver for <literal>insert_exists</literal> and
+ <literal>update_exists</literal>.

Anyway, this demonstrates that the current information was hard to read.

I can tell from the code implementation what the document was supposed
to say, but I will leave it to the patch authors to fix this one.
(e.g. "apply_remote" says the wrong defaults)

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