Hi,
On Thu, Apr 9, 2020 at 12:06 AM Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:
> Both of your patches fix the problem. I don't have much exposure in
> this area to comment on whether we should keep/remove the assertion
> from the code. But, here is my opinion:
>
> The code structure looks like following:
> Assert(condition A);
> if (Condition B)
> merge_*_bounds(....);
>
> Inside merge_*_bounds(), you have both the above assert and the if
> condition as another assert:
> Assert(condition A and Condition B);
>
> And, merge_*_bounds() are called from only one place. So, something is
> redundant here and I'm inclined towards removal of the assert
> condition. Another thing I noticed:
>
> /* The partitioning strategies should be the same. */
> Assert(outer_binfo->strategy == inner_binfo->strategy);
>
> The comment just reads the assertion aloud which looks unnecessary.
>
Yeah, partition_bounds_merge() is currently called only from
try_partitionwise_join(), which guarantees that the strategies are the
same. The assertion cost would be cheap, but not zero, so I still
think it would be better to remove the assertion from
partition_bounds_merge().
Best regards,
Etsuro Fujita