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

On Fri, 30 Aug 2024 at 11:01, Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> Here is the v11 patch-set. Changes are:
> 1) Updated conflict type names in accordance with the recent commit[1] as -
>   update_differ --> update_origin_differs
>   delete_differ --> delete_origin_differs
>
> 2) patch-001:
>  - Implemented the RESET command to restore the default resolvers as
> suggested in pt.2a & 2b in [2]

Few comments on 0001 patch:
1) Currently create subscription has WITH option before conflict
resolver, I felt WITH option can be after CONNECTION, PUBLICATION and
CONFLICT RESOLVER option and WITH option at the end:
 CreateSubscriptionStmt:
-                       CREATE SUBSCRIPTION name CONNECTION Sconst
PUBLICATION name_list opt_definition
+       CREATE SUBSCRIPTION name CONNECTION Sconst PUBLICATION
name_list opt_definition opt_resolver_definition
                                {
                                        CreateSubscriptionStmt *n =

makeNode(CreateSubscriptionStmt);
@@ -10696,6 +10702,7 @@ CreateSubscriptionStmt:
                                        n->conninfo = $5;
                                        n->publication = $7;
                                        n->options = $8;
+                                       n->resolvers = $9;
                                        $$ = (Node *) n;

2) Case sensitive:
2.a) Should conflict type be case insensitive:
CREATE SUBSCRIPTION sub3 CONNECTION 'dbname=postgres host=localhost
port=5432' PUBLICATION pub1 with (copy_data= true) CONFLICT RESOLVER
("INSERT_EXISTS" = 'error');
ERROR:  INSERT_EXISTS is not a valid conflict type

In few other places it is not case sensitive:
create publication pub1 with ( PUBLISH= 'INSERT,UPDATE,delete');
set log_min_MESSAGES TO warning ;

2.b) Similarly in case of conflict resolver too:
CREATE SUBSCRIPTION sub3 CONNECTION 'dbname=postgres host=localhost
port=5432' PUBLICATION pub1 with (copy_data= true) CONFLICT RESOLVER
("insert_exists" = 'erroR');
ERROR:  erroR is not a valid conflict resolver

3) Since there is only one key used to search, we can remove nkeys
variable and directly specify as 1:
+RemoveSubscriptionConflictBySubid(Oid subid)
+{
+       Relation        rel;
+       HeapTuple       tup;
+       TableScanDesc scan;
+       ScanKeyData skey[1];
+       int                     nkeys = 0;
+
+       rel = table_open(SubscriptionConflictId, RowExclusiveLock);
+
+       /*
+        * Search using the subid, this should return all conflict resolvers for
+        * this sub
+        */
+       ScanKeyInit(&skey[nkeys++],
+                               Anum_pg_subscription_conflict_confsubid,
+                               BTEqualStrategyNumber,
+                               F_OIDEQ,
+                               ObjectIdGetDatum(subid));
+
+       scan = table_beginscan_catalog(rel, nkeys, skey);

4) Currently we are including CONFLICT RESOLVER even if a subscription
with default CONFLICT RESOLVER is created, we can add the CONFLICT
RESOLVER option only for non-default subscription option:
+       /* add conflict resolvers, if any */
+       if (fout->remoteVersion >= 180000)
+       {
+               PQExpBuffer InQry = createPQExpBuffer();
+               PGresult   *res;
+               int                     i_confrtype;
+               int                     i_confrres;
+
+               /* get the conflict types and their resolvers from the
catalog */
+               appendPQExpBuffer(InQry,
+                                                 "SELECT confrtype, confrres "
+                                                 "FROM
pg_catalog.pg_subscription_conflict"
+                                                 " WHERE confsubid =
%u;\n", subinfo->dobj.catId.oid);
+               res = ExecuteSqlQuery(fout, InQry->data, PGRES_TUPLES_OK);
+
+               i_confrtype = PQfnumber(res, "confrtype");
+               i_confrres = PQfnumber(res, "confrres");
+
+               if (PQntuples(res) > 0)
+               {
+                       appendPQExpBufferStr(query, ") CONFLICT RESOLVER (");

5) Should remote_apply be apply_remote here as this is what is
specified in code:
+       <varlistentry
id="sql-createsubscription-params-with-conflict_resolver-remote-apply">
+        <term><literal>remote_apply</literal> (<type>enum</type>)</term>
+         <listitem>
+          <para>

6) I think this should be "It is the default resolver for update_origin_differs"
6.a)
+       <varlistentry
id="sql-createsubscription-params-with-conflict_resolver-remote-apply">
+        <term><literal>remote_apply</literal> (<type>enum</type>)</term>
+         <listitem>
+          <para>
+           This resolver applies the remote change. It can be used for
+           <literal>insert_exists</literal>, <literal>update_exists</literal>,
+           <literal>update_differ</literal> and
<literal>delete_differ</literal>.
+           It is the default resolver for <literal>insert_exists</literal> and
+           <literal>update_exists</literal>.
+          </para>
+         </listitem>
+       </varlistentry>

6.b)
+       <varlistentry
id="sql-createsubscription-params-with-conflict_type-update-differ">
+        <term><literal>update_differ</literal> (<type>enum</type>)</term>
+         <listitem>
+          <para>
+           This conflict occurs when updating a row that was previously

6.c)
+       <varlistentry
id="sql-createsubscription-params-with-conflict_resolver-remote-apply">
+        <term><literal>remote_apply</literal> (<type>enum</type>)</term>
+         <listitem>
+          <para>
+           This resolver applies the remote change. It can be used for
+           <literal>insert_exists</literal>, <literal>update_exists</literal>,
+           <literal>update_differ</literal> and
<literal>delete_differ</literal>.
+           It is the default resolver for <literal>insert_exists</literal> and
+           <literal>update_exists</literal>.

6.d)
+       <varlistentry
id="sql-createsubscription-params-with-conflict_resolver-remote-apply">
+        <term><literal>remote_apply</literal> (<type>enum</type>)</term>
+         <listitem>
+          <para>
+           This resolver applies the remote change. It can be used for
+           <literal>insert_exists</literal>, <literal>update_exists</literal>,
+           <literal>update_differ</literal> and
<literal>delete_differ</literal>.

Similarly this change should be done in other places too.

7)
7.a) Should delete_differ be changed to delete_origin_differs as that
is what is specified in the subscription commands:
+check_conflict_detection(void)
+{
+       if (!track_commit_timestamp)
+               ereport(WARNING,
+
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                               errmsg("conflict detection and
resolution could be incomplete due to disabled
track_commit_timestamp"),
+                               errdetail("Conflicts update_differ and
delete_differ cannot be detected, "
+                                                 "and the origin and
commit timestamp for the local row will not be logged."));

7.b) similarly here too:
+       <varlistentry
id="sql-createsubscription-params-with-conflict_type-delete-differ">
+        <term><literal>delete_differ</literal> (<type>enum</type>)</term>
+         <listitem>
+          <para>
+           This conflict occurs when deleting a row that was
previously modified
+           by another origin. Note that this conflict can only be detected when
+           <link
linkend="guc-track-commit-timestamp"><varname>track_commit_timestamp</varname></link>
+           is enabled on the subscriber. Currently, the delete is
always applied
+           regardless of the origin of the local row.
+          </para>
+         </listitem>
+       </varlistentry>

Similarly this change should be done in other places too.

8) ConflictTypeResolver should be added to typedefs.list to resolve
the pgindent issues:
8.a)
+static void
+parse_subscription_conflict_resolvers(List *stmtresolvers,
+
   ConflictTypeResolver * resolvers)

8.b) Similarly FormData_pg_subscription_conflict should also be added:
} FormData_pg_subscription_conflict;

/* ----------------
 * Form_pg_subscription_conflict corresponds to a pointer to a row with
 * the format of pg_subscription_conflict relation.
 * ----------------
 */
typedef FormData_pg_subscription_conflict * Form_pg_subscription_conflict;

Regards,
Vignesh