Hi Ajin.
Some review comments for patch v4-0001
======
src/backend/commands/sequence.c
GetSequence:
1.
+/*
+ * Read the current sequence values (last_value and is_called)
+ *
+ * This is a read-only operation that acquires AccessShareLock on the sequence.
+ * Used by logical replication sequence synchronization to detect drift.
+ */
The comment seems stale. e.g. the function is not acquiring a lock
anymore, contrary to what that comment says.
======
.../replication/logical/sequencesync.c
2.
-static List *seqinfos = NIL;
The removal of this global was not strictly part of this patch; it is
more like a prerequisite to make everything tidier, so your new code
does not go further down that track of side-affecting a global. From
that POV, I thought this global removal should be implemented as a
first/separate (0001) patch so that it might be quickly reviewed and
committed independently of the new seq-sync logic.
~~~
LogicalRepSyncSequences:
3.
+ /* Error on unexpected states */
+ if (relstate != SUBREL_STATE_INIT && relstate != SUBREL_STATE_READY)
+ {
+ table_close(sequence_rel, NoLock);
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("unexpected relstate '%c' for sequence \"%s.%s\" in
subscription \"%s\"",
+ relstate,
+ get_namespace_name(RelationGetNamespace(sequence_rel)),
+ RelationGetRelationName(sequence_rel),
+ MySubscription->name)));
+ }
+
How is this possible? Should it just be Assert?
~~~
start_sequence_sync:
4.
+ /*
+ * Synchronize all sequences (both READY and INIT states).
+ * The function will process INIT sequences first, then READY sequences.
+ */
+ sequence_copied = LogicalRepSyncSequences(LogRepWorkerWalRcvConn);
Why is talking about the processing order relevant?
======
src/backend/replication/logical/syncutils.c
5.
+ /* Check if any new sequences need syncing */
+ ProcessSequencesForSync();
+
Maybe don't say "new" because IIUC it also handles older sequences
where the values have drifted.
======
src/test/subscription/t/036_sequences.pl
6.
-$result = $node_publisher->safe_psql(
- 'postgres', qq(
- SELECT last_value, is_called FROM regress_s1;
-));
-is($result, '200|t', 'Check sequence value in the publisher');
-
-# Check - existing sequence ('regress_s1') is not synced
-$result = $node_subscriber->safe_psql(
- 'postgres', qq(
- SELECT last_value, is_called FROM regress_s1;
-));
-is($result, '100|t', 'REFRESH PUBLICATION will not sync existing sequence');
-
Since you are no longer testing "existing sequences" in this test
part, should you also remove the earlier INSERT for 'regress_s1'?
======
Kind Regards,
Peter Smith.
Fujitsu Australia.
