Re: Skipping schema changes in publication - Mailing list pgsql-hackers

On Thu, Dec 18, 2025 at 5:15 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Thu, 18 Dec 2025 at 11:30, shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Wed, Dec 17, 2025 at 11:24 AM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > 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?
> > >
> It can change the result set of callers. I analysed more and saw that
> GetTopMostAncestorInPublication is called from 3 places.
> 1. pub_rf_contains_invalid_column: it is called when publication is
> not ALL TABLES. It will have no impact with the change.
> 2. pub_contains_invalid_column : it is called for all type of
> publication. it calls GetTopMostAncestorInPublication like:
> ```
>     if (pubviaroot && relation->rd_rel->relispartition)
>   {
>     publish_as_relid = GetTopMostAncestorInPublication(pubid, ancestors,
>                                NULL, puballtables);
>
>     if (!OidIsValid(publish_as_relid))
>       publish_as_relid = relid;
>   }
> ```
> In HEAD for ALL TABLES publication GetTopMostAncestorInPublication
> will always return InvalidOid. With this patch it can have some value.
> So there is a difference in behaviour.
>
> 3. get_rel_sync_entry
> in HEAD we had
> ```
> if (pub->alltables)
>       {
>         publish = true;
>         if (pub->pubviaroot && am_partition)
>         {
>           List     *ancestors = get_partition_ancestors(relid);
>
>           pub_relid = llast_oid(ancestors);
>           ancestor_level = list_length(ancestors);
>         }
>       }
> ```
> With patch this condition is not valid because we cannot set
> 'pub_relid = llast_oid(ancestors);' directly as the table can be
> excluded.
> So, the change in GetTopMostAncestorInPublication will even handle the
> case of "ALL TABLES" publication.
>
> Since we have a behaviour difference for the 2nd function, I have
> removed the changes for 'ALL TABLES' from
> GetTopMostAncestorInPublication and added it separately
> 'get_rel_sync_entry'. Thoughts?

I find the current implementation better, the previous one was
impacting the results of different paths.

Regarding:
+ if (list_member_oid(aexceptpubids, puboid))
+ {
+ list_free(aexceptpubids);
+ continue;
+ }

IMO, if puboid is part of apubids, that check is enough. This is
because aexceptpubids and apubids are mutually exclusive lists for a
particular 'ancestor'. But if we want to have it to avoid
schma-mapping check later, we should add a comment. How about:

This step isn’t strictly necessary, but we keep it so we can skip the
table if it appears in the EXCEPT list, avoiding an expensive
schema-mapping check later.

>
> > > 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
> > >
> Added assert for all tables. I found during testing that this function
> can be called for ALL SEQUENCES in HEAD. So I have not added an
> assertion for it.
> I think it is a bug and shared the same in [1]. Will add assert for
> all sequences as well once the bug is fixed.
>

Okay.

> > > 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.
> > >
> >
> > Few more:
> >
> > 8)
> > get_rel_sync_entry()
> > + List    *exceptTablePubids = NIL;
> >
> > At all other places, we are using exceptpubids, shall we use the same here?
> >
> > 9)
> > ObjectsInPublicationToOids()
> >
> >   case PUBLICATIONOBJ_TABLE:
> > + case PUBLICATIONOBJ_EXCEPT_TABLE:
> > + pubobj->pubtable->except = (pubobj->pubobjtype ==
> > PUBLICATIONOBJ_EXCEPT_TABLE);
> >   *rels = lappend(*rels, pubobj->pubtable);
> >   break;
> >
> > It looks slightly odd that for pubobjtype case
> > 'PUBLICATIONOBJ_EXCEPT_TABLE', we have to check pubobjtype against
> > PUBLICATIONOBJ_EXCEPT_TABLE itself.
> >
> > Shall we make it:
> > case PUBLICATIONOBJ_EXCEPT_TABLE:
> >     pubobj->pubtable->except = true;
> >     /* fall through */
> > case PUBLICATIONOBJ_TABLE:
> >     *rels = lappend(*rels, pubobj->pubtable);
> >     break;
> >
> We should also make pubobj->pubtable->except = false for PUBLICATIONOBJ_TABLE?

yes, right.

> Updated the condition like:
>       case PUBLICATIONOBJ_EXCEPT_TABLE:
>         pubobj->pubtable->except = true;
>         *rels = lappend(*rels, pubobj->pubtable);
>         break;
>       case PUBLICATIONOBJ_TABLE:
>         pubobj->pubtable->except = false;
>         *rels = lappend(*rels, pubobj->pubtable);
>         break;
>

Looks good.

> > 10)
> > I want to understand the usage of DO_PUBLICATION_EXCEPT_REL. Can you
> > give a scenario where its usage in DOTypeNameCompare() will be hit?
> > Its all other usages too need some analysis and validation.
> >
> In the current patch we are not setting an objecttype to
> DO_PUBLICATION_EXCEPT_REL.
> We are storing the list of except tables in 'pubinfo[i].excepttbls'
> list in function getPublications and "pubinfo[i].dobj.objType =
> DO_PUBLICATION". So, I don't see any requirement of
> DO_PUBLICATION_EXCEPT_REL now. I have removed it.
>

Yes, that was my initial thought as well, that we might not need it.
But I’ll review it further and let you know.

> > 11)
> > + List    *except_objects; /* List of publication object to be excluded */
> >
> > object --> objects
> > Currently since we exclude only tables, does it make sense to name it
> > as except_tables?
> >
> I have also addressed the remaining comments and attached the updated v33 patch.
> [1]:
https://www.postgresql.org/message-id/CALDaNm0qoNtsX%2B9KPug6qb%3DuC-H2iPMYW%2BgL%3DHehx%2BNgOxga6w%40mail.gmail.com
>

Thanks, will review.

thanks
Shveta