Re: Multi-Column List Partitioning - Mailing list pgsql-hackers
| From | Zhihong Yu |
|---|---|
| Subject | Re: Multi-Column List Partitioning |
| Date | |
| Msg-id | CALNJ-vRuMcCH7xPS4wnSLSxjWViibEH4JtfgQP0Denf24-QnEw@mail.gmail.com Whole thread Raw |
| In response to | Re: Multi-Column List Partitioning (Zhihong Yu <zyu@yugabyte.com>) |
| Responses |
Re: Multi-Column List Partitioning
|
| List | pgsql-hackers |
On Fri, Oct 22, 2021 at 3:50 AM Zhihong Yu <zyu@yugabyte.com> wrote:
On Fri, Oct 22, 2021 at 2:48 AM Nitin Jadhav <nitinjadhavpostgres@gmail.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 JadhavHi,+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_countFor 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;
+ }
+ {
+ 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
+ {
+ *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));
+ 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
pgsql-hackers by date: