On Fri, Jul 10, 2026 at 5:07 PM Nisha Moond <[email protected]> wrote:
>
>
> Yes, that works for me as we have FOR TABLES example to follow. I've
> made the changes in v20.
>
Thanks! I had a quick look, few initial comments:
1)
GetTopMostAncestorInPublication():
+ if (ancestors == NIL)
+ return InvalidOid;
Should this be Assert or can there be a case where ancestors list can
come as NIL?
2)
GetAllSchemaPublicationRelations
+ if (!excluded && get_rel_relispartition(relid))
+ {
+ List *ancestors = get_partition_ancestors(relid);
+ ListCell *alc;
+
+ foreach(alc, ancestors)
+ {
+ if (list_member_oid(except_relids, lfirst_oid(alc)))
+ {
+ excluded = true;
+ break;
+ }
+ }
+ list_free(ancestors);
+ }
Should we check root only in except_relids instead of complete ancestor chain?
3)
postgres=# ALTER TABLE s2.tab_root_new ATTACH PARTITION s2.tab_root
FOR VALUES FROM (1) TO (2000);
ERROR: 55000: cannot attach table "tab_root" as partition because it
is referenced in publication "pub1" EXCEPT clause
DETAIL: The publication EXCEPT clause cannot contain tables that are
partitions.
HINT: Change the publication's EXCEPT clause using ALTER PUBLICATION
... SET ALL TABLES.
LOCATION: ATExecAttachPartition, tablecmds.c:21107
pub1 is a schmea publication table here, I think HINT needs some adjustment.
4)
+ root_tup = SearchSysCache2(PUBLICATIONRELMAP,
+ ObjectIdGetDatum(llast_oid(ancestors)),
+ ObjectIdGetDatum(pub->oid));
+ if (HeapTupleIsValid(root_tup))
+ {
+ bool root_except = ((Form_pg_publication_rel) GETSTRUCT(root_tup))->prexcept;
+
+ ReleaseSysCache(root_tup);
+ if (root_except)
+ return false;
+ }
Above code snipet appears a lot many times either for relid or
llast_oid(ancestors), can we replace this with a common function like:
bool lookup_pub_relmap(Oid pubid, Oid relid, bool *is_except); (or
get_publication_rel_entry or anything which you find better)
The return value will indicate when the relid is present. 'is_except'
will indicate whetehr it is marked with except flag or not. Then above
can be replaced with:
if (lookup_pub_relmap(pub->oid, llast_oid(ancestors), &root_except)
&& root_except)
return false;
Do this only if you feel it will simplify the code and apply it where it helps.
thanks
Shveta
