From: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
---|---|
To: | Richard Guo <guofenglinux(at)gmail(dot)com> |
Cc: | David Christensen <david+pg(at)pgguru(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Reuse child_relids in try_partitionwise_join was Re: Assert failure on bms_equal(child_joinrel->relids, child_joinrelids) |
Date: | 2024-07-24 07:30:23 |
Message-ID: | CAExHW5sx9EembrsnZjJdCtoXzeHnBWhKHRLeAX-Wp4+6sPxC=w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Jul 23, 2024 at 2:05 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> On Wed, Jun 5, 2024 at 3:48 PM Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > This is different. But it needs a rebase. PFA rebased patch. The revised commit message explains the change better.
>
> I looked through this patch and I think it is in a good shape.
>
> Some minor comments:
>
> * I think it's better to declare 'child_relids' as type Relids
> rather than Bitmapset *. While they are the same thing, Bitmapset *
> isn't typically used in joinrels.c.
Agreed. Sorry. Fixed now.
> And I doubt that it is necessary
> to explicitly initialize it to NULL.
Done.
>
> * This patch changes the signature of function build_child_join_rel().
> I recommend arranging the two newly added parameters as 'int
> nappinfos, AppendRelInfo **appinfos' to maintain consistency with
> other functions that include these parameters.
Yes. Done.
>
> * At the end of the for loop in try_partitionwise_join(), we attempt
> to free the variables used within the loop. I suggest freeing these
> variables in the order or reverse order of their allocation, rather
> than arbitrarily.
Done. Are you suggesting it for aesthetic purposes or there's
something else (read, less defragmentation, performance gain etc.)? I
am curious.
> Also, in non-partitionwise-join planning, we do not
> free local variables in this manner. I understand that we do this for
> partitionwise-join because accumulating local variables can lead to
> significant memory usage, particularly with many partitions. I think
> it would be beneficial to add a comment in try_partitionwise_join()
> explaining this behavior.
Sounds useful. Done.
PFA patches:
0001 - same as previous one
0002 - addresses your review comments
--
Best Wishes,
Ashutosh Bapat
Attachment | Content-Type | Size |
---|---|---|
0001-Avoid-repeated-computation-in-try_partition-20240724.patch | text/x-patch | 5.5 KB |
0002-Address-Richard-s-comments-20240724.patch | text/x-patch | 3.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | John Naylor | 2024-07-24 07:42:59 | Re: Vacuum ERRORs out considering freezing dead tuples from before OldestXmin |
Previous Message | Tatsuo Ishii | 2024-07-24 06:55:25 | warning: dereferencing type-punned pointer |