On Thu, 13 Apr 2023 at 15:45, David Rowley <dgrowle...@gmail.com> wrote: > > On Thu, 13 Apr 2023 at 15:30, Richard Guo <guofengli...@gmail.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
