Re: Multi-Column List Partitioning

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

In addition to Amit's suggestions, here are a few more:

+ char **colname = (char **) palloc0(partnatts * sizeof(char *));
+ Oid *coltype = palloc0(partnatts * sizeof(Oid));
+ int32 *coltypmod = palloc0(partnatts * sizeof(int));
+ Oid *partcollation = palloc0(partnatts * sizeof(Oid));
+

None of them really needed to be palloc0; also, as described
previously you can avoid the last three by using get_partition_col_*
directly.
---

+ i = 0;
+ foreach(cell2, rowexpr->args)
+ {

It's up to you, rather than using a separate index variable and
incrementing that at the end, I think we can use
foreach_current_index(cell2) which would look much nicer.
---

+ all_values[j].values = (Datum *) palloc0(key->partnatts *
sizeof(Datum));
+ all_values[j].isnulls = (bool *) palloc0(key->partnatts *
sizeof(bool));
+ all_values[j].index = i;

palloc0 is unnecessary for the "values".
---

dest->datums[i] = &boundDatums[i * natts];
+ if (src->isnulls)
+ dest->isnulls[i] = (bool *) palloc(sizeof(bool) * natts);

I think you can allocate memory for isnulls the same way you do
allocate boundDatums and just do the memcpy.
---

+ for (i = 0; i < partnatts; i++)
+ {
+ if (outer_isnull && outer_isnull[i])
+ {
+ outer_has_null = true;
+ if (outer_map.merged_indexes[outer_index] == -1)
+ consider_outer_null = true;
+ }

I am wondering why you are not breaking the loop once you set
consider_outer_null?
Note that if you do that then you need a separate loop for the
inner_isnull part.
---

@@ -1351,14 +1431,30 @@ merge_list_bounds(FmgrInfo *partsupfunc, Oid
*partcollation,
/* A list value missing from the inner side. */
Assert(outer_pos < outer_bi->ndatums);

- /*
- * If the inner side has the default partition, or this is an
- * outer join, try to assign a merged partition to the outer
- * partition (see process_outer_partition()). Otherwise, the
- * outer partition will not contribute to the result.
- */
- if (inner_has_default || IS_OUTER_JOIN(jointype))
+ if (outer_has_null || inner_has_null)
{
+ if (consider_outer_null || consider_inner_null)
+ {
+ /* Merge the NULL partitions. */
+ merged_index = merge_null_partitions(&outer_map, &inner_map,
+ consider_outer_null,
+ consider_inner_null,
+ outer_index,
inner_index,
+ jointype, &next_index);
+

I have doubts about the condition that allows reaching
merge_null_partitions() but I am not sure I am correct. I think if the
list values missing from the __inner side__ then we might need to
check only "inner_has_null" & "consider_inner_null" and merge the
same, but why is this code also checking "outer_has_null" &
"consider_outer_null". Correct me if I am missing something.
---

+ if (isnulls && isnulls[i])
+ cmpval = 0; /* NULL "=" NULL */
+ else
+ cmpval = 1; /* NULL ">" not-NULL */
+ }
+ else if (isnulls && isnulls[i])
+ cmpval = -1; /* not-NULL "<" NULL */

I really doubt this assumption is correct; aren't those strict operators?
---

+get_list_partbound_value_string(List *bound_value)
+{
+ StringInfo buf = makeStringInfo();
+ StringInfo boundconstraint = makeStringInfo();

boundconstraint should be declared inside "if (ncols > 1)" block.
---

+ foreach(cell, bound_value)
+ {
+ Const *val = castNode(Const, lfirst(cell));
+
+ appendStringInfoString(buf, sep);
+ get_const_expr(val, &context, -1);
+ sep = ", ";
+ ncols++;
+ }

I think no need to increment ncols every time, you have a list and you
can get that. Also, I think since you have ncols already, you can
prepend and append parenthesis before and after so that you can avoid
extra StringInfo.
---

typedef struct PartitionBoundInfoData
{
char strategy; /* hash, list or range? */
+ int partnatts; /* number of partition key columns */
int ndatums; /* Length of the datums[] array */
Datum **datums;
+ bool **isnulls;

Adding "partnatts" to this struct seems to be unnecessary, AFAIUC,
added that for partition_bound_accepts_nulls(), but we can easily get
that value from the partitioning key & pass an additional argument.
Also, no information about the length of the "isnulls" array.
---

I think it would be helpful if you could split the patch: one for
multi-value list partitioning and another for the partition wise join, thanks.

Regards,
Amul