On Thu, 18 Dec 2025 at 11:30, shveta malik <[email protected]> wrote: > > On Wed, Dec 17, 2025 at 11:24 AM shveta malik <[email protected]> wrote: > > > > On Tue, Dec 16, 2025 at 2:50 PM Shlok Kyal <[email protected]> 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
v33-0001-Skip-publishing-the-tables-specified-in-EXCEPT-T.patch
Description: Binary data
