Re: Multi-Column List Partitioning

On Wed, Dec 29, 2021 at 7:26 PM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:
>
>
> > -        * 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.
>
> AFAIK, the above condition/comments says that, either we should
> include all keys or prefixes of the partition keys to get the
> partition pruning results. For example if we have a table with 2
> columns and both are present in the partition key. Let the column
> names be 'a' and 'b'.
>
> SELECT * FROM table WHERE a=1 AND b=1;  - This query works for pruning
> and it refers to a comment which says all partition keys are included.
> SELECT * FROM table WHERE b=1;  - Here partition pruning does not work
> as it does not contain prefix of the partition keys.
> SELECT * FROM table WHERE a=1; - This query works fine as column 'a'
> is prefix of partition keys.
>
> Please let me know if you need more information.

That what I was assuming is not correct. The dependency of the prefix
is true for the range partitioning but why should that be in the case
of list partitioning? I think all partitioning keys in the list will
not be dependent on each other, AFAICU. If you prune list partitions
based on the b=1 value that still is correct & gives the correct
result, correct me If I am wrong.

> ---
>
> > +/*
> > + * 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 we don't make a separate function, then we have to include this
> code in get_matching_list_bounds() which is already a big function. I
> just made a separate function to not increase the complexity of
> get_matching_list_bounds() and most of the code present in
> get_min_or_max_off() is common for min and max calculation. If we make
> it separate then there might be a lot of duplications. Please let me
> know if you still feel if any action is required.

Hmm, ok, I personally didn't like to have two functions one gives max
and min and the other gives only max or min, the other could have
different opinions.

How about keeping only one function say, get_min_max_off() and based
on the argument e.g. minoff & maxoff fetch the value, I mean e.g. if
minoff is not null then fetch the value otherwise skip that, same for
maxoff too.

> ---
>
> > +       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.
>
> As other partitions which support multiple partition keys (Range
> partitioning) do not support NULL values. This feature supports
> multiple partition keys with list partitioning and it also supports
> NULL values. With the existing design, I have tried to support this
> feature with minimal changes as possible. If this is not the right
> approach to support NULL values, I would like to know how we can
> support multiple NULL values. Kindly provide more information.

I haven't studied the whole partition pruning code and don't know the
complete code flow, but AFAICU, this is not the correct way to handle null
value.

Regards,
Amul