Re: BUG #14235: inconsistencies with IS NULL / IS NOT NULL

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> "Andrew" == Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> Andrew> It seems possible that this could be fixed by simply setting
> Andrew> argisrow=false for all the null tests generated in such cases.

> This turns out to be more somplicated than I thought. A lot of places
> look at argisrow to distinguish "simple" null tests from the
> standard-required logic for composite values. In some of the cases I've
> looked at, fixing the bug actually looks like it improves things (for
> example, in (row(1,a) IS NOT NULL), a can be deduced to be not null in
> the "not the null value" sense); but some places like
> match_clause_to_indexcol I'm less sure of.

I looked around to see what other consequences there might be. I think
that match_clause_to_indexcol is fine, because it's mighty unlikely that
any index type would implement argisrow semantics for IS[NOT]NULL.
However, I found a couple of things that seem like bugs:

1. get_relation_constraints() converts attnotnull column markings into
"col IS NOT NULL" constraints, ie it sets argisrow true for a composite
column. Perhaps ideally that would be an accurate representation of the
constraint semantics, but today it is not; a correct representation would
be a "simple not null" constraint. We are overpromising what the
constraint implies.

2. ruleutils.c will print a NullTest node as "IS [NOT] NULL" regardless
of its argisrow setting. For the case with a rowtype argument and not
argisrow, that is an incorrect depiction of the semantics. Since such a
case can't arise in parser output, only in a plan, this doesn't break
view dumping, but it could lead to misleading EXPLAIN output.

3. postgres_fdw's deparse.c is likewise naive about NullTest, and here
that *can* lead to an indisputable bug if a const-folded condition is
being sent to the remote side. It'd be better to issue IS [NOT] DISTINCT
FROM NULL in such cases.

Problem #1 is a pre-existing error, but problems #2/#3 are newly
introduced by this patch, since up to now argisrow has merely been a
pre-cached indication of the argument type, not an independent variable.

So one solution approach is to say "okay, argisrow is now independent,
deal with it". In that case #1 could be fixed trivially by setting
argisrow = false in the generated NullTest, and I think we would have
to modify ruleutils.c and deparse.c to print scalar NullTest on a
composite value as IS [NOT] DISTINCT FROM NULL.

The other general answer is to revert to the previous belief that argisrow
is merely a helpful cache. In that case, ruleutils is fine, but both
eval_const_expressions and get_relation_constraints would need to be fixed
to emit DistinctExprs in the cases where they now want to emit an argisrow
= false NullTest for a composite input.

The first approach seems to carry a rather larger risk of breaking
third-party code, since it removes an invariant that used to exist.
(The fact that we have to touch postgres_fdw is certainly a red flag
that other FDWs might be affected.) On the other hand, the planner
generally has a lot more intelligence about NullTest than about
DistinctExpr, and so I'm worried that substituting the latter for the
former might result in some nasty performance regressions for specific
queries.

I'm leaning slightly to the first approach, but could probably be
talked out of it if anyone sees additional arguments. Comments?

regards, tom lane