On Tue, Jul 21, 2026 at 11:23 AM Peter Smith <[email protected]> wrote: > > Some review comments for v22* > > ====== > > 1. GENERAL -- Consistent wording in messages > > I found some examples of error messages that could be made more similar: > > 1a. > "modify" versus "change" > > modify... > > + errhint("Use ALTER PUBLICATION ... SET TABLES IN SCHEMA ... EXCEPT > to modify the EXCEPT list."))); > > versus (Change) > > + errhint("Change the EXCEPT clause using ALTER PUBLICATION ... SET > TABLES IN SCHEMA ... EXCEPT.")); > + errhint("Change the EXCEPT clause using ALTER PUBLICATION ... SET > TABLES IN SCHEMA ... EXCEPT."))); > > + errhint("%s", has_alltables_pub ? > + _("Change the EXCEPT clause using ALTER PUBLICATION ... SET ALL > TABLES ... EXCEPT.") : > + _("Change the EXCEPT clause using ALTER PUBLICATION ... SET TABLES > IN SCHEMA ... EXCEPT."))); > > ~ > > Shouldn't 1st one (from patch v22-0004) be identical to the others? e.g.: > Change the EXCEPT clause using ALTER PUBLICATION ... SET TABLES IN > SCHEMA ... EXCEPT. > > ~~~ > > 1b. > "referenced" versus "named" versus "listed" > > referenced... > > + errmsg_plural("cannot attach table \"%s\" as partition because it is > referenced in EXCEPT clause of publication %s", > + "cannot attach table \"%s\" as partition because it is referenced > in EXCEPT clause of publications %s", > > versus (named) > > + errdetail("Partition root \"%s\" is named in the publication's > EXCEPT clause for schema \"%s\".", > + quote_qualified_identifier(get_namespace_name(get_rel_namespace(root)), > + get_rel_name(root)), > > + errdetail("Partition root \"%s\" is named in the publication's > EXCEPT clause for schema \"%s\".", > + RelationGetQualifiedRelationName(pri->relation), > + get_namespace_name(RelationGetNamespace(pri->relation))))); > > versus (listed) > > + errdetail("Ancestor \"%s\" of partition \"%s\" is currently listed > in the EXCEPT clause of the publication.", > + quote_qualified_identifier(get_namespace_name(get_rel_namespace(root)), > + get_rel_name(root)), > > + errdetail("The table is currently listed in the EXCEPT clause of the > publication."), > > + * publication, excluding any tables listed in the EXCEPT clause. > > + errdetail("Ancestor \"%s\" of partition \"%s\" is currently listed > in the EXCEPT clause of this command.", > + RelationGetQualifiedRelationName(pri->relation), > + > quote_qualified_identifier(get_namespace_name(get_rel_namespace(explicitrelid)), > > ~ > > IMO there is no real difference here in saying "referenced" v "named" > v "listed", so why not make them all "named" so they are alike. > > ~~~ > > 1c. > "EXCEPT list" versus "EXCEPT clause" > > EXCEPT list: > > + errhint("Use ALTER PUBLICATION ... SET TABLES IN SCHEMA ... EXCEPT > to modify the EXCEPT list."))); > > + * A partition cannot be included when its partition root is in the > + * same publication's EXCEPT list. Doing so would leave the catalog > > + errhint("Change EXCEPT list using ALTER PUBLICATION ... SET TABLES > IN SCHEMA ... EXCEPT.")); > > + errhint("Change EXCEPT list using ALTER PUBLICATION ... SET TABLES > IN SCHEMA ... EXCEPT."))); > > + /* filter out any tables that appear in the EXCEPT list */ > > + * A partition whose root is in the publication's EXCEPT list > + * is also excluded, even if the partition itself lives in a > > + * Validate that each excluded table is not also in the > + * explicit table list (which would be contradictory), and > + * that no explicit partition's root is in the except list. > > etc. > > I don't recognise the reasons for the difference -- it all seems a bit > random. There are a lot more "EXCEPT clause" than there are "EXCEPT > list". > > EXCEPT list makes sense for some comments and parameters, but for > user-facing messages maybe those should consistently say "EXCEPT > clause"? >
Thanks, agreed on all three. Standardized the user-facing messages using "clause" and updated the comments to use "EXCEPT clause", keeping "list" only where it refers to the actual List variable. I'll keep an eye on the next iterations and correct in case I missed any. > ====== > src/backend/catalog/pg_publication.c > > 2. > publication_add_relation: > On Mon, Jul 20, 2026 at 6:22 PM Nisha Moond <[email protected]> wrote: > > > > On Thu, Jul 16, 2026 at 2:04 PM Peter Smith <[email protected]> wrote: > > > > > > > > 7. > > > + errdetail("Ancestor \"%s\" of partition \"%s\" is currently listed > > > in the EXCEPT clause of the publication.", > > > + > > > quote_qualified_identifier(get_namespace_name(get_rel_namespace(root)), > > > > > > Similar (singular) comment to before. > > > > > > /in the EXCEPT clause/in an EXCEPT clause/ > > > > > > > It looks like comment #4 is repeated here. In any case, I addressed > > both occurrences of the errdetail (in check_publication_add_relation() > > and CheckExceptNotInTableList()): > > "Ancestor \"%s\" of partition \"%s\" is curre..". > > > > Looks like a previous review comment #7 was missed despite saying it > was addressed. It still says: > + errdetail("The table is currently listed in the EXCEPT clause of the > publication."), > Sorry, I misread the comment; it's corrected now. > ====== > src/backend/commands/publicationcmds.c > > ProcessSchemaExceptTables > > 3. > (patch 0002) > > + * *schemas: accumulates all schema OIDs seen so far in this statement. > + * *schemas_with_except: tracks which schema OIDs have already been seen > + * with an EXCEPT clause in this statement. > > Maybe this can clarify that `*schemas_with_except` OIDs will be a > subset of `*schemas`.OIDs > > SUGGESTION > *schemas_with_except: This is a subset of *schemas. Tracks which of > the seen schemas of this statement had an EXCEPT clause. > Done. ~~~ -- Thanks, Nisha
