public inbox for [email protected]
help / color / mirror / Atom feedFrom: Zhijie Hou (Fujitsu) <[email protected]>
To: Amit Kapila <[email protected]>
To: Peter Smith <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Jan Wieck <[email protected]>
Cc: [email protected] <[email protected]>
Subject: RE: Initial COPY of Logical Replication is too slow
Date: Tue, 31 Mar 2026 12:07:42 +0000
Message-ID: <TY4PR01MB169070A34D1C74867EF5A2DE49453A@TY4PR01MB16907.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAA4eK1K+rumWz=mHDLVVCig-i_cWWSzwDE1eMySq0WYc7_ve+Q@mail.gmail.com>
References: <CAB-JLwbBFNuASyEnZWP0Tck9uNkthBZqi6WoXNevUT6+mV8XmA@mail.gmail.com>
<CAD21AoA6i2ui8FMZeuU_KxX4t-fM8G==zTW2Dp6-goujttrpew@mail.gmail.com>
<CAB-JLwZpp=7c9_r0beWWJxRh2BS_2Vvth8UDv7H57DBeaqggVg@mail.gmail.com>
<CAD21AoDT3sL2COprsRumM9zEpL1Bk5VWboK4V2mRnjGua8xfeA@mail.gmail.com>
<CAD21AoDQM62GOtaTzD_CVMSsFhv6o9c0Au1dSM1QuxeKFkWAKw@mail.gmail.com>
<CAD21AoCz7HjEr3oeb=haK31YHxHZLcvD_wx_a-+xLPKywq++3A@mail.gmail.com>
<TY4PR01MB16907733B75A99117F013AFCA947FA@TY4PR01MB16907.jpnprd01.prod.outlook.com>
<CAD21AoA9YgiY1rVKMPZwB00WU_G4UfzoawY=7hyd7hpvBPcK6w@mail.gmail.com>
<CAA4eK1KoSi60dtakJzn0MxNnHF1Yf4indSAffTjJxQG_31jsgQ@mail.gmail.com>
<CAD21AoB4B3MOxJ7-v9YLjV5fTOtaLRUhX3jN3kqhEi7D7-uY4A@mail.gmail.com>
<[email protected]>
<CAD21AoCmHpKrNg9D3mcOA973CZ5N_dBLxb8pERpSxEeRLSQxpA@mail.gmail.com>
<CAD21AoAEVyxwn_bMWHvcU-Gcz3aUVjAtMbdgfoJ8MZNiLLEh0g@mail.gmail.com>
<CAA4eK1Jkouj=w+PHzMB6v890ES3QOLf=cUTvZmGFr-WMQW2OnA@mail.gmail.com>
<CAD21AoB4_n7+s=uM9apX1JVtvGvgM8ismAx_uMxvDmUXfQULsw@mail.gmail.com>
<CAD21AoBJcxRcaWQot302diaxoDcsnezRhnZa7p8UrPh5AGNeHQ@mail.gmail.com>
<CAHut+PuSkabUB8H_hcwQz=BX5TWEj-8Ba+CP_PX78zN1fkhtKA@mail.gmail.com>
<CAA4eK1K+rumWz=mHDLVVCig-i_cWWSzwDE1eMySq0WYc7_ve+Q@mail.gmail.com>
On Tuesday, March 31, 2026 5:36 PM Amit Kapila <[email protected]> wrote:
>
> On Wed, Mar 25, 2026 at 2:19 PM Peter Smith <[email protected]>
> wrote:
> >
> > There are many return points, and most of those "if" blocks cannot
> > fall through (they return).
> >
> > I found it slightly difficult to read the code because I kept having
> > to think, "OK, if we reached here, it means pubviaroot must be false,"
> > or "OK, if we reached this far, then puballtables must be false, and
> > pubviaroot must be false," etc.
> >
>
> I can't say exactly why, but I find it difficult to read this function. So, I share
> your concerns about the code of this function.
> Because of its complexity it is difficult to ascertain that the functionality is
> correct or we missed something. Also, considering it is correct today, in its
> current form, it may become difficult to enhance it in future.
>
I attempted to refactor the code a bit based on my preferred style, as shown in
the attachment. While the number of return points couldn't be reduced, I tried
to eliminate if-else branches where possible. Sharing this top-up patch as a
reference for an alternative style that reduces code size.
Best Regards,
Hou zj
Attachments:
[application/octet-stream] v1-0001-refactor-the-function.patch (3.5K, 2-v1-0001-refactor-the-function.patch)
download | inline diff:
From 706d7cb4b3ac7f30f131c64f859cabe00773b07d Mon Sep 17 00:00:00 2001
From: Zhijie Hou <[email protected]>
Date: Tue, 31 Mar 2026 19:35:44 +0800
Subject: [PATCH v1] refactor the function
---
src/backend/catalog/pg_publication.c | 79 +++++++++-------------------
1 file changed, 26 insertions(+), 53 deletions(-)
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 14ae03fc0ff..a036cd8ff33 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -1312,75 +1312,48 @@ static bool
is_table_publishable_in_publication(Oid relid, Publication *pub)
{
bool relispartition;
+ List *ancestors = NIL;
+ Oid topmost = InvalidOid;
/*
* For non-pubviaroot publications, a partitioned table is never the
* effective published OID; only its leaf partitions can be.
*/
- if (!pub->pubviaroot && get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE)
+ if (!pub->pubviaroot &&
+ get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE)
return false;
relispartition = get_rel_relispartition(relid);
- if (pub->alltables)
- {
- Oid target_relid = relid;
-
- if (pub->pubviaroot)
- {
- /*
- * ALL TABLES with pubviaroot includes only regular tables or
- * top-most partitioned tables -- never child partitions.
- */
- if (relispartition)
- return false;
- }
- else if (relispartition)
- {
- List *ancestors = get_partition_ancestors(relid);
+ /*
+ * ALL TABLES with pubviaroot includes only regular tables or
+ * top-most partitioned tables -- never child partitions.
+ */
+ if (pub->alltables && pub->pubviaroot && relispartition)
+ return false;
- /*
- * Only the top-most ancestor can appear in the EXCEPT clause.
- * Therefore, for a partition, exclusion must be evaluated at the
- * top-most ancestor.
- */
- target_relid = llast_oid(ancestors);
- list_free(ancestors);
- }
+ if (relispartition)
+ ancestors = get_partition_ancestors(relid);
- /*
- * The table is published unless it appears in the EXCEPT clause. ALL
- * TABLES publications store only EXCEPT'ed tables in
- * pg_publication_rel, so checking existence is sufficient.
- */
+ /*
+ * The table is published unless it appears in the EXCEPT clause. ALL
+ * TABLES publications store only EXCEPT'ed tables in
+ * pg_publication_rel, so checking existence is sufficient.
+ */
+ if (pub->alltables)
return !SearchSysCacheExists2(PUBLICATIONRELMAP,
- ObjectIdGetDatum(target_relid),
+ ObjectIdGetDatum(ancestors
+ ? llast_oid(ancestors) : relid),
ObjectIdGetDatum(pub->oid));
- }
/*
- * Non-alltables
+ * If pubviaroot is true, the ancestor is published instead of the
+ * partition, so exclude it. Otherwise, the ancestor covers the partition,
+ * so include it.
*/
-
- if (relispartition)
- {
- List *ancestors = get_partition_ancestors(relid);
- Oid topmost = GetTopMostAncestorInPublication(pub->oid, ancestors, NULL);
-
- list_free(ancestors);
-
- if (OidIsValid(topmost))
- {
- /*
- * If pubviaroot is true, the ancestor is published instead of the
- * partition, so exclude it. Otherwise, the ancestor covers the
- * partition, so include it.
- */
- return !pub->pubviaroot;
- }
-
- /* Ancestor not published; fall through to check the partition itself */
- }
+ if (relispartition &&
+ OidIsValid(GetTopMostAncestorInPublication(pub->oid, ancestors, NULL)))
+ return !pub->pubviaroot;
/*
* Check whether the table is explicitly published via pg_publication_rel
--
2.53.0.windows.2
view thread (51+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: RE: Initial COPY of Logical Replication is too slow
In-Reply-To: <TY4PR01MB169070A34D1C74867EF5A2DE49453A@TY4PR01MB16907.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox