On Thu, Dec 9, 2021 at 12:03 PM Amit Langote <amitlangot...@gmail.com> wrote:
>
> On Thu, Dec 9, 2021 at 3:12 PM Amul Sul <sula...@gmail.com> wrote:
> > On Thu, Dec 9, 2021 at 11:24 AM Amit Langote <amitlangot...@gmail.com>
> > wrote:
> > >
> > [....]
> > > On Mon, Dec 6, 2021 at 10:57 PM Nitin Jadhav
> > > <nitinjadhavpostg...@gmail.com> wrote:
> > > > > Looks difficult to understand at first glance, how about the
> > > > > following:
> > > > >
> > > > > if (b1->isnulls != b2->isnulls)
> > > > > return false;
> > >
> > > I don't think having this block is correct, because this says that two
> > > PartitionBoundInfos can't be "logically" equal unless their isnulls
> > > pointers are the same, which is not the case unless they are
> > > physically the same PartitionBoundInfo. What this means for its only
> > > caller compute_partition_bounds() is that it now always needs to
> > > perform partition_bounds_merge() for a pair of list-partitioned
> > > relations, even if they have exactly the same bounds.
> > >
> > > So, I'd suggest removing the block.
> > >
> >
> > Agreed, I too realized the same; the check is incorrect and have noted
> > it for the next post. But note that, we need a kind of check here otherwise,
> > how could two bounds be equal if one has nulls and the other doesn't.
>
> We check partition strategy at the top and that ensures that isnulls
> fields should either be both NULL or not, same as the block above that
> checks 'kind'. Maybe adding an Assert inside the block makes sense,
> like this:
>
> /*
> * If the bound datums can be NULL, check that the datums on
> * both sides are either both NULL or not NULL.
> */
> if (b1->isnulls != NULL)
> {
> /*
> * Both bound collections have the same partition
> strategy,
> * so the other side must allow NULL datums as well.
> */
> Assert(b2->isnulls != NULL);
>
Make sense, thanks!
Regards,
Amul
