Re: Conflict Detection and Resolution

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].

New in patches:
1) Added partition table tests in 034_conflict_resolver.pl in 002 and
003 patches.
2) 003 has a bug fix for update_exists conflict resolution on
partitioned tables.

[1]: https://www.postgresql.org/message-id/CALDaNm3es1JqU8Qcv5Yw%3D7Ts2dOvaV8a_boxPSdofB%2BDTx1oFg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CALDaNm18HuAcNsEC47J6qLRC7rMD2Q9_wT_hFtcc4UWqsfkgjA%40mail.gmail.com

Thanks,
Nisha