Hi, On Thu, Apr 9, 2020 at 12:06 AM Kuntal Ghosh <kuntalghosh.2...@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