Re: Logical Replication of sequences

Hi Vignesh,

Some review comments for patch v20251105-0001 (stats counter)

======
General.

1.
It was not obvious to me this counter is actually counting. Both the
commit message and the docs say it counts "errors" during sequence
synchronization. AFAIK the sequencesync worker processes sequences in
"batches" of ~100, so in reality even if there are a dozen different
sequences with problems, the worker combines them all and reports just
one actual ERROR, doesn't it? So this counter appears to be just an
indication that "something bad happened" during the synchronization.
Even a high count value doesn't mean there are lots of sequences with
errors; there might just be 1 sequence error that has remained
unaddressed and so keeps incrementing the count every time the
sequencesync reruns and re-fails, right?

Maybe the descriptions can clarify what the counter really means?

======
Commit message

2.
See general comment #1

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

sequence_sync_error_count:

3.
+ <para>
+ Number of times an error occurred during the sequence synchronization
+ </para></entry>

See general comment #1

~~~

sync_error_count:

4.
Now there are 2 kinds of synchronization workers -- tablesync and sequencesync.

But there was already a stats counter called "sync_error_count". Are
you worried that this name now seems ambiguous? (e.g. versus if it was
called "table_sync_error_count"). How can we prevent users from being
misled by this old generic looking name?

======
src/backend/catalog/system_views.sql

5.
ss.apply_error_count,
+ ss.seq_sync_error_count,
ss.sync_error_count,

The documentation said the new column was called "sequence_sync_error_count".

======
src/backend/utils/adt/pgstatfuncs.c

pg_stat_get_subscription_stats:

6.
- TupleDescInitEntry(tupdesc, (AttrNumber) 3, "sync_error_count",
+ TupleDescInitEntry(tupdesc, (AttrNumber) 3, "seq_sync_error_count",
INT8OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 4, "confl_insert_exists",
+ TupleDescInitEntry(tupdesc, (AttrNumber) 4, "sync_error_count",
INT8OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 5, "confl_update_origin_differs",
+ TupleDescInitEntry(tupdesc, (AttrNumber) 5, "confl_insert_exists",
INT8OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 6, "confl_update_exists",
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "confl_update_origin_differs",
INT8OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 7, "confl_update_deleted",
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "confl_update_exists",
INT8OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 8, "confl_update_missing",
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "confl_update_deleted",
INT8OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 9, "confl_delete_origin_differs",
+ TupleDescInitEntry(tupdesc, (AttrNumber) 9, "confl_update_missing",
INT8OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 10, "confl_delete_missing",
+ TupleDescInitEntry(tupdesc, (AttrNumber) 10, "confl_delete_origin_differs",
INT8OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 11,
"confl_multiple_unique_conflicts",
+ TupleDescInitEntry(tupdesc, (AttrNumber) 11, "confl_delete_missing",
INT8OID, -1, 0);
- TupleDescInitEntry(tupdesc, (AttrNumber) 12, "stats_reset",
+ TupleDescInitEntry(tupdesc, (AttrNumber) 12,
"confl_multiple_unique_conflicts",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 13, "stats_reset",

6a.
IMO now is a good opportunity to make more use of that 'i' variable,
instead of hardwiring all the column indexes 1,2,3...13. That way
would avoid all this code churn every time a new stats column gets
added in the future.

~

6b.
The documentation said the new column was called "sequence_sync_error_count".

======
src/test/subscription/t/026_stats.pl

7.
sub create_sub_pub_w_errors
{
- my ($node_publisher, $node_subscriber, $db, $table_name) = @_;
+ my ($node_publisher, $node_subscriber, $db, $table_name, $sequence_name)
+ = @_;
# Initial table setup on both publisher and subscriber. On subscriber we
# create the same tables but with primary keys. Also, insert some data that
# will conflict with the data replicated from publisher later.

~

I think that comment should also mention that you've made a subscriber
SEQUENCE with a different INCREMENT to deliberately clash with the
publisher sequence of the same name.

Also typos:
/On subscriber/On the subscriber/
/from publisher/from the publisher/

~~~

8.
+ # Change the sequence start value back to the default on the subscriber so
+ # it doesn't error out.
+ $node_subscriber->safe_psql($db,
+ qq(ALTER SEQUENCE $sequence_name INCREMENT 1));
+

That comment is not quite right. e.g. you are changing the INCREMENT
to match, not the "start value".

~~~

9.
- # Wait for initial tablesync to finish.
+ # Wait for initial tablesync sync to finish.

"tablesync sync"?

Was this change needed at all? If any change is needed at all (I am
not saying it is) I thought it should say something more like: "Wait
for the initial tablesync to finish successfully."

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