Re: Skipping schema changes in publication

On Mon, 22 Dec 2025 at 11:37, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shlok.
>
> Some review comments for patch v33-0001 (code part)
>
> ======
> src/backend/catalog/pg_publication.c
>
> GetPublicationRelationsInternal:
>
> 1.
> Static function names should be snake_case.
>
> ~~~
>
> GetPublicationIncludedRelations:
>
> 2.
> +/*
> + * Return the list of relation OIDs for a publication.
> + *
> + * For a FOR TABLE publication, this returns the list of relations explicitly
> + * included in the publication.
> + *
> + * Publications declared with FOR ALL TABLES or FOR ALL SEQUENCES should use
> + * GetAllPublicationRelations() to obtain the complete set of tables covered by
> + * the publication.
> + */
> +List *
> +GetPublicationIncludedRelations(Oid pubid, PublicationPartOpt pub_partopt)
> +{
> + Assert(!GetPublication(pubid)->alltables);
> +
> + return GetPublicationRelationsInternal(pubid, pub_partopt, false);
> +}
>
> Why isn't the Assert also saying something about puballsequences, as
> mentioned in the function comment?
>
I reported a similar kind of issue in HEAD in [1].
As per the latest discussion, I understood that it is ok to call this
function for ALL SEQUENCES.
I have updated the comment.

> ~~~
>
> GetAllPublicationRelations:
>
> 3.
> + * root partitioned tables. The list also excludes tables that are
> + * explicitly excluded via the EXCEPT TABLE clause of the publication
> + * identified by pubid. Neither of these rules applies to FOR ALL SEQUENCES
> + * publications.
>
> 3.
> It seems wrong to say "FOR ALL SEQUENCES" ... that seems to assume the
> "FOR ALL SEQUENCES" and "FOR ALL TABLES" cannot co-exist. Did you mean
> "Neither of ... to published sequences"?
>
I have modified the comment.

> ~
>
> 4.
> -GetAllPublicationRelations(char relkind, bool pubviaroot)
> +GetAllPublicationRelations(Oid pubid, char relkind, bool pubviaroot)
>
> There are tricky rules about relation vs sequences and the
> publish_via_partition_root parameter value. It would be better if you
> encapsulate all this within this function. Specifically, it would be
> simpler if you passed the 'Publication' arg instead of the pubid. Then
> you can get the pubviaroot value from that (within the function)
> instead of passing around "fake" values of false when you are looking
> at RELKIND_SEQUENCE.
> ======
> src/backend/commands/publicationcmds.c
>
> ObjectsInAllPublicationToOids:
>
> 5.
> + foreach_ptr(PublicationAllObjSpec, puballobj, puballobjspec_list)
> + {
> + if (puballobj->pubobjtype != PUBLICATION_ALL_TABLES)
> + continue;
> +
> + foreach_ptr(PublicationObjSpec, pubobj, puballobj->except_tables)
> + {
> + pubobj->pubtable->except = true;
> + *rels = lappend(*rels, pubobj->pubtable);
> + }
> + }
>
> I think it's tidier to code this like below:
>
> if (puballobj->pubobjtype == PUBLICATION_ALL_TABLES)
> {
> foreach_ptr...
> }
>
> ~~~
>
> pub_contains_invalid_column:
>
> 6.
> bool
> pub_contains_invalid_column(Oid pubid, Relation relation, List *ancestors,
> bool pubviaroot, char pubgencols_type,
> - bool *invalid_column_list,
> - bool *invalid_gen_col)
> + bool *invalid_column_list, bool *invalid_gen_col)
>
> Why does this change even exist at all in this patch?
This change is not required. I have reverted it.

>
> ~~~
>
> CreatePublication:
>
> 7.
> + /*
> + * If the publication is for ALL TABLES and 'relations' is not empty, it
> + * indicates that some relations should be excluded from the publication.
> + * Add those excluded relations to the publication with 'prexcept' set to
> + * true. Otherwise, 'relations' contains the list of relations to be
> + * explicitly included in the publication.
> + */
> + if (relations != NIL)
> + {
> + List *rels;
> +
> + rels = OpenTableList(relations);
> + TransformPubWhereClauses(rels, pstate->p_sourcetext,
> + publish_via_partition_root);
> +
> + CheckPubRelationColumnList(stmt->pubname, rels,
> + schemaidlist != NIL,
> + publish_via_partition_root);
> +
> + PublicationAddTables(puboid, rels, true, NULL);
> + CloseTableList(rels);
> + }
> +
>
> The comment and the code don't match. The comment is talking about
> rules for FOR ALL TABLES, but puballtables is not part of any
> condition here (??). Was all this supposed to be within the "if
> (stmt->for_all_tables)" code block?
>
For both ALL TABLES publication and non-ALL TABLES publication we need
the same code block.
Setting of prexcept flag will be handled in PublicationAddTables.
This comment clarifies what the list 'relations' would mean for ALL
TABLES publication and non-ALL TABLES publication

> ======
> src/bin/pg_dump/pg_dump.c
>
> 8.
> - "SELECT tableoid, oid, prpubid, prrelid, "
> + "SELECT tableoid, oid, prpubid, prrelid,\n"
> "pg_catalog.pg_get_expr(prqual, prrelid) AS prrelqual, "
> "(CASE\n"
> " WHEN pr.prattrs IS NOT NULL THEN\n"
> @@ -4868,6 +4929,9 @@ getPublicationTables(Archive *fout, TableInfo
> tblinfo[], int numTables)
> " WHERE attrelid = pr.prrelid AND attnum = prattrs[s])\n"
> " ELSE NULL END) prattrs "
> "FROM pg_catalog.pg_publication_rel pr");
> + if (fout->remoteVersion >= 190000)
> + appendPQExpBufferStr(query, " WHERE prexcept = false");
>
> 8a
> Isn't it better to qualify everything here with the alias 'pr'?
>
It is an existing code. So I prefer not to modify it in this patch. I
have added the alias for the column added by this patch.

> ~
>
> 8b.
> Also "WHERE NOT pr.prexcept;" might be more conssitent with other code
> I saw in describe.c
>
> ======
> src/bin/pg_dump/pg_dump.h
>
> 9.
> PublishGencolsType pubgencols_type;
> + SimplePtrList excepttbls;
> } PublicationInfo;
>
> How about "tables instead of "tbls" (e.g. "excepttables" or
> "except_tables") here? That would also be more consistent with the
> other puballtables member.
>
> ======
> src/test/regress/sql/publication.sql
>
> 10.
> RESET client_min_messages;
> \dRp+ testpub3
> \dRp+ testpub4
> +\dRp+ testpub5
> +\dRp+ testpub6
> +\dRp+ testpub7
>
>
> I feel it would be better to keep each \dRp+ together with the test it
> belongs to, rather than have a bunch of different tests which are then
> followed by a bunch of different \dRp+. Note: this same comment
> applies to other place of places -- not just here. Check everywhere
> you do \dRp+
>

I have addressed the remaining comments, did some cosmetic changes and
addressed the comment shared by Shveta in [2].
[1]: https://www.postgresql.org/message-id/CAA4eK1+rnjBOvkiQC2r4LuTwuje653iVPPAXcmJZXPpKvsNbOQ@mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAJpy0uCf5tXvqyVS3GQzU9J5HdSLAxX6Lxt1UKY4HJ8qnimCAw%40mail.gmail.com

Thanks,
Shlok Kyal