Re: Unexpected (wrong?) result querying boolean partitioned table with NULL partition

On Thu, 13 Apr 2023 at 15:45, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> On Thu, 13 Apr 2023 at 15:30, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> > BTW, I wonder if we should elog an Error here.
> >
> > default:
> > - Assert(false); /* hmm? */
> > - return PARTCLAUSE_UNSUPPORTED;
> > + elog(ERROR, "unrecognized booltesttype: %d",
> > + (int) btest->booltesttype);
> > + break;
>
> I wondered about that, hence my not-so-commitable comment left in there.
>
> My last thoughts were that maybe we should just move the IS_UNKNOWN
> and IS_NOT_UNKNOWN down into the switch and let -Wall let us know if
> something is missing.
>
> It hardly seems worth keeping the slightly earlier exit for those two
> cases. That just amounts to the RelabelType check and is this the
> partition key. I doubt IS[_NOT]_UNKNOWN is common enough for us to
> warrant contorting the code to make it a few dozen nanoseconds faster.
> Having smaller code is probably more of a win, which we'd get if we
> didn't add the ERROR you propose.

After having looked at the code in more detail, I don't think it's a
good idea to move the IS_UNKNOWN and IS_NOT_UNKNOWN down into the
switch. Having them tested early means we can return
PARTCLAUSE_UNSUPPORTED even when the clause does not match the current
partition key. If we moved those into the switch statement, then if
the qual didn't match to the partition key, then we'd return
PARTCLAUSE_NOMATCH and we'd maybe waste further effort later trying to
match the same qual to some other partition key.

All I ended up doing was removing the Assert(). I don't really see
the need to add an ERROR. It's not like any other value would cause
the code to misbehave. We'll just return PARTCLAUSE_UNSUPPORTED and
no pruning would get done for that qual. I also struggle to imagine
what possible other values we could ever add to BoolTestType.

After looking a bit deeper and testing a bit more, I found another bug
in match_boolean_partition_clause() around the
equal(negate_clause((Node *) leftop), partkey). The code there just
always set *outconst to a false Const regardless of is_not_clause. I
see the code coverage tool shows that line as untested, so I fixed the
bug and wrote some tests to exercise the code.

As David Kimura suggested, I also added some data to the tables in
question and repeated the same queries again without the EXPLAIN. I
generated the expected output with enable_partition_pruning = off then
put it back on again and saw that the same results are shown. I
considered writing a plpgsql function that we can pass a table name
and a query and it goes and makes a temp table, populates it with the
query with enable_partition_pruning = off then tries again with
pruning on and verifies the results are the same as what's stored in
the temp table. I'll maybe go and do that for master only, it's just a
bit more than what I wanted to do in the back branches.

I've pushed the fix now.

Thanks for the report about this, David, and thank you both for the reviews.

David

David