Re: Logical Replication of sequences

On Wed, Oct 8, 2025 at 2:41 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Oct 7, 2025 at 4:52 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Tue, 7 Oct 2025 at 12:09, Amit Kapila <amit.kapila16@gmail.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'?

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