Conflict detection and logging in logical replication

On Friday, June 21, 2024 3:47 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:

- The detail of the conflict detection

We add a new parameter detect_conflict for CREATE and ALTER subscription
commands. This new parameter will decide if subscription will go for
confict detection. By default, conflict detection will be off for a
subscription.

When conflict detection is enabled, additional logging is triggered in the
following conflict scenarios:
insert_exists: Inserting a row that violates a NOT DEFERRABLE unique
constraint.
update_differ: updating a row that was previously modified by another origin.
update_missing: The tuple to be updated is missing.
delete_missing: The tuple to be deleted is missing.

For insert_exists conflict, the log can include origin and commit
timestamp details of the conflicting key with track_commit_timestamp
enabled. And update_differ conflict can only be detected when
track_commit_timestamp is enabled.

Regarding insert_exists conflicts, the current design is to pass
noDupErr=true in ExecInsertIndexTuples() to prevent immediate error
handling on duplicate key violation. After calling
ExecInsertIndexTuples(), if there was any potential conflict in the
unique indexes, we report an ERROR for the insert_exists conflict along
with additional information (origin, committs, key value) for the
conflicting row. Another way for this is to conduct a pre-check for
duplicate key violation before applying the INSERT operation, but this
could introduce overhead for each INSERT even in the absence of conflicts.
We welcome any alternative viewpoints on this matter.

When testing the patch, I noticed a bug that when reporting the conflict
after calling ExecInsertIndexTuples(), we might find the tuple that we
just inserted and report it.(we should only report conflict if there are
other conflict tuples which are not inserted by us) Here is a new patch
which fixed this and fixed a compile warning reported by CFbot.

Best Regards,
Hou zj

On Mon, Jun 24, 2024 at 7:39 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

When testing the patch, I noticed a bug that when reporting the conflict
after calling ExecInsertIndexTuples(), we might find the tuple that we
just inserted and report it.(we should only report conflict if there are
other conflict tuples which are not inserted by us) Here is a new patch
which fixed this and fixed a compile warning reported by CFbot.

Thanks for the patch. Few comments:

1) Few typos:
Commit msg of patch001: iolates--> violates
execIndexing.c: ingored --> ignored

2) Commit msg of stats patch: "The commit adds columns in view
pg_stat_subscription_workers to shows"
--"pg_stat_subscription_workers" --> "pg_stat_subscription_stats"

3) I feel, chapter '31.5. Conflicts' in docs should also mention about
detection or point to the page where it is already mentioned.

thanks
Shveta

On Mon, Jun 24, 2024 at 7:39 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

When testing the patch, I noticed a bug that when reporting the conflict
after calling ExecInsertIndexTuples(), we might find the tuple that we
just inserted and report it.(we should only report conflict if there are
other conflict tuples which are not inserted by us) Here is a new patch
which fixed this and fixed a compile warning reported by CFbot.

Thank you for the patch!
A review comment: The patch does not detect 'update_differ' conflicts
when the Publisher has a non-partitioned table and the Subscriber has
a partitioned version.

Here’s a simple failing test case:
Pub: create table tab (a int primary key, b int not null, c varchar(5));

Sub: create table tab (a int not null, b int not null, c varchar(5))
partition by range (b);
alter table tab add constraint tab_pk primary key (a, b);
create table tab_1 partition of tab for values from (minvalue) to (100);
create table tab_2 partition of tab for values from (100) to (maxvalue);

With the above setup, in case the Subscriber table has a tuple with
its own origin, the incoming remote update from the Publisher fails to
detect the 'update_differ' conflict.

--
Thanks,
Nisha

On Monday, June 24, 2024 8:35 PM Nisha Moond <nisha.moond412@gmail.com> wrote:

On Mon, Jun 24, 2024 at 7:39 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

When testing the patch, I noticed a bug that when reporting the
conflict after calling ExecInsertIndexTuples(), we might find the
tuple that we just inserted and report it.(we should only report
conflict if there are other conflict tuples which are not inserted by
us) Here is a new patch which fixed this and fixed a compile warning

reported by CFbot.

Thank you for the patch!
A review comment: The patch does not detect 'update_differ' conflicts when
the Publisher has a non-partitioned table and the Subscriber has a partitioned
version.

Thanks for reporting the issue !

Here is the new version patch set which fixed this issue. I also fixed
some typos and improved the doc in logical replication conflict based
on the comments from Shveta[1]/messages/by-id/CAJpy0uABSf15E+bMDBRCpbFYo0dh4N=Etpv+SNw6RMy8ohyrcQ@mail.gmail.com.

[1]: /messages/by-id/CAJpy0uABSf15E+bMDBRCpbFYo0dh4N=Etpv+SNw6RMy8ohyrcQ@mail.gmail.com

Best Regards,
Hou zj

On Wednesday, June 26, 2024 10:58 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:

Hi,

As suggested by Sawada-san in another thread[1]/messages/by-id/CAD21AoDzo8ck57nvRVFWOCsjWBCjQMzqTFLY4cCeFeQZ3V_oQg@mail.gmail.com.

I am attaching the V4 patch set which tracks the delete_differ
conflict in logical replication.

delete_differ means that the replicated DELETE is deleting a row
that was modified by a different origin.

[1]: /messages/by-id/CAD21AoDzo8ck57nvRVFWOCsjWBCjQMzqTFLY4cCeFeQZ3V_oQg@mail.gmail.com

Best regards,
Hou zj

On Wed, Jul 3, 2024 at 8:31 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, June 26, 2024 10:58 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:

Hi,

As suggested by Sawada-san in another thread[1].

I am attaching the V4 patch set which tracks the delete_differ
conflict in logical replication.

delete_differ means that the replicated DELETE is deleting a row
that was modified by a different origin.

Thanks for the patch. I am still in process of review but please find
few comments:

1) When I try to *insert* primary/unique key on pub, which already
exists on sub, conflict gets detected. But when I try to *update*
primary/unique key to a value on pub which already exists on sub,
conflict is not detected. I get the error:

2024-07-10 14:21:09.976 IST [647678] ERROR: duplicate key value
violates unique constraint "t1_pkey"
2024-07-10 14:21:09.976 IST [647678] DETAIL: Key (pk)=(4) already exists.

This is because such conflict detection needs detection of constraint
violation using the *new value* rather than *existing* value during
UPDATE. INSERT conflict detection takes care of this case i.e. the
columns of incoming row are considered as new values and it tries to
see if all unique indexes are okay to digest such new values (all
incoming columns) but update's logic is different. It searches based
on oldTuple *only* and thus above detection is missing.

Shall we support such detection? If not, is it worth docuementing? It
basically falls in 'pkey_exists' conflict category but to user it
might seem like any ordinary update leading to 'unique key constraint
violation'.

2)
Another case which might confuse user:

CREATE TABLE t1 (pk integer primary key, val1 integer, val2 integer);

On PUB: insert into t1 values(1,10,10); insert into t1 values(2,20,20);

On SUB: update t1 set pk=3 where pk=2;

Data on PUB: {1,10,10}, {2,20,20}
Data on SUB: {1,10,10}, {3,20,20}

Now on PUB: update t1 set val1=200 where val1=20;

On Sub, I get this:
2024-07-10 14:44:00.160 IST [648287] LOG: conflict update_missing
detected on relation "public.t1"
2024-07-10 14:44:00.160 IST [648287] DETAIL: Did not find the row to
be updated.
2024-07-10 14:44:00.160 IST [648287] CONTEXT: processing remote data
for replication origin "pg_16389" during message type "UPDATE" for
replication target relation "public.t1" in transaction 760, finished
at 0/156D658

To user, it could be quite confusing, as val1=20 exists on sub but
still he gets update_missing conflict and the 'DETAIL' is not
sufficient to give the clarity. I think on HEAD as well (have not
tested), we will get same behavior i.e. update will be ignored as we
make search based on RI (pk in this case). So we are not worsening the
situation, but now since we are detecting conflict, is it possible to
give better details in 'DETAIL' section indicating what is actually
missing?

thanks
Shveta

On Wednesday, July 10, 2024 5:39 PM shveta malik <shveta.malik@gmail.com> wrote:

On Wed, Jul 3, 2024 at 8:31 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
wrote:

On Wednesday, June 26, 2024 10:58 AM Zhijie Hou (Fujitsu)

<houzj.fnst@fujitsu.com> wrote:

Hi,

As suggested by Sawada-san in another thread[1].

I am attaching the V4 patch set which tracks the delete_differ
conflict in logical replication.

delete_differ means that the replicated DELETE is deleting a row that
was modified by a different origin.

Thanks for the patch. I am still in process of review but please find few
comments:

Thanks for the comments!

1) When I try to *insert* primary/unique key on pub, which already exists on
sub, conflict gets detected. But when I try to *update* primary/unique key to a
value on pub which already exists on sub, conflict is not detected. I get the
error:

2024-07-10 14:21:09.976 IST [647678] ERROR: duplicate key value violates
unique constraint "t1_pkey"
2024-07-10 14:21:09.976 IST [647678] DETAIL: Key (pk)=(4) already exists.

Yes, I think the detection of this conflict is not added with the
intention to control the size of the patch in the first version.

This is because such conflict detection needs detection of constraint violation
using the *new value* rather than *existing* value during UPDATE. INSERT
conflict detection takes care of this case i.e. the columns of incoming row are
considered as new values and it tries to see if all unique indexes are okay to
digest such new values (all incoming columns) but update's logic is different.
It searches based on oldTuple *only* and thus above detection is missing.

I think the logic is the same if we want to detect the unique violation
for UDPATE, we need to check if the new value of the UPDATE violates any
unique constraints same as the detection of insert_exists (e.g. check
the conflict around ExecInsertIndexTuples())

Shall we support such detection? If not, is it worth docuementing?

I am personally OK to support this detection. And
I think it's already documented that we only detect unique violation for
insert which mean update conflict is not detected.

2)
Another case which might confuse user:

CREATE TABLE t1 (pk integer primary key, val1 integer, val2 integer);

On PUB: insert into t1 values(1,10,10); insert into t1 values(2,20,20);

On SUB: update t1 set pk=3 where pk=2;

Data on PUB: {1,10,10}, {2,20,20}
Data on SUB: {1,10,10}, {3,20,20}

Now on PUB: update t1 set val1=200 where val1=20;

On Sub, I get this:
2024-07-10 14:44:00.160 IST [648287] LOG: conflict update_missing detected
on relation "public.t1"
2024-07-10 14:44:00.160 IST [648287] DETAIL: Did not find the row to be
updated.
2024-07-10 14:44:00.160 IST [648287] CONTEXT: processing remote data for
replication origin "pg_16389" during message type "UPDATE" for replication
target relation "public.t1" in transaction 760, finished at 0/156D658

To user, it could be quite confusing, as val1=20 exists on sub but still he gets
update_missing conflict and the 'DETAIL' is not sufficient to give the clarity. I
think on HEAD as well (have not tested), we will get same behavior i.e. update
will be ignored as we make search based on RI (pk in this case). So we are not
worsening the situation, but now since we are detecting conflict, is it possible
to give better details in 'DETAIL' section indicating what is actually missing?

I think It's doable to report the row value that cannot be found in the local
relation, but the concern is the potential risk of exposing some
sensitive data in the log. This may be OK, as we are already reporting the
key value for constraints violation, so if others also agree, we can add
the row value in the DETAIL as well.

Best Regards,
Hou zj

On Thu, Jul 11, 2024 at 7:47 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, July 10, 2024 5:39 PM shveta malik <shveta.malik@gmail.com> wrote:

On Wed, Jul 3, 2024 at 8:31 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
wrote:

On Wednesday, June 26, 2024 10:58 AM Zhijie Hou (Fujitsu)

<houzj.fnst@fujitsu.com> wrote:

Hi,

As suggested by Sawada-san in another thread[1].

I am attaching the V4 patch set which tracks the delete_differ
conflict in logical replication.

delete_differ means that the replicated DELETE is deleting a row that
was modified by a different origin.

Thanks for the patch. I am still in process of review but please find few
comments:

Thanks for the comments!

1) When I try to *insert* primary/unique key on pub, which already exists on
sub, conflict gets detected. But when I try to *update* primary/unique key to a
value on pub which already exists on sub, conflict is not detected. I get the
error:

2024-07-10 14:21:09.976 IST [647678] ERROR: duplicate key value violates
unique constraint "t1_pkey"
2024-07-10 14:21:09.976 IST [647678] DETAIL: Key (pk)=(4) already exists.

Yes, I think the detection of this conflict is not added with the
intention to control the size of the patch in the first version.

This is because such conflict detection needs detection of constraint violation
using the *new value* rather than *existing* value during UPDATE. INSERT
conflict detection takes care of this case i.e. the columns of incoming row are
considered as new values and it tries to see if all unique indexes are okay to
digest such new values (all incoming columns) but update's logic is different.
It searches based on oldTuple *only* and thus above detection is missing.

I think the logic is the same if we want to detect the unique violation
for UDPATE, we need to check if the new value of the UPDATE violates any
unique constraints same as the detection of insert_exists (e.g. check
the conflict around ExecInsertIndexTuples())

Shall we support such detection? If not, is it worth docuementing?

I am personally OK to support this detection.

+1. I think it should not be a complex or too big change.

And
I think it's already documented that we only detect unique violation for
insert which mean update conflict is not detected.

2)
Another case which might confuse user:

CREATE TABLE t1 (pk integer primary key, val1 integer, val2 integer);

On PUB: insert into t1 values(1,10,10); insert into t1 values(2,20,20);

On SUB: update t1 set pk=3 where pk=2;

Data on PUB: {1,10,10}, {2,20,20}
Data on SUB: {1,10,10}, {3,20,20}

Now on PUB: update t1 set val1=200 where val1=20;

On Sub, I get this:
2024-07-10 14:44:00.160 IST [648287] LOG: conflict update_missing detected
on relation "public.t1"
2024-07-10 14:44:00.160 IST [648287] DETAIL: Did not find the row to be
updated.
2024-07-10 14:44:00.160 IST [648287] CONTEXT: processing remote data for
replication origin "pg_16389" during message type "UPDATE" for replication
target relation "public.t1" in transaction 760, finished at 0/156D658

To user, it could be quite confusing, as val1=20 exists on sub but still he gets
update_missing conflict and the 'DETAIL' is not sufficient to give the clarity. I
think on HEAD as well (have not tested), we will get same behavior i.e. update
will be ignored as we make search based on RI (pk in this case). So we are not
worsening the situation, but now since we are detecting conflict, is it possible
to give better details in 'DETAIL' section indicating what is actually missing?

I think It's doable to report the row value that cannot be found in the local
relation, but the concern is the potential risk of exposing some
sensitive data in the log. This may be OK, as we are already reporting the
key value for constraints violation, so if others also agree, we can add
the row value in the DETAIL as well.

Okay, let's see what others say. JFYI, the same situation holds valid
for delete_missing case.

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)

2) Now since we have a section explaining conflicts detected and
logged with detect_conflict=true, shall we mention default behaviour
with each?

insert_exists: error will be raised until resolved manually.
update_differ: update will be applied
update_missing: update will be skipped
delete_missing: delete will be skipped
delete_differ: delete will be applied.

thanks
Shveta

On Wed, Jul 10, 2024 at 3:09 PM shveta malik <shveta.malik@gmail.com> wrote:

On Wed, Jul 3, 2024 at 8:31 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, June 26, 2024 10:58 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:

Hi,

As suggested by Sawada-san in another thread[1].

I am attaching the V4 patch set which tracks the delete_differ
conflict in logical replication.

delete_differ means that the replicated DELETE is deleting a row
that was modified by a different origin.

Thanks for the patch. please find few comments for patch002:

1)
Commit msg says: The conflicts will be tracked only when
track_conflict option of the subscription is enabled.

track_conflict --> detect_conflict

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.

2d) To be consistent, we can change 'is not found' to 'was not found'
in update_missing_count , delete_missing_count cases as well.

3)
create_subscription.sgml has:
When conflict detection is enabled, additional logging is triggered
and the conflict statistics are collected in the following scenarios:

--Can we rephrase a little and link pg_stat_subscription_stats
structure here for reference.

4)
IIUC, conflict_count array (in pgstat.h) maps directly to ConflictType
enum. So if the order of entries ever changes in this enum, without
changing it in pg_stat_subscription_stats and pg_proc, we may get
wrong values under each column when querying
pg_stat_subscription_stats. If so, then perhaps it is good to add a
comment atop ConflictType that if someone changes this order, order in
other files too needs to be changed.

5)
conflict.h:CONFLICT_NUM_TYPES

--Shall the macro be CONFLICT_TYPES_NUM instead?

6)
pgstatsfuncs.c

-----
/* conflict count */
for (int i = 0; i < CONFLICT_NUM_TYPES; i++)
values[3 + i] = Int64GetDatum(subentry->conflict_count[i]);

/* stats_reset */
if (subentry->stat_reset_timestamp == 0)
nulls[8] = true;
else
values[8] = TimestampTzGetDatum(subentry->stat_reset_timestamp);
-----

After setting values for [3+i], we abruptly had [8]. I think it will
be better to use i++ to increment values' index. And at the end, we
can check if it reached 'PG_STAT_GET_SUBSCRIPTION_STATS_COLS'.

thanks
Shveta

On Thursday, July 11, 2024 1:03 PM shveta malik <shveta.malik@gmail.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.

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.

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.

Best Regards,
Hou zj

On Thu, Jul 18, 2024 at 7:52 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Thursday, July 11, 2024 1:03 PM shveta malik <shveta.malik@gmail.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

On Thu, Jul 18, 2024 at 7:52 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

Attach the V5 patch set which changed the following.

Thanks for the patch. Tested that previous reported issues are fixed.
Please have a look at below scenario, I was expecting it to raise
'update_differ' but it raised both 'update_differ' and 'delete_differ'
together:

-------------------------
Pub:
create table tab (a int not null, b int primary key);
create publication pub1 for table tab;

Sub (partitioned table):
create table tab (a int not null, b int primary key) partition by
range (b);
create table tab_1 partition of tab for values from (minvalue) to
(100);
create table tab_2 partition of tab for values from (100) to
(maxvalue);
create subscription sub1 connection '.......' publication pub1 WITH
(detect_conflict=true);

Pub - insert into tab values (1,1);
Sub - update tab set b=1000 where a=1;
Pub - update tab set b=1000 where a=1; -->update_missing detected
correctly as b=1 will not be found on sub.
Pub - update tab set b=1 where b=1000; -->update_differ expected, but
it gives both update_differ and delete_differ.
-------------------------

Few trivial comments:

1)
Commit msg:
For insert_exists conflict, the log can include origin and commit
timestamp details of the conflicting key with track_commit_timestamp
enabled.

--Please add update_exists as well.

2)
execReplication.c:
Return false if there is no conflict and *conflictslot is set to NULL.

--This gives a feeling that this function will return false if both
the conditions are true. But instead first one is the condition, while
the second is action. Better to rephrase to:

Returns false if there is no conflict. Sets *conflictslot to NULL in
such a case.
Or
Sets *conflictslot to NULL and returns false in case of no conflict.

3)
FindConflictTuple() shares some code parts with
RelationFindReplTupleByIndex() and RelationFindReplTupleSeq() for
checking status in 'res'. Is it worth making a function to be used in
all three.

thanks
Shveta

On Fri, Jul 19, 2024 at 2:06 PM shveta malik <shveta.malik@gmail.com> wrote:

On Thu, Jul 18, 2024 at 7:52 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

Attach the V5 patch set which changed the following.

Please find last batch of comments on v5:

patch001:
1)
create subscription sub1 ... (detect_conflict=true);
I think it will be good to give WARNING here indicating that
detect_conflict is enabled but track_commit_timestamp is disabled and
thus few conflicts detection may not work. (Rephrase as apt)

2)
013_partition.pl: Since we have added update_differ testcase here, we
shall add delete_differ as well. And in some file where appropriate,
we shall add update_exists test as well.

3)
013_partition.pl (#799,802):
For update_missing and delete_missing, we have log verification format
as 'qr/conflict delete_missing/update_missing detected on relation '.
But for update_differ, we do not capture "conflict update_differ
detected on relation ...". We capture only the DETAIL.
I think we should be consistent and capture conflict name here as well.

patch002:

4)
pg_stat_get_subscription_stats():

---------
/* conflict count */
for (int nconflict = 0; nconflict < CONFLICT_NUM_TYPES; nconflict++)
values[i + nconflict] = Int64GetDatum(subentry->conflict_count[nconflict]);

i += CONFLICT_NUM_TYPES;
---------

Can't we do values[i++] here as well (instead of values[i +
nconflict])? Then we don't need to do 'i += CONFLICT_NUM_TYPES'.

5)
026_stats.pl:
Wherever we are checking this: 'Apply and Sync errors are > 0 and
reset timestamp is NULL', we need to check update_exssts_count as well
along with other counts.

thanks
Shveta

On Thu, Jul 18, 2024 at 7:52 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

Attach the V5 patch set which changed the following:

Tested v5-0001 patch, and it fails to detect the update_exists
conflict for a setup where Pub has a non-partitioned table and Sub has
the same table partitioned.
Below is a testcase showcasing the issue:

Setup:
Pub:
create table tab (a int not null, b int not null);
alter table tab add constraint tab_pk primary key (a,b);

Sub:
create table tab (a int not null, b int not null) partition by range (b);
alter table tab add constraint tab_pk primary key (a,b);
CREATE TABLE tab_1 PARTITION OF tab FOR VALUES FROM (MINVALUE) TO (100);
CREATE TABLE tab_2 PARTITION OF tab FOR VALUES FROM (101) TO (MAXVALUE);

Test:
Pub: insert into tab values (1,1);
Sub: insert into tab values (2,1);
Pub: update tab set a=2 where a=1; --> After this update on Pub,
'update_exists' is expected on Sub, but it fails to detect the
conflict and logs the key violation error -

ERROR: duplicate key value violates unique constraint "tab_1_pkey"
DETAIL: Key (a, b)=(2, 1) already exists.

Thanks,
Nisha

On Monday, July 22, 2024 5:03 PM shveta malik <shveta.malik@gmail.com> wrote:

On Fri, Jul 19, 2024 at 2:06 PM shveta malik <shveta.malik@gmail.com> wrote:

On Thu, Jul 18, 2024 at 7:52 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

Attach the V5 patch set which changed the following.

Please find last batch of comments on v5:

Thanks Shveta and Nisha for giving comments!

2)
013_partition.pl: Since we have added update_differ testcase here, we shall
add delete_differ as well.

I didn't add tests for delete_differ in partition test, because I think the main
codes and functionality of delete_differ have been tested in 030_origin.pl.
The test for update_differ is needed because the patch adds new codes in
partition code path to report this conflict.

Here is the V6 patch set which addressed Shveta and Nisha's comments
in [1]/messages/by-id/CAJpy0uDWdw2W-S8boFU0KOcZjw0+sFFgLrHYrr1TROtrcTPZMg@mail.gmail.com[2]/messages/by-id/CAJpy0uDGJXdVCGoaRHP-5G0pL0zhuZaRJSqxOxs=CNsSwc+SJQ@mail.gmail.com[3]/messages/by-id/CAJpy0uC+1puapWdOnAMSS=QUp_1jj3GfAeivE0JRWbpqrUy=ug@mail.gmail.com[4]/messages/by-id/CABdArM6+N1Xy_+tK+u-H=sCB+92rAUh8qH6GDsB+1naKzgGKzQ@mail.gmail.com.

[1]: /messages/by-id/CAJpy0uDWdw2W-S8boFU0KOcZjw0+sFFgLrHYrr1TROtrcTPZMg@mail.gmail.com
[2]: /messages/by-id/CAJpy0uDGJXdVCGoaRHP-5G0pL0zhuZaRJSqxOxs=CNsSwc+SJQ@mail.gmail.com
[3]: /messages/by-id/CAJpy0uC+1puapWdOnAMSS=QUp_1jj3GfAeivE0JRWbpqrUy=ug@mail.gmail.com
[4]: /messages/by-id/CABdArM6+N1Xy_+tK+u-H=sCB+92rAUh8qH6GDsB+1naKzgGKzQ@mail.gmail.com

Best Regards,
Hou zj

On Thu, Jul 25, 2024 at 12:04 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

Here is the V6 patch set which addressed Shveta and Nisha's comments
in [1][2][3][4].

Do we need an option detect_conflict for logging conflicts? The
possible reason to include such an option is to avoid any overhead
during apply due to conflict detection. IIUC, to detect some of the
conflicts like update_differ and delete_differ, we would need to fetch
commit_ts information which could be costly but we do that only when
GUC track_commit_timestamp is enabled which would anyway have overhead
on its own. Can we do performance testing to see how much additional
overhead we have due to fetching commit_ts information during conflict
detection?

The other time we need to enquire commit_ts is to log the conflict
detection information which is an ERROR path, so performance shouldn't
matter in this case.

In general, it would be good to enable conflict detection/logging by
default but if it has overhead then we can consider adding this new
option. Anyway, adding an option could be a separate patch (at least
for review), let the first patch be the core code of conflict
detection and logging.

minor cosmetic comments:
1.
+static void
+check_conflict_detection(void)
+{
+ if (!track_commit_timestamp)
+ ereport(WARNING,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("conflict detection could be incomplete due to disabled
track_commit_timestamp"),
+ errdetail("Conflicts update_differ and delete_differ cannot be
detected, and the origin and commit timestamp for the local row will
not be logged."));
+}

The errdetail string is too long. It would be better to split it into
multiple rows.

2.
-
+static void check_conflict_detection(void);

Spurious line removal.

--
With Regards,
Amit Kapila.

On Thu, Jul 11, 2024 at 7:47 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, July 10, 2024 5:39 PM shveta malik <shveta.malik@gmail.com> wrote:

2)
Another case which might confuse user:

CREATE TABLE t1 (pk integer primary key, val1 integer, val2 integer);

On PUB: insert into t1 values(1,10,10); insert into t1 values(2,20,20);

On SUB: update t1 set pk=3 where pk=2;

Data on PUB: {1,10,10}, {2,20,20}
Data on SUB: {1,10,10}, {3,20,20}

Now on PUB: update t1 set val1=200 where val1=20;

On Sub, I get this:
2024-07-10 14:44:00.160 IST [648287] LOG: conflict update_missing detected
on relation "public.t1"
2024-07-10 14:44:00.160 IST [648287] DETAIL: Did not find the row to be
updated.
2024-07-10 14:44:00.160 IST [648287] CONTEXT: processing remote data for
replication origin "pg_16389" during message type "UPDATE" for replication
target relation "public.t1" in transaction 760, finished at 0/156D658

To user, it could be quite confusing, as val1=20 exists on sub but still he gets
update_missing conflict and the 'DETAIL' is not sufficient to give the clarity. I
think on HEAD as well (have not tested), we will get same behavior i.e. update
will be ignored as we make search based on RI (pk in this case). So we are not
worsening the situation, but now since we are detecting conflict, is it possible
to give better details in 'DETAIL' section indicating what is actually missing?

I think It's doable to report the row value that cannot be found in the local
relation, but the concern is the potential risk of exposing some
sensitive data in the log. This may be OK, as we are already reporting the
key value for constraints violation, so if others also agree, we can add
the row value in the DETAIL as well.

This is still awaiting some feedback. I feel it will be good to add
some pk value at-least in DETAIL section, like we add for other
conflict types.

thanks
Shveta

On Thu, Jul 25, 2024 at 12:04 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Monday, July 22, 2024 5:03 PM shveta malik <shveta.malik@gmail.com> wrote:

On Fri, Jul 19, 2024 at 2:06 PM shveta malik <shveta.malik@gmail.com> wrote:

On Thu, Jul 18, 2024 at 7:52 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

Attach the V5 patch set which changed the following.

Please find last batch of comments on v5:

Thanks Shveta and Nisha for giving comments!

2)
013_partition.pl: Since we have added update_differ testcase here, we shall
add delete_differ as well.

I didn't add tests for delete_differ in partition test, because I think the main
codes and functionality of delete_differ have been tested in 030_origin.pl.
The test for update_differ is needed because the patch adds new codes in
partition code path to report this conflict.

Here is the V6 patch set which addressed Shveta and Nisha's comments
in [1][2][3][4].

Thanks for addressing the comments.

[1] /messages/by-id/CAJpy0uDWdw2W-S8boFU0KOcZjw0+sFFgLrHYrr1TROtrcTPZMg@mail.gmail.com
[2] /messages/by-id/CAJpy0uDGJXdVCGoaRHP-5G0pL0zhuZaRJSqxOxs=CNsSwc+SJQ@mail.gmail.com
[3] /messages/by-id/CAJpy0uC+1puapWdOnAMSS=QUp_1jj3GfAeivE0JRWbpqrUy=ug@mail.gmail.com
[4] /messages/by-id/CABdArM6+N1Xy_+tK+u-H=sCB+92rAUh8qH6GDsB+1naKzgGKzQ@mail.gmail.com

I was re-testing all the issues reported so far. I think the issue
reported in [4] above is not fixed yet.

Please find a few more comments:

patch001:

1)
030_origin.pl:
I feel tests added in this file may fail. Since there are 3 nodes here
and if the actual order of replication is not as per the expected
order by your test, it will fail.

Example:
---------
$node_B->safe_psql('postgres', "DELETE FROM tab;");
$node_A->safe_psql('postgres', "INSERT INTO tab VALUES (33);");

# The delete should remove the row on node B that was inserted by node A.
$node_C->safe_psql('postgres', "DELETE FROM tab WHERE a = 33;");

$node_B->wait_for_log(
qr/conflict delete_differ detected..);
---------

The third line assumes Node A's change is replicated to Node B already
before Node C's change reaches NodeB, but it may not be true. Should
we do wait_for_catchup and have a verification step that Node A data
is replicated to Node B before we execute Node C query?
Same for the rest of the tests.

2) 013_partition.pl:
---------
$logfile = slurp_file($node_subscriber1->logfile(), $log_location);
ok( $logfile =~
qr/Updating a row that was modified by a different origin [0-9]+ in
transaction [0-9]+ at .*/,
'updating a tuple that was modified by a different origin');
---------

To be consistent, here as well, we can have 'conflict update_differ
detected on relation ....'

patch002:
3) monitoring.sgml:
'Number of times that the updated value of a row violates a NOT
DEFERRABLE unique constraint while applying changes.'

To be consistent, we can change: 'violates' --> 'violated'

thanks
Shveta

On Fri, Jul 26, 2024 at 9:39 AM shveta malik <shveta.malik@gmail.com> wrote:

On Thu, Jul 11, 2024 at 7:47 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Wednesday, July 10, 2024 5:39 PM shveta malik <shveta.malik@gmail.com> wrote:

2)
Another case which might confuse user:

CREATE TABLE t1 (pk integer primary key, val1 integer, val2 integer);

On PUB: insert into t1 values(1,10,10); insert into t1 values(2,20,20);

On SUB: update t1 set pk=3 where pk=2;

Data on PUB: {1,10,10}, {2,20,20}
Data on SUB: {1,10,10}, {3,20,20}

Now on PUB: update t1 set val1=200 where val1=20;

On Sub, I get this:
2024-07-10 14:44:00.160 IST [648287] LOG: conflict update_missing detected
on relation "public.t1"
2024-07-10 14:44:00.160 IST [648287] DETAIL: Did not find the row to be
updated.
2024-07-10 14:44:00.160 IST [648287] CONTEXT: processing remote data for
replication origin "pg_16389" during message type "UPDATE" for replication
target relation "public.t1" in transaction 760, finished at 0/156D658

To user, it could be quite confusing, as val1=20 exists on sub but still he gets
update_missing conflict and the 'DETAIL' is not sufficient to give the clarity. I
think on HEAD as well (have not tested), we will get same behavior i.e. update
will be ignored as we make search based on RI (pk in this case). So we are not
worsening the situation, but now since we are detecting conflict, is it possible
to give better details in 'DETAIL' section indicating what is actually missing?

I think It's doable to report the row value that cannot be found in the local
relation, but the concern is the potential risk of exposing some
sensitive data in the log. This may be OK, as we are already reporting the
key value for constraints violation, so if others also agree, we can add
the row value in the DETAIL as well.

This is still awaiting some feedback. I feel it will be good to add
some pk value at-least in DETAIL section, like we add for other
conflict types.

I agree that displaying pk where applicable should be okay as we
display it at other places but the same won't be possible when we do
sequence scan to fetch the required tuple. So, the message will be
different in that case, right?

--
With Regards,
Amit Kapila.