Re: BUG #14235: inconsistencies with IS NULL / IS NOT NULL - Mailing list pgsql-bugs
From | Tom Lane |
---|---|
Subject | Re: BUG #14235: inconsistencies with IS NULL / IS NOT NULL |
Date | |
Msg-id | 10682.1469566308@sss.pgh.pa.us Whole thread Raw |
In response to | Re: BUG #14235: inconsistencies with IS NULL / IS NOT NULL (Andrew Gierth <andrew@tao11.riddles.org.uk>) |
List | pgsql-bugs |
Andrew Gierth <andrew@tao11.riddles.org.uk> writes: > "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.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
pgsql-bugs by date: