On Thu, Feb 19, 2026 at 10:13 AM Shlok Kyal <[email protected]> wrote:
>
> Thanks for reviewing the patch. I have addressed the remaining
> comments in the v46 patch..
>
Can we think of some ideas to split this patch? One possibility is
that in the first patch we give an ERROR, if a non-root partitioned
table is present in EXCEPT Clause. I see that a lot of code is written
to handle partitions in EXCEPT clause. I feel such a split will make
code easier to review and validate.
Few other comments:
=================
1.
if (stmt->for_all_tables)
{
+ /* Process EXCEPT table list */
+ if (relations != NIL)
+ {
+ Assert(rels != NIL);
+ PublicationAddTables(puboid, rels, true, NULL);
+ }
+
/*
* Invalidate relcache so that publication info is rebuilt. Sequences
* publication doesn't require invalidation, as replica identity
CacheInvalidateRelcacheAll();
Here, the relations listed in the except table list will be
invalidated twice, once inside
PublicationAddTables->publication_add_relation, and second time via
CacheInvalidateRelcacheAll. Can we avoid that by adding a parameter to
PublicationAddTables?
2.
- root_relids = GetPublicationRelations(pubform->oid,
- PUBLICATION_PART_ROOT);
+ root_relids = GetIncludedRelationsByPub(pubform->oid,
+ PUBLICATION_PART_ROOT);
To follow the previous function naming pattern, can we rename
GetIncludedRelationsByPub to GetIncludedPublicationRelations? If we
agree to this then rename the excluded version of the function as
well.
3.
+/*
+ * Return the list of relation Oids for a publication.
+ *
+ * For a FOR TABLE publication, this returns the list of relations explicitly
+ * included in the publication.
+ *
+ * Publications declared with FOR ALL TABLES/SEQUENCES should use
+ * GetAllPublicationRelations() to obtain the complete set of tables/sequences
+ * covered by the publication.
+ */
+List *
+GetIncludedRelationsByPub(Oid pubid, PublicationPartOpt pub_partopt)
This is equivalent to the existing function GetPublicationRelations()
which has more precise comments atop. We can use the same comments
unless there is any functionality difference.
4.
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -29,6 +29,7 @@
#include "catalog/pg_publication.h"
#include "catalog/pg_publication_namespace.h"
#include "catalog/pg_publication_rel.h"
+#include "catalog/pg_subscription.h"
It appears odd to include pg_subscription.h in the publication code.
Is there a reason for the same? If not then avoid it.
Apart from above, a few cosmetic changes are attached.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/catalog/pg_publication.c
b/src/backend/catalog/pg_publication.c
index f71ad1e49e5..ae179a9c647 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -757,9 +757,10 @@ publication_add_schema(Oid pubid, Oid schemaid, bool
if_not_exists)
/*
* Get the list of publication oids associated with a specified relation.
*
- * Parameter 'pubids' returns the Oids of the publications the relation is part
- * of. Parameter 'except_pubids' returns the Oids of publications the relation
- * is excluded from.
+ * 'pubids' returns the Oids of the publications the relation is part of.
+ *
+ * 'except_pubids' returns the Oids of publications the relation is excluded
+ * from.
*
* This function returns true if the relation is part of any publication.
*/
@@ -866,6 +867,7 @@ GetIncludedRelationsByPub(Oid pubid, PublicationPartOpt
pub_partopt)
/*
* Return the list of tables Oids excluded from a publication.
+ *
* This is only applicable for FOR ALL TABLES publications.
*/
List *
