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

On Wed, Jun 5, 2024 at 3:48 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.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.  And I doubt that it is necessary
to explicitly initialize it to NULL.

* 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.

* 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.  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.

Thanks
Richard