Re: Multi-Column List Partitioning

On Thu, Dec 9, 2021 at 12:03 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Thu, Dec 9, 2021 at 3:12 PM Amul Sul <sulamul@gmail.com> wrote:
> > On Thu, Dec 9, 2021 at 11:24 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > >
> > [....]
> > > On Mon, Dec 6, 2021 at 10:57 PM Nitin Jadhav
> > > <nitinjadhavpostgres@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