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