Re: Skipping schema changes in publication

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>
Cc: shveta malik <shveta(dot)malik(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, YeXiu <1518981153(at)qq(dot)com>, Ian Lawrence Barwick <barwick(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Skipping schema changes in publication
Date: 2026-02-19 10:36:46
Message-ID: CAA4eK1JcrqPi7Pa9eQqj-=NFPAdzMhfVD=1jbwRWXT6tGtRyEw@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Feb 19, 2026 at 10:13 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> 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.

Attachment Content-Type Size
v46_amit_1.patch.txt text/plain 1.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Mingli Zhang 2026-02-19 10:37:05 Re: Recommended TPC-DS tools/setup for PostgreSQL benchmarking?
Previous Message Zsolt Parragi 2026-02-19 10:01:15 Re: Headerscheck support for meson