Re: Initial COPY of Logical Replication is too slow

From: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
To: "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>
Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, Jan Wieck <jan(at)wi3ck(dot)info>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Initial COPY of Logical Replication is too slow
Date: 2026-04-01 22:23:06
Message-ID: CAD21AoCNvrrkqZFRofPdyW7sd0CoRsnZZc4RusFbLrZax0ELzw@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Mar 31, 2026 at 12:40 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Tue, Mar 31, 2026 at 5:07 AM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > On Tuesday, March 31, 2026 5:36 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Mar 25, 2026 at 2:19 PM Peter Smith <smithpb2250(at)gmail(dot)com>
> > > 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.
> >
>
> Thanks. It looks like a good refactoring! I'd prefer to free the
> ancestors list to avoid memory leak.
>
> I've attached the patch that incorporated all comments I got so far.
> Feedback is very welcome.
>

I decided to simplify the code flow in the
is_table_publishable_in_publication() by taking more proposed changes
from Hou-san while accepting some memory leak. This function is
limited to be used only in per-call-memory context so we don't need to
worry about the actual memory leak. While we would need to change this
function in the futuer when we want to use it other places too, I
think it would be better to keep the function simple until then. I
hope the added new comment also help understand the code flow of this
function.

I think the patch is good shape, so planning to push it barring any objections.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Attachment Content-Type Size
v8-0001-Add-target_relid-parameter-to-pg_get_publication_.patch text/x-patch 31.0 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Sami Imseih 2026-04-01 22:29:11 Re: Re[2]: [PATCH] pg_stat_statements: add last_execution_start column
Previous Message Alvaro Herrera 2026-04-01 21:52:12 Re: Adding REPACK [concurrently]