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

On Thu, 12 Sept 2024 at 14:03, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Tue, Sep 3, 2024 at 7:42 PM vignesh C <vignesh21@gmail.com> wrote:
>
>     On Fri, 30 Aug 2024 at 11:01, Nisha Moond <nisha.moond412@gmail.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.
>

Few comments:
1) This should be in  (fout->remoteVersion >= 180000) check to support
dumping backward compatible server objects, else dump with older
version will fail:
+               /* Populate conflict type fields using the new query */
+               confQuery = createPQExpBuffer();
+               appendPQExpBuffer(confQuery,
+                                                 "SELECT confrtype,
confrres FROM pg_catalog.pg_subscription_conflict "
+                                                 "WHERE confsubid =
%u;", subinfo[i].dobj.catId.oid);
+               confRes = ExecuteSqlQuery(fout, confQuery->data,
PGRES_TUPLES_OK);
+
+               ntuples = PQntuples(confRes);
+               for (j = 0; j < ntuples; j++)

2) Can we check and throw an error before the warning is logged in
this case as it seems strange to throw a warning first and then an
error for the same track_commit_timestamp configuration:
postgres=# create subscription sub1 connection ... publication pub1
conflict resolver (insert_exists = 'last_update_wins');
WARNING:  conflict detection and resolution could be incomplete due to
disabled track_commit_timestamp
DETAIL:  Conflicts update_origin_differs and delete_origin_differs
cannot be detected, and the origin and commit timestamp for the local
row will not be logged.
ERROR:  resolver last_update_wins requires "track_commit_timestamp" to
be enabled
HINT:  Make sure the configuration parameter "track_commit_timestamp" is set.

Regards,
Vignesh