Re: Multi-Column List Partitioning

On Tue, Dec 21, 2021 at 6:34 PM Nitin Jadhav
<nitinjadhavpostgres(at)gmail(dot)com> wrote:
>
> ---
>
> > + 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?
>
> Now there are possibilities of multiple NULL values. We should have a
> mechanism to sort it when the bound values contain Non NULL and NULL
> values. As per the above logic we put the NULL values at the end.
> Please let me know if I am wrong.

Ok, but I am not sure about the comparison approach, let's see what
others think.

> ---
[...]
>
> > 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.
>
> This is required during merge_list_bounds(). AFAIK partition key
> information is not available here.
>

You can get that as an argument, see merge_range_bounds().

> > 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.
>
> I have split the patch into 2 patches. One is for the multi column
> list partitioning core changes and the other is for partition-wise
> join support. Each patch has its respective test cases in the
> regression suit and regression tests run successfully on each patch.
> Kindly let me know if any other changes are required here.
>

Thanks, for the slit that is much helpful, I have a few comments for
the 0001 patch as follow:

+ char **colname = (char **) palloc0(partnatts * sizeof(char *));

palloc0 is unnecessary.
---

+ foreach(cell2, rowexpr->args)
+ {
+ int idx = foreach_current_index(cell2);
+ Node *expr = lfirst(cell2);
+ Const *val =
+ transformPartitionBoundValue(pstate, expr, colname[i],
+ get_partition_col_typid(key, idx),
+ get_partition_col_typmod(key, idx),
+ get_partition_col_collation(key, idx));
+
+ values = lappend(values, val);
+ }

Array index for colname should be "idx".
---

result->scan_default = partition_bound_has_default(boundinfo);
+
return result;
...

/* Always include the default partition if any. */
result->scan_default = partition_bound_has_default(boundinfo);
-
return result;

...
else
result->scan_default = partition_bound_has_default(boundinfo);
+
return result;
...

- /* Add columns specified to SET NULL or SET DEFAULT if
provided. */
+ /*
+ * Add columns specified to SET NULL or SET DEFAULT if
+ * provided.
+ */

spurious change -- look like something not related to your patch.
--

- * For range partitioning, we must only perform pruning with values
- * for either all partition keys or a prefix thereof.
+ * For range partitioning and list partitioning, we must only perform
+ * pruning with values for either all partition keys or a prefix
+ * thereof.
*/
- if (keyno > nvalues && context->strategy == PARTITION_STRATEGY_RANGE)
+ if (keyno > nvalues && (context->strategy == PARTITION_STRATEGY_RANGE ||
+ context->strategy == PARTITION_STRATEGY_LIST))
break;

I think this is not true for multi-value list partitions, we might
still want prune partitions for e.g. (100, IS NULL, 20). Correct me
if I am missing something here.
---

/*
- * For range partitioning, if we have no clauses for the current key,
- * we can't consider any later keys either, so we can stop here.
+ * For range partitioning and list partitioning, if we have no clauses
+ * for the current key, we can't consider any later keys either, so we
+ * can stop here.
*/
- if (part_scheme->strategy == PARTITION_STRATEGY_RANGE &&
+ if ((part_scheme->strategy == PARTITION_STRATEGY_RANGE ||
+ part_scheme->strategy == PARTITION_STRATEGY_LIST) &&
clauselist == NIL)
break

Similarly, why would this be true for list partitioning? How can we
prune partitions if values is for e.g. (100, <not given>, 20).
--

- if (bms_is_member(keyno, opstep->nullkeys))
+ if (bms_is_member(keyno, opstep->nullkeys) &&
+ context->strategy != PARTITION_STRATEGY_LIST)
continue;
Will that prune for all NULL partitioning key values?
---

+ appendStringInfoString
+ (buf,
get_list_partbound_value_string(lfirst(cell)));

Formatting is not quite right.
--

+/*
+ * get_min_and_max_offset
+ *
+ * Fetches the minimum and maximum offset of the matching partitions.
+ */

...

+/*
+ * get_min_or_max_off
+ *
+ * Fetches either minimum or maximum offset of the matching partitions
+ * depending on the value of is_min parameter.
+ */

I am not sure we really have to have separate functions but if needed
then I would prefer to have a separate function for each min and max
rather than combining.
---

+ if (part_scheme->strategy != PARTITION_STRATEGY_LIST)
+ {
+ *clause_is_not_null = (nulltest->nulltesttype == IS_NOT_NULL);
+ return PARTCLAUSE_MATCH_NULLNESS;
+ }
+
+ expr = makeConst(UNKNOWNOID, -1, InvalidOid, -2, (Datum) 0,
true, false);
+ partclause = (PartClauseInfo *) palloc(sizeof(PartClauseInfo));
+
+ partclause->keyno = partkeyidx;
+ partclause->expr = (Expr *) expr;
+ partclause->is_null = true;
+
+ if (nulltest->nulltesttype == IS_NOT_NULL)
+ {
+ partclause->op_is_ne = true;
+ partclause->op_strategy = InvalidStrategy;
+ }
+ else
+ {
+ partclause->op_is_ne = false;
+ partclause->op_strategy = BTEqualStrategyNumber;
+ }

- return PARTCLAUSE_MATCH_NULLNESS;
+ *pc = partclause;
+ return PARTCLAUSE_MATCH_CLAUSE;

I still believe considering NULL value for match clause is not a
fundamentally correct thing. And that is only for List partitioning
which isn't aligned with the other partitioning.
---

Regards,
Amul