Re: Logical Replication of sequences

On Fri, 5 Sept 2025 at 13:54, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
> 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.

Could you please explain the reason you’d like this to be done?

> 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

Modified

> 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.

Modified

> 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”.

Modified

Thanks for the comments, the attached v20250908 version patch has the
changes for the same.

Regards,
Vignesh