On Wed, Jun 5, 2024 at 3:48 PM Ashutosh Bapat <ashutosh.bapat....@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
