Thread

Re: Skipping schema changes in publication

Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-12-18T11:45:06Z

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?

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

> > 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?
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;

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

> 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,
Shlok Kyal