On Tue, Dec 23, 2025 at 12:03 PM Shlok Kyal <[email protected]> wrote:
>
>
> I have addressed the remaining comments, did some cosmetic changes and
> addressed the comment shared by Shveta in [2].
> [1]:
> https://www.postgresql.org/message-id/caa4ek1+rnjbovkiqc2r4lutwuje653ivppaxcmjzxppkvsn...@mail.gmail.com
> [2]:
> https://www.postgresql.org/message-id/CAJpy0uCf5tXvqyVS3GQzU9J5HdSLAxX6Lxt1UKY4HJ8qnimCAw%40mail.gmail.com
>
Thank You for the patch. Please find a few comments:
1)
GetTopMostAncestorInPublication():
+ if (list_member_oid(aexceptpubids, puboid))
+ {
+ list_free(aexceptpubids);
+ continue;
+ }
We need to do 'list_free(apubids)' as well here.
2)
GetTopMostAncestorInPublication(). Currently it has:
if (list_member_oid(aexceptpubids, puboid))
...
if (list_member_oid(apubids, puboid))
...
else
...schema mapping check
IMO more natural order of checks will be
if (list_member_oid(apubids, puboid))
..
else if (list_member_oid(aexceptpubids, puboid))
...
else
...schema mapping check
3)
+/*
+ * Return the list of relation OIDs excluded from a publication.
+ * This is only applicable for FOR ALL TABLES publications.
+ */
+List *
+GetPublicationExcludedRelations(Oid pubid, PublicationPartOpt pub_partopt)
a) Since now 'Relations' term means both tables and sequences, but
here we mean only Tables, we can rename it to have 'Tables' rather
than 'Relations'
b) Similar to GetAllPublicationRelations which is for 'ALL Tables'
pub, we can rename it to have 'All'
So the name can be 'GetAllPublicationExcludedTables' to be more clear.
Also we can move this function close to GetAllPublicationRelations as
it is more related to that.
4)
ObjectsInPublicationToOids()
+ case PUBLICATIONOBJ_EXCEPT_TABLE:
+ pubobj->pubtable->except = true;
+ *rels = lappend(*rels, pubobj->pubtable);
+ break;
Let me know when this will be hit when we already have
'ObjectsInAllPublicationToOids' in place?
5)
get_rel_sync_entry():
+ level++;
+ GetRelationPublications(ancestor, NULL, &aexceptpubids);
+
+ if (!list_member_oid(aexceptpubids, pub->oid))
+ {
+ pub_relid = ancestor;
+ ancestor_level = level;
+ }
+ }
Consider the following table structure:
t1 has a partition p1, which in turn has a child partition
child_part1. When publish_via_partition_root is set to true, any
changes made to child_part1 are replicated through t1. If we add t1 to
the EXCEPT list, get_rel_sync_entry() still marks p1 as an ancestor to
publish changes or child_part1. Is it correct?
6)
RelationBuildPublicationDesc() also needs some more analysis about
getting and setting ancestor part for above case.
7)
Currently the way we deal with the except table in pg_dump.c differs
from how we deal with included-table. To explain the same, how about
adding below comment in getPublications() just before we fetch
except-list:
We process EXCEPT TABLES here instead of in getPublicationTables(),
and output them directly in dumpPublication(). This differs from the
approach used in dumpPublicationTable() and
dumpPublicationNamespace(). Following that approach would require
dumping table additions later as ALTER PUBLICATION … ADD EXCEPT, which
is currently not supported.
thanks
Shveta
