Re: Logical Replication of sequences

On Sat, Oct 11, 2025 at 7:42 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> The attached patch has the changes for the same.
>

I have a few more comments on 0002 patch:
1. In check_publications_origin(), isn't it better to name
check_table_sync as check_sync as it is used for both tables and
sequences?

2. In check_publications_origin(), for all three queries, only the
following part seems to be different:

< 19
" LATERAL pg_get_publication_tables(P.pubname) GPT\n"
>=19
only_sequences
" LATERAL pg_get_publication_sequences(P.pubname) GPT\n"
else
" CROSS JOIN LATERAL (SELECT relid FROM
pg_get_publication_tables(P.pubname) UNION ALL"
" SELECT relid FROM
pg_get_publication_sequences(P.pubname)) GPT\n"

2A. Can this part of the query be made dynamic, and then we can have a
query instead of three? If so, I think it would simplify the code.
What do you think?
2B. Can we add/modify the comment atop check_publications_origin to
mention about sequence case?

3.
void
-CheckSubscriptionRelkind(char relkind, const char *nspname,
+CheckSubscriptionRelkind(char relkind, char pubrelkind, const char *nspname,
const char *relname)
{
- if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
+ if (relkind != RELKIND_RELATION &&
+ relkind != RELKIND_PARTITIONED_TABLE &&
+ relkind != RELKIND_SEQUENCE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("cannot use relation \"%s.%s\" as logical replication target",
nspname, relname),
errdetail_relkind_not_supported(relkind)));
+
+ if (pubrelkind == '\0')
+ return;

This looks ad hoc. I think it would be better if the caller passes the
same value for local and remote relkind to this function. And
accordingly, change the name of the first two parameters.

4.
+static void
+AlterSubscription_refresh_seq(Subscription *sub)
…
+ check_publications_origin(wrconn, sub->publications, false,
+ sub->retaindeadtuples, sub->origin, NULL, 0,
+ sub->name, true);

Write a few comments to explain why it is necessary to check origins
in this case. If the additional comments atop
check_publications_origin() cover this case, then it's okay as it is.

5.
AlterSubscription_refresh()
- sub_remove_rels[remove_rel_len].relid = relid;
- sub_remove_rels[remove_rel_len++].state = state;
…
- char originname[NAMEDATALEN];
+ SubRemoveRels *rel = palloc(sizeof(SubRemoveRels));
+
+ rel->relid = relid;
+ rel->state = state;
+
+ sub_remove_rels = lappend(sub_remove_rels, rel);

Why do we change an offset based array into list? It looks slightly
odd that in the same function one of the other similar array
pubrel_local_oids is not converted when the above is converted. And
even if we do so, I don't think we need a retail free
(list_free_deep(sub_remove_rels);) as the memory allocation here is in
portal context which should be reset by end of the current statement
execution.

--
With Regards,
Amit Kapila.