Re: Conflict Detection and Resolution

On Fri, 30 Aug 2024 at 11:01, Nisha Moond <nisha(dot)moond412(at)gmail(dot)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