Re: Logical Replication of sequences

> On Sep 5, 2025, at 13:49, vignesh C <vignesh21@gmail.com> wrote:
> 
> On Fri, 5 Sept 2025 at 03:04, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> 
>> Please rebase the patches as they conflict with current HEAD (due to
>> commit 6359989654).
> 
> Attached a rebased version of the patches.
> 
> Regards,
> Vignesh
> <v20250905-0001-Enhance-pg_get_sequence_data-function.patch><v20250905-0003-Reorganize-tablesync-Code-and-Introduce-sy.patch><v20250905-0002-Introduce-ALL-SEQUENCES-support-for-Postgr.patch><v20250905-0004-Update-ALTER-SUBSCRIPTION-REFRESH-to-ALTER.patch><v20250905-0005-Introduce-REFRESH-PUBLICATION-SEQUENCES-fo.patch><v20250905-0006-New-worker-for-sequence-synchronization-du.patch><v20250905-0007-Documentation-for-sequence-synchronization.patch>


A few small comments:

1 - 0001
```
diff --git a/src/test/regress/sql/sequence.sql b/src/test/regress/sql/sequence.sql
index 2c220b60749..c8adddbfa31 100644
--- a/src/test/regress/sql/sequence.sql
+++ b/src/test/regress/sql/sequence.sql
@@ -414,6 +414,6 @@ SELECT nextval('test_seq1');
 SELECT nextval('test_seq1');
 
 -- pg_get_sequence_data
-SELECT * FROM pg_get_sequence_data('test_seq1');
+SELECT last_value, is_called, log_cnt, page_lsn <= pg_current_wal_lsn() as lsn FROM pg_get_sequence_data('test_seq1');
 
 DROP SEQUENCE test_seq1;
```

As it shows log_cnt now, after calling pg_get_sequence_data(), I suggest add 8 nextval(), so that sequence goes to 11, and log_cnt should become to 22.

2 - 0002
```
-	if (schemaidlist && pubform->puballtables)
+	pub_type = makeStringInfo();
+
+	appendStringInfo(pub_type, "%s", pubform->puballtables && pubform->puballsequences ? "FOR ALL TABLES, ALL SEQUENCES" :
+					 pubform->puballtables ? "FOR ALL TABLES" : "FOR ALL SEQUENCES");
+
+	if (schemaidlist && (pubform->puballtables || pubform->puballsequences))
 		ereport(ERROR,
 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-				 errmsg("publication \"%s\" is defined as FOR ALL TABLES",
-						NameStr(pubform->pubname)),
-				 errdetail("Schemas cannot be added to or dropped from FOR ALL TABLES publications.")));
+				 errmsg("publication \"%s\" is defined as %s",
+						NameStr(pubform->pubname), pub_type->data),
+				 errdetail("Schemas cannot be added to or dropped from %s publications.", pub_type->data)));
```

Here you build a string at runtime and inject it into log message, which seems to break some PG rules. In one of my previous review, I raised a comment for removing some duplicate code using this way, and get this info: https://www.postgresql.org/message-id/397c16a7-f57b-4f81-8497-6d692a9bf596%40eisentraut.org

3 - 0005
```
+		/*
+		 * Skip sequence tuples. If even a single table tuple exists then the
+		 * subscription has tables.
+		 */
+		if (get_rel_relkind(subrel->srrelid) != RELKIND_SEQUENCE)
+		{
+			has_subrels = true;
+			break;
+		}
```
For publication, only valid relkind are RELKIND_RELATION, RELKIND_PARTITIONED_TABLE and newly added RELKIND_SEQUENCE. Here you want to check for table, using “!=  RELKIND_SEQUENCE” works. But I think doing “ kind == RELKIND_RELATION || kind == RELKIND_PARTITIONED_TABLE” is clearer and more reliable. Consider if some other kind is added, then “kind != RELKIND_SEQUENCE” might be broken, and hard to find the root cause.

4 -0006
```
-		appendStringInfo(&cmd, "SELECT DISTINCT n.nspname, c.relname, gpt.attrs\n"
+		appendStringInfo(&cmd, "SELECT DISTINCT n.nspname, c.relname, gpt.attrs,  c.relkind\n"
```
There is an extra whitespace before “c.relkind”.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/