On Wed, 8 Oct 2025 at 3:15 PM, Amit Kapila <[email protected]> wrote:
> On Wed, Oct 8, 2025 at 2:41 PM Dilip Kumar <[email protected]> wrote: > > > > On Tue, Oct 7, 2025 at 4:52 PM vignesh C <[email protected]> wrote: > > > > > > On Tue, 7 Oct 2025 at 12:09, Amit Kapila <[email protected]> > 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
