Re: Reuse child_relids in try_partitionwise_join was Re: Assert failure on bms_equal(child_joinrel->relids, child_joinrelids)

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