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);
if (b1->isnulls[i][j] != b2->isnulls[i][j])
return false;
/* Must not pass NULL datums to datumIsEqual(). */
if (b1->isnulls[i][j])
continue;
}
/* < the long comment snipped >*/
if (!datumIsEqual(b1->datums[i][j], b2->datums[i][j],
parttypbyval[j], parttyplen[j]))
return false;
--
Amit Langote
EDB: http://www.enterprisedb.com