Re: Incorrect selectivity estimates for NOT on nullable boolean columns

Hi Ilia,

Thank you for finding this issue and for fixing the patch.

I've reviewed the patch and run some tests.

When I try WHERE b = false, the parser also represents it as an is_notclause (a BoolExpr with NOT_EXPR),
just as it does for WHERE NOT b.

The original semantic logic is:
s1 = 1.0 - clause_selectivity_ext(root, arg, ...);

In other words, clause_selectivity_ext(root, arg, ...) is expected to return the selectivity of the underlying clause being TRUE
, so we subtract it from 1.0 to obtain the selectivity of the FALSE case.

To preserve this original semantic behavior, I can think of two approaches. I tested them , and all two are relatively straightforward
and provide a consistent way to handle the logic as patch does.

[1]. Call booltestsel() in selfuncs.c directly and delegate the logic to booltestsel(IS_FALSE):
s1 = booltestsel(root, IS_FALSE, arg, varRelid, jointype, sjinfo);

The main logic of booltestsel() is:
[1.1] When statistics are available, it applies the relevant IS_FALSE logic.
[1.2] When statistics are unavailable, it still falls back to: 1.0 - (double) clause_selectivity()

Since clause_selectivity() unconditionally calls clause_selectivity_ext() without any additional transformation, this is equivalent to:
1.0 - (double) clause_selectivity_ext(..., true)

[2]. Construct a BooleanTest(IS_FALSE) node and delegate the processing to the existing BooleanTest handling
in the else if (IsA(clause, BooleanTest)) branch of clause_selectivity_ext():

BooleanTest *booltest = makeNode(BooleanTest);

booltest->arg = get_notclausearg((Expr *) clause);
booltest->booltesttype = IS_FALSE;
booltest->location = -1;

s1 = clause_selectivity_ext(root,
(Node *) booltest,
varRelid,
jointype,
sjinfo,
use_extended_stats);

Abouting patch:
[3]. Keep the handling within the current if branch and split the logic of booltestsel() between the two cases:
Node *arg = (Node *) get_notclausearg((Expr *) clause);
VariableStatData vardata;

examine_variable(root, arg, varRelid, &vardata);
if (HeapTupleIsValid(vardata.statsTuple))
{
/* Use statistics to select FALSE values, excluding NULLs. */
s1 = booltestsel(root, IS_FALSE, arg, varRelid, jointype, sjinfo);
}
else
{
/* Otherwise, estimate the underlying clause */
s1 = 1.0 - clause_selectivity_ext(root, arg,
varRelid,
jointype,
sjinfo,
use_extended_stats);
}
ReleaseVariableStats(vardata);

In the patch, case [1.1] is used when statistics are available, while case [1.2] is handled directly when statistics are unavailable.

After testing them, my understanding is that all three approaches produce the same selectivity estimates in both cases:
when statistics are available and when they are not.

The main differences are where the responsibility for handling this logic resides:
- [2] keeps the handling within the current clause_selectivity_ext() function by constructing a BooleanTest node.

- [1] delegates the handling to booltestsel(IS_FALSE) in selfuncs.c.

- [3] keeps the handling within the current if branch and reimplements the relevant parts of booltestsel() locally.
This avoids the additional call chain booltestsel() -> clause_selectivity() -> clause_selectivity_ext() when
statistics are unavailable, while retaining the same logic.

Personally, I prefer approach [1]. I think centralizing the semantics in booltestsel() could help keep the handling consistent,
reduce maintenance overhead, and make future extensions to the function easier to manage.

Please let me know if I've overlooked anything or if you see any issues with this reasoning.

What do you think?

regards,
--
ZizhuanLiu (X-MAN) 
44973863(at)qq(dot)com