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

On Fri, Sep 20, 2024 at 8:40 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Wed, Sep 18, 2024 at 10:46 AM vignesh C <vignesh21@gmail.com> wrote:
> >
> > 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.
> >
>
> 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