Hi Hou-san. Here are some review comments for your patch v1-0001.
======
doc/src/sgml/logical-replication.sgml
nit - added a comma.
======
doc/src/sgml/monitoring.sgml
nit - use <literal> for 'apply_error_count'.
nit - added a period when there are multiple sentences.
nit - adjusted field descriptions using Chat-GPT clarification suggestions
======
src/include/pgstat.h
nit - change the param to 'type' -- ie. same as the implementation calls it
======
src/include/replication/conflict.h
nit - defined 'NUM_CONFLICT_TYPES' inside the enum (I think this way
is often used in other PG source enums)
======
src/test/subscription/t/026_stats.pl
1.
+ # Delete data from the test table on the publisher. This delete operation
+ # should be skipped on the subscriber since the table is already empty.
+ $node_publisher->safe_psql($db, qq(DELETE FROM $table_name;));
+
+ # Wait for the subscriber to report tuple missing conflict.
+ $node_subscriber->poll_query_until(
+ $db,
+ qq[
+ SELECT update_missing_count > 0 AND delete_missing_count > 0
+ FROM pg_stat_subscription_stats
+ WHERE subname = '$sub_name'
+ ])
+ or die
+ qq(Timed out while waiting for tuple missing conflict for
subscription '$sub_name');
Can you write a comment to explain why the replicated DELETE is
expected to increment both the 'update_missing_count' and the
'delete_missing_count'?
~
nit - update several "Apply and Sync errors..." comments that did not
mention conflicts
nit - tweak comments wording for update_differ and delete_differ
nit - /both > 0/> 0/
nit - /both 0/0/
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/logical-replication.sgml
b/doc/src/sgml/logical-replication.sgml
index f3e3641..f682369 100644
--- a/doc/src/sgml/logical-replication.sgml
+++ b/doc/src/sgml/logical-replication.sgml
@@ -1585,7 +1585,7 @@ test_sub=# SELECT * FROM t1 ORDER BY id;
</para>
<para>
- Additional logging is triggered and the conflict statistics are collected
(displayed in the
+ Additional logging is triggered, and the conflict statistics are collected
(displayed in the
<link
linkend="monitoring-pg-stat-subscription-stats"><structname>pg_stat_subscription_stats</structname></link>
view)
in the following <firstterm>conflict</firstterm> cases:
<variablelist>
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index ea36d46..ac3c773 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -2159,7 +2159,7 @@ description | Waiting for a newly initialized WAL file to
reach durable storage
<para>
Number of times an error occurred while applying changes. Note that any
conflict resulting in an apply error will be counted in both
- apply_error_count and the corresponding conflict count.
+ <literal>apply_error_count</literal> and the corresponding conflict
count.
</para></entry>
</row>
@@ -2179,8 +2179,8 @@ description | Waiting for a newly initialized WAL file to
reach durable storage
</para>
<para>
Number of times a row insertion violated a
- <literal>NOT DEFERRABLE</literal> unique constraint while applying
- changes
+ <literal>NOT DEFERRABLE</literal> unique constraint during the
+ application of changes
</para></entry>
</row>
@@ -2189,11 +2189,11 @@ description | Waiting for a newly initialized WAL file
to reach durable storage
<structfield>update_differ_count</structfield> <type>bigint</type>
</para>
<para>
- Number of times an update was performed on a row that was previously
- modified by another source while applying changes. This conflict is
+ Number of times an update was applied to a row that had been previously
+ modified by another source during the application of changes. This
conflict is
counted only when the
<link
linkend="guc-track-commit-timestamp"><varname>track_commit_timestamp</varname></link>
- option is enabled on the subscriber
+ option is enabled on the subscriber.
</para></entry>
</row>
@@ -2202,9 +2202,9 @@ description | Waiting for a newly initialized WAL file to
reach durable storage
<structfield>update_exists_count</structfield> <type>bigint</type>
</para>
<para>
- Number of times that the updated value of a row violated a
- <literal>NOT DEFERRABLE</literal> unique constraint while applying
- changes
+ Number of times that an updated row value violated a
+ <literal>NOT DEFERRABLE</literal> unique constraint during the
+ application of changes
</para></entry>
</row>
@@ -2213,8 +2213,8 @@ description | Waiting for a newly initialized WAL file to
reach durable storage
<structfield>update_missing_count</structfield> <type>bigint</type>
</para>
<para>
- Number of times that the tuple to be updated was not found while
applying
- changes
+ Number of times the tuple to be updated was not found during the
+ application of changes
</para></entry>
</row>
@@ -2223,11 +2223,11 @@ description | Waiting for a newly initialized WAL file
to reach durable storage
<structfield>delete_differ_count</structfield> <type>bigint</type>
</para>
<para>
- Number of times a delete was performed on a row that was previously
- modified by another source while applying changes. This conflict is
- counted only when the
+ Number of times a delete operation was applied to row that had been
+ previously modified by another source during the application of changes.
+ This conflict is counted only when the
<link
linkend="guc-track-commit-timestamp"><varname>track_commit_timestamp</varname></link>
- option is enabled on the subscriber
+ option is enabled on the subscriber.
</para></entry>
</row>
@@ -2236,8 +2236,8 @@ description | Waiting for a newly initialized WAL file to
reach durable storage
<structfield>delete_missing_count</structfield> <type>bigint</type>
</para>
<para>
- Number of times that the tuple to be deleted was not found while
applying
- changes
+ Number of times the tuple to be deleted was not found during the
application
+ of changes
</para></entry>
</row>
diff --git a/src/backend/utils/activity/pgstat_subscription.c
b/src/backend/utils/activity/pgstat_subscription.c
index e06c927..ebb0135 100644
--- a/src/backend/utils/activity/pgstat_subscription.c
+++ b/src/backend/utils/activity/pgstat_subscription.c
@@ -116,7 +116,7 @@ pgstat_subscription_flush_cb(PgStat_EntryRef *entry_ref,
bool nowait)
#define SUB_ACC(fld) shsubent->stats.fld += localent->fld
SUB_ACC(apply_error_count);
SUB_ACC(sync_error_count);
- for (int i = 0; i < CONFLICT_NUM_TYPES; i++)
+ for (int i = 0; i < NUM_CONFLICT_TYPES; i++)
SUB_ACC(conflict_count[i]);
#undef SUB_ACC
diff --git a/src/backend/utils/adt/pgstatfuncs.c
b/src/backend/utils/adt/pgstatfuncs.c
index 870aee8..7dd4b1e 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -2019,7 +2019,7 @@ pg_stat_get_subscription_stats(PG_FUNCTION_ARGS)
values[i++] = Int64GetDatum(subentry->sync_error_count);
/* conflict count */
- for (int nconflict = 0; nconflict < CONFLICT_NUM_TYPES; nconflict++)
+ for (int nconflict = 0; nconflict < NUM_CONFLICT_TYPES; nconflict++)
values[i++] =
Int64GetDatum(subentry->conflict_count[nconflict]);
/* stats_reset */
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index adb91f5..91b54aa 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -166,7 +166,7 @@ typedef struct PgStat_BackendSubEntry
{
PgStat_Counter apply_error_count;
PgStat_Counter sync_error_count;
- PgStat_Counter conflict_count[CONFLICT_NUM_TYPES];
+ PgStat_Counter conflict_count[NUM_CONFLICT_TYPES];
} PgStat_BackendSubEntry;
/* ----------
@@ -425,7 +425,7 @@ typedef struct PgStat_StatSubEntry
{
PgStat_Counter apply_error_count;
PgStat_Counter sync_error_count;
- PgStat_Counter conflict_count[CONFLICT_NUM_TYPES];
+ PgStat_Counter conflict_count[NUM_CONFLICT_TYPES];
TimestampTz stat_reset_timestamp;
} PgStat_StatSubEntry;
@@ -728,7 +728,7 @@ extern PgStat_SLRUStats *pgstat_fetch_slru(void);
*/
extern void pgstat_report_subscription_error(Oid subid, bool is_apply_error);
-extern void pgstat_report_subscription_conflict(Oid subid, ConflictType
conflict);
+extern void pgstat_report_subscription_conflict(Oid subid, ConflictType type);
extern void pgstat_create_subscription(Oid subid);
extern void pgstat_drop_subscription(Oid subid);
extern PgStat_StatSubEntry *pgstat_fetch_stat_subscription(Oid subid);
diff --git a/src/include/replication/conflict.h
b/src/include/replication/conflict.h
index 7232c88..37c9100 100644
--- a/src/include/replication/conflict.h
+++ b/src/include/replication/conflict.h
@@ -45,9 +45,9 @@ typedef enum
* complex rules than simple equality checks. These conflicts are left
for
* future improvements.
*/
-} ConflictType;
-#define CONFLICT_NUM_TYPES (CT_DELETE_MISSING + 1)
+ NUM_CONFLICT_TYPES /* must be last */
+} ConflictType;
extern bool GetTupleTransactionInfo(TupleTableSlot *localslot,
TransactionId *xmin,
diff --git a/src/test/subscription/t/026_stats.pl
b/src/test/subscription/t/026_stats.pl
index 4773528..e22b0b1 100644
--- a/src/test/subscription/t/026_stats.pl
+++ b/src/test/subscription/t/026_stats.pl
@@ -162,7 +162,7 @@ sub create_sub_pub_w_errors
));
# Update the data in the test table on the publisher. This should
generate
- # a conflict because it attempts to update a row on the subscriber that
has
+ # a conflict because it causes subscriber to attempt to update a row
that has
# been modified by a different origin.
$node_publisher->safe_psql($db, qq(UPDATE $table_name SET a = 2;));
@@ -184,7 +184,7 @@ sub create_sub_pub_w_errors
));
# Delete data from the test table on the publisher. This should
generate a
- # conflict because it attempts to delete a row on the subscriber that
has
+ # conflict because it causes subscriber to attempt to delete a row that
has
# been modified by a different origin.
$node_publisher->safe_psql($db, qq(DELETE FROM $table_name;));
@@ -216,7 +216,7 @@ my ($pub1_name, $sub1_name) =
create_sub_pub_w_errors($node_publisher, $node_subscriber, $db,
$table1_name);
-# Apply and Sync errors are > 0 and reset timestamp is NULL
+# Apply errors, sync errors, and conflicts are > 0 and reset timestamp is NULL
is( $node_subscriber->safe_psql(
$db,
qq(SELECT apply_error_count > 0,
@@ -232,7 +232,7 @@ is( $node_subscriber->safe_psql(
WHERE subname = '$sub1_name')
),
qq(t|t|t|t|t|t|t|t|t),
- qq(Check that apply errors, sync errors, and conflicts are both > 0 and
stats_reset is NULL for subscription '$sub1_name'.)
+ qq(Check that apply errors, sync errors, and conflicts are > 0 and
stats_reset is NULL for subscription '$sub1_name'.)
);
# Reset a single subscription
@@ -240,7 +240,7 @@ $node_subscriber->safe_psql($db,
qq(SELECT pg_stat_reset_subscription_stats((SELECT subid FROM
pg_stat_subscription_stats WHERE subname = '$sub1_name')))
);
-# Apply and Sync errors are 0 and stats reset is not NULL
+# Apply errors, sync errors, and conflicts are 0 and stats reset is not NULL
is( $node_subscriber->safe_psql(
$db,
qq(SELECT apply_error_count = 0,
@@ -256,7 +256,7 @@ is( $node_subscriber->safe_psql(
WHERE subname = '$sub1_name')
),
qq(t|t|t|t|t|t|t|t|t),
- qq(Confirm that apply errors, sync errors, and conflicts are both 0 and
stats_reset is not NULL after reset for subscription '$sub1_name'.)
+ qq(Confirm that apply errors, sync errors, and conflicts are 0 and
stats_reset is not NULL after reset for subscription '$sub1_name'.)
);
# Get reset timestamp
@@ -286,7 +286,7 @@ my ($pub2_name, $sub2_name) =
create_sub_pub_w_errors($node_publisher, $node_subscriber, $db,
$table2_name);
-# Apply and Sync errors are > 0 and reset timestamp is NULL
+# Apply errors, sync errors, and conflicts are > 0 and reset timestamp is NULL
is( $node_subscriber->safe_psql(
$db,
qq(SELECT apply_error_count > 0,
@@ -302,14 +302,14 @@ is( $node_subscriber->safe_psql(
WHERE subname = '$sub2_name')
),
qq(t|t|t|t|t|t|t|t|t),
- qq(Confirm that apply errors, sync errors, and conflicts are both > 0
and stats_reset is NULL for sub '$sub2_name'.)
+ qq(Confirm that apply errors, sync errors, and conflicts are > 0 and
stats_reset is NULL for sub '$sub2_name'.)
);
# Reset all subscriptions
$node_subscriber->safe_psql($db,
qq(SELECT pg_stat_reset_subscription_stats(NULL)));
-# Apply and Sync errors are 0 and stats reset is not NULL
+# Apply errors, sync errors, and conflicts are 0 and stats reset is not NULL
is( $node_subscriber->safe_psql(
$db,
qq(SELECT apply_error_count = 0,
@@ -325,7 +325,7 @@ is( $node_subscriber->safe_psql(
WHERE subname = '$sub1_name')
),
qq(t|t|t|t|t|t|t|t|t),
- qq(Confirm that apply errors, sync errors, and conflicts are both 0 and
stats_reset is not NULL for sub '$sub1_name' after reset.)
+ qq(Confirm that apply errors, sync errors, and conflicts are 0 and
stats_reset is not NULL for sub '$sub1_name' after reset.)
);
is( $node_subscriber->safe_psql(
@@ -343,7 +343,7 @@ is( $node_subscriber->safe_psql(
WHERE subname = '$sub2_name')
),
qq(t|t|t|t|t|t|t|t|t),
- qq(Confirm that apply errors, sync errors, and conflicts are both 0 and
stats_reset is not NULL for sub '$sub2_name' after reset.)
+ qq(Confirm that apply errors, sync errors, and conflicts are 0 and
stats_reset is not NULL for sub '$sub2_name' after reset.)
);
$reset_time1 = $node_subscriber->safe_psql($db,
