Re: Conflict Detection and Resolution - Mailing list pgsql-hackers

On Fri, Sep 27, 2024 at 1:00 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.

This ordering was decided while implementing the 'conflict-detection
and logging' patch and thus perhaps should be maintained as same. The
ordering is insert, update and delete (different variants of these).
Please see a comment on it in [1] (comment #2).


[1]:https://www.postgresql.org/message-id/TYAPR01MB569224262F44875973FAF344F5B22%40TYAPR01MB5692.jpnprd01.prod.outlook.com


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

 I feel ordering of resolvers should be same as that of conflict
types, i.e. resolvers of insert variants first, then update variants,
then delete variants. But would like to know what others think on
this.

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