On Fri, 10 Oct 2025 at 13:03, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 9, 2025 at 3:08 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > >
> > > So my suggestion is very different. Just this:
> > > "ALTER SUBSCRIPTION sub REFRESH SEQUENCES"
> > >
> > > I feel this is entirely consistent, because:
> > >
> > > PUBLICATION objects have changed. Refresh me the new objects => ALTER
> > > SUBSCRIPTION sub REFRESH PUBLICATION;
> > >
> > > SEQUENCE values have changed. Refresh me the new values => ALTER
> > > SUBSCRIPTION sub REFRESH SEQUENCES;
> >
> > +1 for this syntax. Here is an updated patch having the changes for the same.
> >
>
> Few comments:
> =============
> 1.
> + /* From version 19, inclusion of sequences in the target is supported */
> + if (server_version >= 190000)
> + appendStringInfo(&cmd,
> + "UNION ALL\n"
> + " SELECT DISTINCT s.schemaname, s.sequencename, NULL::int2vector AS
> attrs, 'S'::\"char\" AS relkind\n"
>
> Instead of hard coding 'S', can we use RELKIND_SEQUENCE?
Modified
> 2.
> else
> {
> tableRow[2] = NAMEARRAYOID;
> - appendStringInfoString(&cmd, "SELECT DISTINCT t.schemaname, t.tablename \n");
> + appendStringInfoString(&cmd, "SELECT DISTINCT t.schemaname, t.tablename\n");
>
> Why the above change?
This is not required, removed this change
> 3.
> + pubisseq = (relinfo->relkind == RELKIND_SEQUENCE);
> + subisseq = (relkind == RELKIND_SEQUENCE);
> +
> + /*
> + * Allow RELKIND_RELATION and RELKIND_PARTITIONED_TABLE to be
> + * treated interchangeably, but ensure that sequences
> + * (RELKIND_SEQUENCE) match exactly on both publisher and
> + * subscriber.
> + */
> + if (pubisseq != subisseq)
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>
> We can directly compare relkind here and avoid having two extra
> variables. The same code is present in AlterSubscription_refresh. So,
> we can make the similar change there as well. BTW, the same scenario
> could happen between table and sequences, no? If so, then we should
> deal with that as well. It would be better if can make these checks as
> part of CheckSubscriptionRelkind().
Modified
> 4.
> + errmsg("relation \"%s.%s\" has relkind \"%c\" on the publisher but
> relkind \"%c\" on the subscriber",
>
> I would like this message to be bit short and speak in terms of source
> and target, something like: errmsg("relation \"%s.%s\" type mismatch:
> source \"%c\", target \"%c\"")
Modified
> 5.
> + *
> + * XXX: Currently, a replication slot is created for all
> + * subscriptions, including those for sequence-only publications.
> + * However, this is unnecessary, as incremental synchronization of
> + * sequences is not supported.
>
> Can we try to avoid creating slot/origin for sequence-only
> subscriptions? We don't want to make code complicated due to this, so
> try to create a top-up patch so that we can evaluate this change
> separately?
I will do this and post it along with the next version
> 6
> @@ -2403,11 +2503,15 @@ check_publications_origin(WalReceiverConn
> *wrconn, List *publications,
> for (i = 0; i < subrel_count; i++)
> {
> Oid relid = subrel_local_oids[i];
> - char *schemaname = get_namespace_name(get_rel_namespace(relid));
> - char *tablename = get_rel_name(relid);
> - appendStringInfo(&cmd, "AND NOT (N.nspname = '%s' AND C.relname = '%s')\n",
> - schemaname, tablename);
> + if (get_rel_relkind(relid) != RELKIND_SEQUENCE)
>
> Why do we have the above check in the 0002 patch? We can add some
> comments to clarify the same, if it is required. Also, we should do
> this origin check during ALTER SUBSCRIPTION … REFRESH command as we
> don't have incremental WAL based filtering of origin for sequences.
This check is not required, as there is a scenario where N1
replicates sequences to N2 and N2 replicates sequences to N3. This
warning will be helpful
The attached patch has the changes for the same.
Regards,
Vignesh
