Re: Skipping schema changes in publication

On Tue, Dec 16, 2025 at 2:50 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> I have also addressed the remaining comments and attached the latest patch.
>

Thanks. A few comments:

1)
+ if (!set_top && puballtables)
+ set_top = !list_member_oid(aexceptpubids, puboid);

In GetTopMostAncestorInPublication(), we have made the above change
which will now get ancestor from all-tables publication as well,
provided table is not part of 'except' List. Earlier this function was
only checking pg_subscription_rel and pg_publication_namespace which
does not include all-tables publication. Won't it change the
result-set for callers?

2)
+ * 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)
+{
+ return GetPublicationRelationsInternal(pubid, pub_partopt, false);
+}

We can have an Assert here that pubid passed is not for ALL-Tables or
ALL-sequences

3)
GetAllPublicationRelations:
* If the publication publishes partition changes via their respective root
* partitioned tables, we must exclude partitions in favor of including the
* root partitioned tables. This is not applicable to FOR ALL SEQUENCES
* publication.

+ * The list does not include relations that are explicitly excluded via the
+ * EXCEPT TABLE clause of the publication specified by pubid.

Suggestion:
/*
* If the publication publishes partition changes via their respective root
* partitioned tables, we must exclude partitions in favor of including the
* 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.
*/

4)
GetAllPublicationRelations:
+ if (relkind == RELKIND_RELATION)
+ exceptlist = GetPublicationExcludedRelations(pubid, pubviaroot ?
+ PUBLICATION_PART_ALL :
+ PUBLICATION_PART_ROOT);

Assert(!(relkind == RELKIND_SEQUENCE && pubviaroot));

Generally we keep such parameters' sanity checks as the first step. We
can add new code after Assert.

5)
ObjectsInAllPublicationToOids() only has one caller which calls it
under check: 'if (stmt->for_all_tables)'

Thus IMO, we do not need a switch-case in
ObjectsInAllPublicationToOids(). We can simply have a sanity check to
see it is 'PUBLICATION_ALL_TABLES' and then do the needed operation
for this pub-type.

6)
CreatePublication():
/*
* If publication is for ALL TABLES and relations is not empty, it means
* that there are some relations to be excluded from the publication.
* Else, relations is the list of relations to be added to the
* publication.
*/

Shall we rephrase slightly to:

/*
* 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.
*/

7)
+ /* Associate objects with the publication. */
+ if (stmt->for_all_tables)
+ {
+ /* Invalidate relcache so that publication info is rebuilt. */
+ CacheInvalidateRelcacheAll();
+ }

I think this comment is misplaced. We shall have it at previous place, atop:
if (stmt->for_all_tables)
This is because here we are just trying to invalidate cache while at
previous place we are trying to associate.

thanks
Shveta