Re: Logical Replication of sequences

On Wed, 8 Oct 2025 at 3:15 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Wed, Oct 8, 2025 at 2:41 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Tue, Oct 7, 2025 at 4:52 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Tue, 7 Oct 2025 at 12:09, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > > >
> >
> > I think the patch is mostly LGTM, I have 2 suggestions, see if you
> > think this is useful.
> >
> > 1.
> > postgres[1390699]=# CREATE PUBLICATION pub FOR ALL SEQUENCES, ALL
> > TABLES WITH (publish = insert);
> > NOTICE: 55000: publication parameters are not applicable to sequence
> > synchronization and will be ignored
> > LOCATION: CreatePublication, publicationcmds.c:905
> >
> > IMHO this notice seems confusing, from this it appears that (publish =
> > insert) is ignored completely, but actually it is is not ignored for
> > table, should we explicitely say that it will be ignored only for
> > sequences. Something like below?
> >
> > "publication parameters are not applicable to sequence synchronization
> > so it will be used only for tables and will be ignored for sequence
> > synchronization"
> > or
> > "publication parameters are not applicable to sequence synchronization
> > so it will be ignored for the sequence synchronization"
> >
>
> How about a slightly shorter form like: 'publication parameters are
> not applicable to sequence synchronization and will be ignored for
> sequences'?

Works for me.

> 2.
> > + if (schemaidlist && (pubform->puballtables ||
> pubform->puballsequences))
> > + {
> > + if (pubform->puballtables && pubform->puballsequences)
> > + ereport(ERROR,
> > + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> > + errmsg("publication \"%s\" is defined as FOR ALL TABLES, ALL
> SEQUENCES",
> > + NameStr(pubform->pubname)),
> > + errdetail("Schemas cannot be added to or dropped from FOR ALL
> > TABLES, ALL SEQUENCES publications."));
> > + else if (pubform->puballtables)
> > + 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."));
> > + else
> > + ereport(ERROR,
> > + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> > + errmsg("publication \"%s\" is defined as FOR ALL SEQUENCES",
> > + NameStr(pubform->pubname)),
> > + errdetail("Schemas cannot be added to or dropped from FOR ALL
> > SEQUENCES publications."));
> > + }
> >
> > Can't we make this a single generic error message instead of
> > duplicating for each combination? Something like
> >
> > errmsg("publication \"%s\" is defined as FOR ALL TABLES or ALL
> SEQUENCES",
> > NameStr(pubform->pubname)),
> > errdetail("Schemas cannot be added to or dropped from FOR ALL TABLES
> > or ALL SEQUENCES publications."));
> >
>
> Yeah, we can do that but Note that these messages are for the
> existing publication and we are aware of its publicized contents, so
> we can give clear messages to users. Why make it ambiguous?

Hmm, yeah that makes sense.

—
Dilip