Re: Conflict detection and logging in logical replication

On Thu, Jul 18, 2024 at 7:52 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Thursday, July 11, 2024 1:03 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> Hi,
>
> Thanks for the comments!
>
> >
> > I have one concern about how we deal with conflicts. As for insert_exists, we
> > keep on erroring out while raising conflict, until it is manually resolved:
> > ERROR: conflict insert_exists detected
> >
> > But for other cases, we just log conflict and either skip or apply the operation. I
> > LOG: conflict update_differ detected
> > DETAIL: Updating a row that was modified by a different origin
> >
> > I know that it is no different than HEAD. But now since we are logging conflicts
> > explicitly, we should call out default behavior on each conflict. I see some
> > incomplete and scattered info in '31.5.
> > Conflicts' section saying that:
> > "When replicating UPDATE or DELETE operations, missing data will not
> > produce a conflict and such operations will simply be skipped."
> > (lets say it as pt a)
> >
> > Also some more info in a later section saying (pt b):
> > :A conflict will produce an error and will stop the replication; it must be resolved
> > manually by the user."
> >
> > My suggestions:
> > 1) in point a above, shall we have:
> > missing data or differing data (i.e. somehow reword to accommodate
> > update_differ and delete_differ cases)
>
> I am not sure if rewording existing words is better. I feel adding a link to
> let user refer to the detect_conflict section for the all the
> conflicts is sufficient, so did like that.

Agree, it looks better with detect_conflict link.

> >
> > 2)
> > monitoring.sgml: Below are my suggestions, please change if you feel apt.
> >
> > 2a) insert_exists_count : Number of times inserting a row that violates a NOT
> > DEFERRABLE unique constraint while applying changes. Suggestion: Number of
> > times a row insertion violated a NOT DEFERRABLE unique constraint while
> > applying changes.
> >
> > 2b) update_differ_count : Number of times updating a row that was previously
> > modified by another source while applying changes. Suggestion: Number of times
> > update was performed on a row that was previously modified by another source
> > while applying changes.
> >
> > 2c) delete_differ_count: Number of times deleting a row that was previously
> > modified by another source while applying changes. Suggestion: Number of times
> > delete was performed on a row that was previously modified by another source
> > while applying changes.
>
> I am a bit unsure which one is better, so I didn't change in this version.

I still feel the wording is bit unclear/incomplete Also to be
consistent with previous fields (see sync_error_count:Number of times
an error occurred during the initial table synchronization), we should
at-least have it in past tense. Another way of writing could be:

'Number of times inserting a row violated a NOT DEFERRABLE unique
constraint while applying changes.' and likewise for each conflict
field.

> >
> > 5)
> > conflict.h:CONFLICT_NUM_TYPES
> >
> > --Shall the macro be CONFLICT_TYPES_NUM instead?
>
> I think the current style followed existing ones(e.g. IOOP_NUM_TYPES,
> BACKEND_NUM_TYPES, IOOBJECT_NUM_TYPES ...), so I didn't change this.
>
> Attach the V5 patch set which changed the following:
> 1. addressed shveta's comments which are not mentioned above.
> 2. support update_exists conflict which indicates
> that the updated value of a row violates the unique constraint.

Thanks for making the changes.

thanks
Shveta