pgsql: Teach expr_is_nonnullable() to handle more expression types
Teach expr_is_nonnullable() to handle more expression types
Currently, the function expr_is_nonnullable() checks only Const and
Var expressions to determine if an expression is non-nullable. This
patch extends the detection logic to handle more expression types.
This can enable several downstream optimizations, such as reducing
NullTest quals to constant truth values (e.g., "COALESCE(var, 1) IS
NULL" becomes FALSE) and converting "COUNT(expr)" to the more
efficient "COUNT(*)" when the expression is proven non-nullable.
This breaks a test case in test_predtest.sql, since we now simplify
"ARRAY[] IS NULL" to constant FALSE, preventing it from weakly
refuting a strict ScalarArrayOpExpr ("x = any(ARRAY[])"). To ensure
the refutation logic is still exercised as intended, wrap the array
argument in opaque_array().
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Reviewed-by: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Discussion: /messages/by-id/CAMbWs49UhPBjm+NRpxerjaeuFKyUZJ_AjM3NBcSYK2JgZ6VTEQ@mail.gmail.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/c8d2f68cc800e02f858f8f59e3e9ba46ce43e880
Modified Files
--------------
src/backend/optimizer/util/clauses.c | 123 ++++++++++++++++++++-
.../test_predtest/expected/test_predtest.out | 2 +-
.../modules/test_predtest/sql/test_predtest.sql | 2 +-
src/test/regress/expected/predicate.out | 117 +++++++++++++++++++-
src/test/regress/sql/predicate.sql | 50 ++++++++-
5 files changed, 284 insertions(+), 10 deletions(-)
On Wed, Dec 24, 2025 at 4:02 AM Richard Guo <rguo@postgresql.org> wrote:
Teach expr_is_nonnullable() to handle more expression types
Due to this commit, expr_is_nonnullable() claims that "IS DISTINCT
FROM never returns NULL", but execExprInterp.c's implementation for
EEOP_DISTINCT disagrees; when neither argument is NULL, it returns
null if the underlying operator does. Two Claude-written test cases
illustrating the problem are attached.
--
Robert Haas
Databricks
On Tue, Oct 6, 2026 at 11:50 PM Robert Haas <robertmhaas@gmail.com> wrote:
Due to this commit, expr_is_nonnullable() claims that "IS DISTINCT
FROM never returns NULL", but execExprInterp.c's implementation for
EEOP_DISTINCT disagrees; when neither argument is NULL, it returns
null if the underlying operator does. Two Claude-written test cases
illustrating the problem are attached.
Right, thanks for the report. This needs an equality operator that
returns NULL for two non-null inputs, which I think is rare in
practice, but nothing forbids it, so I agree we should fix it.
I think we can just remove the T_DistinctExpr case from
expr_is_nonnullable(). An alternative is that we keep it, but only
when the operator is a btree equality operator, as such an operator
must not return NULL for non-null inputs. This keeps the optimization
for nearly all real-world cases.
What do you think?
- Richard
On Tue, Oct 6, 2026 at 9:30 PM Richard Guo <guofenglinux@gmail.com> wrote:
I think we can just remove the T_DistinctExpr case from
expr_is_nonnullable(). An alternative is that we keep it, but only
when the operator is a btree equality operator, as such an operator
must not return NULL for non-null inputs. This keeps the optimization
for nearly all real-world cases.
I'm not sure. expr_is_nonnullable() doesn't look like an appealing
place to do being catalog lookups. I see that var_is_nonnullable()
does do catalog lookups when source == NOTNULL_SOURCE_CATALOG, which
strikes me as not great (and why does it test has_subclass() before
rte->relkind?!). But I'd be inclined to use a conditional rule only if
we can base it on information that we already have cached in some
planner data structure, and not if we'd have to look it up in the
catalog.
--
Robert Haas
Databricks
On Thu, Oct 8, 2026 at 10:49 PM Robert Haas <robertmhaas@gmail.com> wrote:
But I'd be inclined to use a conditional rule only if
we can base it on information that we already have cached in some
planner data structure, and not if we'd have to look it up in the
catalog.
We don't have that cached, so I've pushed a fix that just removes the
case. It also moves the relkind test ahead of has_subclass().
- Richard