Re: BUG #16500: SQL Abend. select multi_key_columns_range_partition_table

On Wed, Jul 15, 2020 at 7:00 PM Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
> I have just a couple of questions:
>
> + List *step_exprs1,
> + *step_cmpfns1;
>
> pc = lfirst(lc);
> if (pc->keyno == cur_keyno)
> {
> - /* clean up before starting a new recursion cycle. */
> - if (cur_keyno == 0)
> - {
> - list_free(step_exprs);
> - list_free(step_cmpfns);
> - step_exprs = list_make1(pc->expr);
> - step_cmpfns = list_make1_oid(pc->cmpfn);
> - }
> - else
> - {
> - step_exprs = lappend(step_exprs, pc->expr);
> - step_cmpfns = lappend_oid(step_cmpfns, pc->cmpfn);
> - }
> + /* Leave the original step_exprs unmodified. */
> + step_exprs1 = list_copy(step_exprs);
> + step_exprs1 = lappend(step_exprs1, pc->expr);
> +
> + /* Leave the original step_cmpfns unmodified. */
> + step_cmpfns1 = list_copy(step_cmpfns);
> + step_cmpfns1 = lappend_oid(step_cmpfns1, pc->cmpfn);
>
> Does this also belong to some of mentioned fixes?

Yes, the above change is made to address this issue mentioned in the
commit message in the attached, which is an updated version of the
patch:

* The list to pass to get_steps_using_prefix() is allowed to contain
multiple clauses for the same partition key, as described in the
comment for that function, but that function actually assumed that the
list contained just a single clause for each of middle partition keys,
which led to an assertion failure when the list contained multiple
clauses for such partition keys. Update that function to match the
comment.

> I'm curious if there
> could be memory concumption concerns, since it's done within a recursive
> function.

I didn't think so because I thought that in practice, the number of
partition keys, the number of clauses for the same partition key, and
thus the number of combinations of clauses for partition keys are
small, but yes, it's a good thing to save memory, so I added the
list_free() calls to the above change.

> - Assert(list_length(step_exprs) == cur_keyno);
> + Assert(list_length(step_exprs) == cur_keyno ||
> + (context->rel->part_scheme->strategy ==
> + PARTITION_STRATEGY_HASH &&
> + step_opstrategy == HTEqualStrategyNumber &&
> + !bms_is_empty(step_nullkeys) &&
> + bms_num_members(step_nullkeys) + list_length(step_exprs) + 2 ==
> + context->rel->part_scheme->partnatts));
>
> I see the explanation in the commit message, but this assert condition
> is quite complex, maybe worth couple of commentary lines right there?

OK, to improve the readability, I split the assertion test into three,
and added comments. One thing I noticed related to the assertion test
is that there are still cases where we fail to pass the
"bms_num_members(step_nullkeys) + list_length(step_exprs) + 2 ==
context->rel->part_scheme->partnatts" test. Here is an example (Here,
I use a custom operator === to prevent the equivclass.c machinery from
reordering clauses. See the attached for the definition of it):

create table hp_contradict_test (a int, b int) partition by hash (a
part_test_int4_ops2, b part_test_int4_ops2);
create table hp_contradict_test_p1 partition of hp_contradict_test for
values with (modulus 2, remainder 0);
create table hp_contradict_test_p2 partition of hp_contradict_test for
values with (modulus 2, remainder 1);
explain (costs off) select * from hp_contradict_test where a is null
and a === 1 and b === 1;
QUERY PLAN
--------------------------
Result
One-Time Filter: false
(2 rows)

This works fine, BUT:

explain (costs off) select * from hp_contradict_test where a === 1 and
b === 1 and a is null;

this causes the failure to pass the mentioned test because for the
latter query, we fail to detect self-contradiction from "a === 1" and
"a is null", and then perform get_steps_using_prefix() with prefix
containing "a === 1" and step_nullkeys containing the partition key
"a", causing the failure. Fortunately, that doesn't cause any issue
on a normal build, as the self-contradiction is detected later in the
query processing. I fixed this issue as well in the attached, by
adding to get_partprune_steps() a little bit of code to detect the
self-contradiction.

Thanks for the review! Sorry for the late response.

Best regards,
Etsuro Fujita