Re: Skipping schema changes in publication

On Tue, Dec 16, 2025 at 2:50 PM Shlok Kyal <shlok.kyal.oss@gmail.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