Re: Conflict Detection and Resolution

On Fri, Sep 20, 2024 at 8:40 AM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Wed, Sep 18, 2024 at 10:46 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Thu, 12 Sept 2024 at 14:03, Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> > >
> > > 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.
> > >
> >
> > 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.
> >
>
> Thanks for the review.
> Here is the v14 patch-set fixing review comments in [1] and [2].

Just noticed that there are failures for the new
034_conflict_resolver.pl test on CFbot. From the initial review it
seems to be a test issue and not a bug.
We will fix these along with the next version of patch-sets.

Thanks,
Nisha