Re: Logical Replication of sequences

> On Sep 5, 2025, at 13:49, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Fri, 5 Sept 2025 at 03:04, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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/