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?
>
> 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.
>
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;
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.
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?
thanks
Shveta
