Re: Multi-Column List Partitioning

On Fri, Oct 22, 2021 at 3:50 AM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:

>
>
> On Fri, Oct 22, 2021 at 2:48 AM Nitin Jadhav <
> nitinjadhavpostgres(at)gmail(dot)com> wrote:
>
>> > While testing further I got a crash with partition wise join enabled
>> for multi-col list partitions. please find test case & stack-trace below.
>>
>> Thanks for sharing. I have fixed the issue in the attached patch.
>>
>> Thanks & Regards,
>> Nitin Jadhav
>>
>>
>>>>>>>
>>>>>>> Hi,
>
> +isListBoundDuplicated(List *list_bounds, List *new_bound)
>
> + Const *value1 = castNode(Const, list_nth(elem, i));
> + Const *value2 = castNode(Const, list_nth(new_bound, i));
>
> Should the upper bound for index i take into account the length of
> new_bound ?
> If the length of new_bound is always the same as that for elem, please add
> an assertion.
>
> For transformPartitionListBounds():
> + deparse_expression((Node *) list_nth(partexprs, j),
> +
> deparse_context_for(RelationGetRelationName(parent),
> +
> RelationGetRelid(parent)),
>
> Please consider calling RelationGetRelationName(parent)
> and RelationGetRelid(parent) (and assigning to local variables) outside the
> loop.
>
> +get_list_datum_count(PartitionBoundSpec **boundspecs, int nparts)
>
> get_list_datum_count -> get_list_datums_count
>
> For partition_bounds_equal():
>
> + if (b1->isnulls)
> + b1_isnull = b1->isnulls[i][j];
> + if (b2->isnulls)
> + b2_isnull = b2->isnulls[i][j];
>
> Should the initialization of b1_isnull and b2_isnull be done inside the
> loop (so that they don't inherit value from previous iteration) ?
>
> Cheers
>

Hi,
Continuing review.

+ * For the multi-column case, we must make an BoolExpr that

an BoolExpr -> a BoolExpr

In get_qual_for_list(), it would be better if repetitive code can be
extracted into a helper method:

+ if (val->constisnull)
+ {
+ NullTest *nulltest = makeNode(NullTest);
+
+ key_is_null[j] = true;
+
+ nulltest->arg = keyCol[j];
+ nulltest->nulltesttype = IS_NULL;
+ nulltest->argisrow = false;
+ nulltest->location = -1;
+
+ if (key->partnatts > 1)
+ and_args = lappend(and_args, nulltest);
+ else
+ is_null_test = (Expr *) nulltest;
+ }
+ else
+ {
+ if (key->partnatts > 1)
+ {
+ Expr *opexpr =
+ make_partition_op_expr(key, j,
+ BTEqualStrategyNumber,
+ keyCol[j],
+ (Expr *) val);
+ and_args = lappend(and_args, opexpr);
+ }
+ else
+ datum_elem = (Expr *) val;
+ }

For match_clause_to_partition_key():

+ if (part_scheme->strategy != PARTITION_STRATEGY_LIST)
+ {
+ *clause_is_not_null = (nulltest->nulltesttype == IS_NOT_NULL);
+ return PARTCLAUSE_MATCH_NULLNESS;
+ }
+ else

Since the if block ends with return, the 'else' is not needed - else block
can be indented to the left.

get_min_and_max_off(): I think get_min_and_max_offset as method name would
be more informative.

+ Assert(0 == partition_lbound_datum_cmp(partsupfunc, partcollation,
+ boundinfo->datums[off],
+ boundinfo->isnulls[off],
+ values, isnulls, nvalues));

If the 'while (off >= 1)' loop exits without modifying off, is the above
assertion always true (can boundinfo->datums[off] be accessed without
checking bound) ?

Cheers