Re: Conflict detection for multiple_unique_conflicts in logical replication - Mailing list pgsql-hackers

Hi Nisha.

Some review comments for patch v1-0001

======
GENERAL

1.
This may be a basic/naive question, but it is unclear to me why we
care about the stats of confl_multiple_unique_conflicts?

I can understand it would be useful to get multiple conflicts logged
at the same time so the user doesn't have to stumble across them one
by one when fixing them, but as far as the stats go, why do we care
about this stat? Also, since you don't distinguish between multiple
insert conflicts versus multiple update conflicts the stat usefulness
seemed even more dubious.

(because of this question, I skipped reviewing some parts of this
patch related to stats)

======
Commit message

2.
/reports a multiple_unique_conflicts/reports multiple_unique_conflicts/

~~~

3.
Also, the patch adds a new column in view pg_stat_subscription_stats
to support stats collection for this conflict type.

~

Maybe say the name of the added column.

======
doc/src/sgml/logical-replication.sgml

4.
+      <para>
+       Inserting a row or updating values of a row violates more than
one <literal>NOT DEFERRABLE</literal>
+       unique constraints. Note that to log the origin and commit
timestamp details of the conflicting key,
+       <link linkend="guc-track-commit-timestamp"><varname>track_commit_timestamp</varname></link>
+       should be enabled on the subscriber. In this case, an error will be
+       raised until the conflict is resolved manually.
+      </para>

/unique constraints./unique constraint./

======
doc/src/sgml/monitoring.sgml

5.
See the GENERAL question. Why is this useful?

======
src/backend/executor/execReplication.c

6.
+
+ /*
+ * Report an UPDATE_EXISTS conflict when only one unique constraint is
+ * violated.
+ */

The comment says UPDATE_EXISTS, but I thought that could be something
else like INSERT_EXISTS too.

~~~

7.
+ /*
+ * Report a MULTIPLE_UNIQUE_CONFLICTS when two or more unique constraints
+ * are violated.
+ */

/Report a MULTIPLE_UNIQUE_CONFLICTS/Report MULTIPLE_UNIQUE_CONFLICTS/

======
src/backend/replication/logical/conflict.c

ReportMultipleUniqueConflict:

8.
+ * 'conflictslots_list' is a list of slots containing the local tuples
+ * that conflict with the remote tuple.

There is no parameter called conflictslots_list

======
.../t/035_multiple_unique_conflicts.pl

9.
+# Copyright (c) 2024, PostgreSQL Global Development Group
+

/2024/2025/

~~~

10.
+my $log_location = -s $node_subscriber->logfile;
+##################################################
+# Test multiple_unique_conflicts due to INSERT
+##################################################

Add blank line above that comment.

~~~

11.
+ok( $logfile =~
+   qr/conflict detected on relation \"public.conf_tab\":
conflict=multiple_unique_conflicts*\n.*DETAIL:.* The remote tuple
violates multiple unique constraints on the local table./,
+ 'multiple_unique_conflicts detected during insertion');
+

Won't it be more useful to also log the column name causing the
conflict? Otherwise, if there are 100s of unique columns and just 2 of
them are conflicts how will the user know what to look for when fixing
it?

~~~

12.
+ok( $logfile =~
+   qr/conflict detected on relation \"public.conf_tab\":
conflict=multiple_unique_conflicts*\n.*DETAIL:.* The remote tuple
violates multiple unique constraints on the local table./,
+ 'multiple_unique_conflicts detected during update');

(same as #12)

Won't it be more useful to also log the column name causing the
conflict? Otherwise, if there are 100s of unique columns and just 2 of
them are conflicts how will the user know what to look for when fixing
it?

======
Kind Regards,
Peter Smith.
Fujitsu Australia