| 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 |
| 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 |