I poked into a recent complaint[1] about PG not being terribly smart
about whether an IS NOT NULL index predicate is implied by a WHERE
clause, and determined that there are a couple of places where we
are being less bright than we could be about CoerceViaIO semantics.
CoerceViaIO is strict independently of whether the I/O functions it
calls are (and they might not be --- in particular, domain_in isn't).
However, not everyplace knows that:
* clauses.c's contain_nonstrict_functions_walker() uses default logic
that will examine the referenced I/O functions to see if they're strict.
That's expensive, requiring several syscache lookups, and it might not
even give the right answer --- though fortunately it'd err in the
conservative direction.
* predtest.c's clause_is_strict_for() doesn't know anything about
CoerceViaIO, so it fails to make the proof requested in [1].
I started to fix this, and was in the midst of copying-and-pasting
contain_nonstrict_functions_walker's handling of ArrayCoerceExpr,
when I realized that that code is actually wrong:
return expression_tree_walker((Node *) ((ArrayCoerceExpr *) node)->arg,
contain_nonstrict_functions_walker,
context);
It should be recursing to itself, not to expression_tree_walker.
As coded, the strictness check doesn't get applied to the immediate
child node of the ArrayCoerceExpr, so that if that node is non-strict
we may arrive at the wrong conclusion.
contain_nonstrict_functions() isn't used in very many places, fortunately,
and ArrayCoerceExpr isn't that easy to produce either, which may explain
the lack of field reports. I was able to cons up this example though,
which demonstrates an incorrect conclusion about whether it's safe to
inline a function declared STRICT:
regression=# create table t1 (f1 int);
CREATE TABLE
regression=# insert into t1 values(1),(null);
INSERT 0 2
regression=# create or replace function sfunc(int) returns int[] language sql
as 'select array[0, $1]::bigint[]::int[]' strict;
CREATE FUNCTION
regression=# select sfunc(f1) from t1;
sfunc
----------
{0,1}
{0,NULL}
(2 rows)
Of course, since sfunc is strict, that last output should be NULL not
an array containing a NULL.
The attached patch fixes both of these things. At least the second
hunk needs to be back-patched. I'm less sure about whether the
CoerceViaIO changes merit back-patching; they're not fixing wrong
answers, but they are responding to a field complaint. Thoughts?
regards, tom lane
[1] https://www.postgresql.org/message-id/CAHkN8V9Rfh6uAjQLURJfnHsQfC_MYiFUSWEVcwVSiPdokmkniw%40mail.gmail.com
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 3737166..501b0e9 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -1172,6 +1172,16 @@ contain_nonstrict_functions_walker(Node *node, void *context)
return true;
if (IsA(node, FieldStore))
return true;
+ if (IsA(node, CoerceViaIO))
+ {
+ /*
+ * CoerceViaIO is strict regardless of whether the I/O functions are,
+ * so just go look at its argument; asking check_functions_in_node is
+ * useless expense and could deliver the wrong answer.
+ */
+ return contain_nonstrict_functions_walker((Node *) ((CoerceViaIO *) node)->arg,
+ context);
+ }
if (IsA(node, ArrayCoerceExpr))
{
/*
@@ -1179,9 +1189,8 @@ contain_nonstrict_functions_walker(Node *node, void *context)
* the per-element expression is; so we should ignore elemexpr and
* recurse only into the arg.
*/
- return expression_tree_walker((Node *) ((ArrayCoerceExpr *) node)->arg,
- contain_nonstrict_functions_walker,
- context);
+ return contain_nonstrict_functions_walker((Node *) ((ArrayCoerceExpr *) node)->arg,
+ context);
}
if (IsA(node, CaseExpr))
return true;
diff --git a/src/backend/optimizer/util/predtest.c b/src/backend/optimizer/util/predtest.c
index 01f64ee..179b9aa 100644
--- a/src/backend/optimizer/util/predtest.c
+++ b/src/backend/optimizer/util/predtest.c
@@ -1352,6 +1352,15 @@ clause_is_strict_for(Node *clause, Node *subexpr)
return false;
}
+ /*
+ * CoerceViaIO is strict (whether or not the I/O functions it calls are).
+ * This is worth checking mainly because it saves us having to explain to
+ * users why some type coercions are known strict and others aren't.
+ */
+ if (IsA(clause, CoerceViaIO))
+ return clause_is_strict_for((Node *) ((CoerceViaIO *) clause)->arg,
+ subexpr);
+
return false;
}