Re: Conflict Detection and Resolution

On Tue, Sep 3, 2024 at 7:42 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:

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) This command crashes:
ALTER SUBSCRIPTION name RESET CONFLICT RESOLVER FOR NULL;
#0 __strcmp_avx2 () at ../sysdeps/x86_64/multiarch/strcmp-avx2.S:116
#1 0x000055c67270600a in ResetConflictResolver (subid=16404,
conflict_type=0x0) at conflict.c:744
#2 0x000055c67247e0c3 in AlterSubscription (pstate=0x55c6748ff9d0,
stmt=0x55c67497dfe0, isTopLevel=true) at subscriptioncmds.c:1664

+ | ALTER SUBSCRIPTION name RESET CONFLICT
RESOLVER FOR conflict_type
+ {
+ AlterSubscriptionStmt *n =
+
makeNode(AlterSubscriptionStmt);
+
+ n->kind =
ALTER_SUBSCRIPTION_RESET_CONFLICT_RESOLVER;
+ n->subname = $3;
+ n->conflict_type = $8;
+ $$ = (Node *) n;
+ }
+ ;
+conflict_type:
+ Sconst
{ $$ = $1; }
+ | NULL_P
{ $$ = NULL; }
;

May be conflict_type should be changed to:
+conflict_type:
+ Sconst
{ $$ = $1; }
;

Fixed.

2) Conflict resolver is not shown in describe command:
postgres=# \dRs+

List of subscriptions
Name | Owner | Enabled | Publication | Binary | Streaming |
Two-phase commit | Disable on error | Origin | Password required | Run
as owner? | Failover | Synchronous commit | Conninfo
| Skip LSN

------+---------+---------+-------------+--------+-----------+------------------+------------------+--------+-------------------+---------------+----------+--------------------+----------------------------------
--------+----------
sub1 | vignesh | t | {pub1} | f | off | d
| f | any | t | f
| f | off | dbname=postgres host=localhost po
rt=5432 | 0/0
sub2 | vignesh | t | {pub1} | f | off | d
| f | any | t | f
| f | off | dbname=postgres host=localhost po
rt=5432 | 0/0
(2 rows)

Fixed.

3) Tab completion is not handled to include Conflict resolver:
postgres=# alter subscription sub1
ADD PUBLICATION CONNECTION DISABLE DROP
PUBLICATION ENABLE OWNER TO REFRESH
PUBLICATION RENAME TO SET SKIP (

Fixed

On Tue, Sep 3, 2024 at 8:33 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:

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;

Changed as suggested.

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

Fixed.

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);

Fixed.

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 (");

Fixed.

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.

Fixed.

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.

Fixed.

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)

Fixed.

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;

Fixed.

On Mon, Sep 9, 2024 at 7:28 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:

> On Fri, Sep 6, 2024 at 2:05 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> >
> > Thank you for your feedback, Shveta. I've addressed both sets of
> comments you provided.
>
> Thanks for the patches. I am reviewing v12-patch001, it is WIP. But
> please find first set of comments:
>
> 1)
> src/sgml/logical-replication.sgml:
> + Users have the option to configure a conflict_resolver
>
> Full stop for previous line is missing.
>

Fixed

>
> 2)
> + For more information on the conflict_types detected and the
> supported conflict_resolvers, refer to section CONFLICT RESOLVERS.
>
> We may change to :
> For more information on the supported conflict_types and
> conflict_resolvers, refer to section CONFLICT RESOLVERS.
>
>
>
Fixed.

> 3)
> src/backend/commands/subscriptioncmds.c:
> Line removed. This change is not needed.
>
> static void CheckAlterSubOption(Subscription *sub, const char *option,
> bool slot_needs_update, bool isTopLevel);
> -
>
>
Fixed.

> 4)
>
> Let's stick to the same comments format as the rest of the file i.e.
> first letter in caps.
>
> + /* first initialise the resolvers with default values */
>
> first --> First
> initialise --> initialize
>
> Same for below comments:
> + /* validate the conflict type and resolver */
> + /* update the corresponding resolver for the given conflict type */
>
> Please verify the rest of the file for the same.
>
>
Fixed.

> 5)
> Please add below in header of parse_subscription_conflict_resolvers
> (similar to parse_subscription_options):
>
> * This function will report an error if mutually exclusive options
> are specified.
>

Fixed.

>
> 6)
> + * Warn users if prerequisites are not met.
> + * Initialize with default values.
> + */
> + if (stmt->resolvers)
> + conf_detection_check_prerequisites();
> +
>
> Would it be better to move the above call inside
> parse_subscription_conflict_resolvers(), then we will have all
> resolver related stuff at one place?
> Irrespective of whether we move it or not, please remove 'Initialize
> with default values.' from above as that is now not done here.
>
>
Fixed.

On Mon, Sep 9, 2024 at 7:45 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:

>
> Thanks for the patches. I tested the v12-0001 patch, and here are my
> comments:
>
> 1) An unexpected error occurs when attempting to alter the resolver
> for multiple conflict_type(s) in ALTER SUB...CONFLICT RESOLVER
> command. See below examples :
>
> postgres=# alter subscription sub2 CONFLICT RESOLVER
> (update_exists=keep_local, delete_missing=error,
> update_origin_differs=error);
> ERROR: unrecognized node type: 1633972341
>
> postgres=# alter subscription sub2 CONFLICT RESOLVER (
> update_origin_differs=error, update_exists=error);
> ERROR: unrecognized node type: 1633972341
>
> postgres=# alter subscription sub2 CONFLICT RESOLVER (
> delete_origin_differs=error, delete_missing=error);
> ERROR: unrecognized node type: 1701602660
>
> postgres=# alter subscription sub2 CONFLICT RESOLVER
> (update_exists=keep_local, delete_missing=error);
> ALTER SUBSCRIPTION
>
> -- It appears that the error occurs only when at least two conflict
> types belong to the same category, either UPDATE or DELETE.
>
>
Fixed this

> 2) Given the above issue, it would be beneficial to add a test in
> subscription.sql to cover cases where all valid conflict types are set
> with appropriate resolvers in both the ALTER and CREATE commands.
>
>
I've added a few more cases but I feel adding too many tests into "make
check" will make it too long.
I plan to write an alternate script to test this.

Note: As part of this patch the syntax has been changed, now the CONFLICT
RESOLVER comes before the WITH options as suggested by Vignesh.


CREATE SUBSCRIPTION *subscription_name*
CONNECTION '*conninfo*'
PUBLICATION *publication_name* [, ...]
[ CONFLICT RESOLVER ( *conflict_type* [= *conflict_resolver*] [, ...] ) ]
[ WITH ( *subscription_parameter* [= *value*] [, ... ] ) ]

regards,
Ajin Cherian
Fujitsu Australia