Re: BUG #18344: Pruning tables partitioned by bool range fails with invalid strategy

On Fri, 16 Feb 2024 at 05:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> What seems to be happening is that gen_prune_step_op is getting
> op_is_ne = true and doing this:
>
>         /*
>          * For clauses that contain an <> operator, set opstrategy to
>          * InvalidStrategy to signal get_matching_list_bounds to do the right
>          * thing.
>          */
>         opstep->opstrategy = op_is_ne ? InvalidStrategy : opstrategy;
>
> but then we're failing in get_matching_range_bounds, ie somebody
> taught get_matching_list_bounds to do the right thing but not
> any of the other code paths.

Yeah, prior to e0693faf7, we always did:

       partclause->op_is_ne = false;

so the code you quoted would always use the equality opstrategy,
therefore wouldn't hit the "if (opstrategy == InvalidStrategy)" block
in get_matching_list_bounds().

The old code wrongly assumed "bool IS NOT true" was the same as "bool
IS false" and vice-versa.  I had thought I could fix this by making it
use the same code that we do for cases like int4col <> 123, but:

a) only get_matching_list_bounds() knows how to handle those, not the
equivalent hash and range versions and;
b) bool is not true matches NULLs, but int4col <> 123 does not.

So, I'm a little unsure if we should try and make bool IS NOT clauses
prune partitions at all. It was a bug in the original code that
thought we could do that, and it looks like I didn't do a good job of
fixing that.

I see three options:

1) Make get_matching_list_bounds() work for bool IS NOT clauses by
properly including the NULL partition when handling a bool IS NOT
clause.
2) Just don't generate a pruning step for bool IS NOT clauses.
3) Just always include the NULL partition in
get_matching_list_bounds's "if (opstrategy == InvalidStrategy)" block.

I don't quite see how to do #1 as we don't have an easy way to tell if
we're handling bool IS NOT clauses inside get_matching_list_bounds().
Maybe we could check the comparison function is btboolcmp. That's
kinda cruddy. We don't have oid_symbol for pg_proc.dat as we do in
pg_operator.dat, so there's no #define for the pg_proc entry's Oid.

If we do #2, I'm concerned we'll regress someone's workload by
including the other bool partition.  Likewise, for #3, we'll include
the NULL partition for non-boolean cases, which could cause someone
problems.

The attached does #3 plus disables "bool IS NOT" pruning for RANGE and
HASH to avoid the reported "invalid strategy number 0." error.

Maybe we could do #1 by checking for partsupfunc.fn_oid == 1693 in the
back branches and come up with a cleaner way in master by adding a new
field to PartitionPruneStepOp.

Keen to get feedback on this one. Also included Amit and Álvaro as
they might have another idea.

I also noticed that the following code returns PARTCLAUSE_NOMATCH:

if (part_scheme->strategy != PARTITION_STRATEGY_LIST)
    return PARTCLAUSE_NOMATCH;

I think that should return PARTCLAUSE_UNSUPPORTED. But it's really
only an inefficiency rather than a bug, I think.

David