Fix planner's nullability/strictness logic for ScalarArrayOpExpr.

find_nonnullable_rels and find_nonnullable_vars mistakenly treated a
ScalarArrayOpExpr that could return FALSE as strict, but that's okay
only at top level of a qual expression; further down, we've got to
insist on a guaranteed-NULL result.  The result was that we could draw
mistaken conclusions about whether outer joins can be simplified, if
the decision hinged on a non-top-level ScalarArrayOpExpr with a
potentially-empty array argument.

I believe this error dates to commit 72a070a36, which taught
find_nonnullable_rels to descend into non-top-level parts of qual
expressions.  is_strict_saop (added earlier by 72153c058) already had
enough intelligence to do the case correctly, but it wasn't passed the
proper flag, ie "top_level" needs to be passed for "falseOK".
e006a24ad copied that mistake into find_nonnullable_vars.

Later, over-eager refactoring in commit 2f153ddfd broke
contain_nonstrict_functions' handling of ScalarArrayOpExpr by treating
it as though it were no different from an OpExpr.  It is, because
we must also prove the array is non-empty before concluding that the
expression is strict.  This could result in misclassifying an
expression as strict when it is not, leading to assorted planning
mistakes such as inlining a SQL function that shouldn't be inlined.
We can almost fix this by just re-adding the previous handling of
ScalarArrayOpExpr in that function, but doing only that would lead to
also calling check_functions_in_node() and thus redundantly checking
the operator's strictness.  Avoid that by turning the if-series into
an else-if chain, as it arguably should have been all along.

The reason these errors have escaped detection for decades is that
they are exposed only in arcane corner cases.  ScalarArrayOpExpr with
an empty array isn't typical usage, and even when that's possible
several other conditions apply before the planner can reach a mistaken
conclusion.  While it's possible to build test cases demonstrating
these mistakes, I (tgl) judged them too indirect and special-purpose
to justify consuming regression test cycles forevermore.

Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAJTYsWV3vqRJmST-gv1NsXEef-zOnjVJpYS910aBaiuMij4nFg@mail.gmail.com
Discussion: https://postgr.es/m/CAJTYsWWcLGmz0f8_QPP_Liq-fc7-geiFSCdqoq3XGeRHPPsWeA@mail.gmail.com
Backpatch-through: 14

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index aa8886ec210e944a45d122a4ca445d1d9aadda10..7d7f2f9664bb1b0c614f12e4b94050b78b7982c6 100644 (file)
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -1040,7 +1040,7 @@ contain_nonstrict_functions_walker(Node *node, void *context)
                /* an aggregate could return non-null with null input */
                return true;
        }
-       if (IsA(node, GroupingFunc))
+       else if (IsA(node, GroupingFunc))
        {
                /*
                 * A GroupingFunc doesn't evaluate its arguments, and therefore must
@@ -1048,12 +1048,12 @@ contain_nonstrict_functions_walker(Node *node, void *context)
                 */
                return true;
        }
-       if (IsA(node, WindowFunc))
+       else if (IsA(node, WindowFunc))
        {
                /* a window function could return non-null with null input */
                return true;
        }
-       if (IsA(node, SubscriptingRef))
+       else if (IsA(node, SubscriptingRef))
        {
                SubscriptingRef *sbsref = (SubscriptingRef *) node;
                const SubscriptRoutines *sbsroutines;
@@ -1067,17 +1067,25 @@ contain_nonstrict_functions_walker(Node *node, void *context)
                        return true;
                /* else fall through to check args */
        }
-       if (IsA(node, DistinctExpr))
+       else if (IsA(node, DistinctExpr))
        {
                /* IS DISTINCT FROM is inherently non-strict */
                return true;
        }
-       if (IsA(node, NullIfExpr))
+       else if (IsA(node, NullIfExpr))
        {
                /* NULLIF is inherently non-strict */
                return true;
        }
-       if (IsA(node, BoolExpr))
+       else if (IsA(node, ScalarArrayOpExpr))
+       {
+               ScalarArrayOpExpr *expr = (ScalarArrayOpExpr *) node;
+
+               if (!is_strict_saop(expr, false))
+                       return true;
+               /* else fall through to check args */
+       }
+       else if (IsA(node, BoolExpr))
        {
                BoolExpr   *expr = (BoolExpr *) node;
 
@@ -1091,28 +1099,26 @@ contain_nonstrict_functions_walker(Node *node, void *context)
                                break;
                }
        }
-       if (IsA(node, SubLink))
+       else if (IsA(node, SubLink))
        {
                /* In some cases a sublink might be strict, but in general not */
                return true;
        }
-       if (IsA(node, SubPlan))
+       else if (IsA(node, SubPlan))
                return true;
-       if (IsA(node, AlternativeSubPlan))
+       else if (IsA(node, AlternativeSubPlan))
                return true;
-       if (IsA(node, FieldStore))
+       else if (IsA(node, FieldStore))
                return true;
-       if (IsA(node, CoerceViaIO))
+       else 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.
+                * so we should skip check_functions_in_node() and just fall through
+                * to check the arguments.
                 */
-               return contain_nonstrict_functions_walker((Node *) ((CoerceViaIO *) node)->arg,
-                                                                                                 context);
        }
-       if (IsA(node, ArrayCoerceExpr))
+       else if (IsA(node, ArrayCoerceExpr))
        {
                /*
                 * ArrayCoerceExpr is strict at the array level, regardless of what
@@ -1122,31 +1128,33 @@ contain_nonstrict_functions_walker(Node *node, void *context)
                return contain_nonstrict_functions_walker((Node *) ((ArrayCoerceExpr *) node)->arg,
                                                                                                  context);
        }
-       if (IsA(node, CaseExpr))
+       else if (IsA(node, CaseExpr))
                return true;
-       if (IsA(node, ArrayExpr))
+       else if (IsA(node, ArrayExpr))
                return true;
-       if (IsA(node, RowExpr))
+       else if (IsA(node, RowExpr))
                return true;
-       if (IsA(node, RowCompareExpr))
+       else if (IsA(node, RowCompareExpr))
                return true;
-       if (IsA(node, CoalesceExpr))
+       else if (IsA(node, CoalesceExpr))
                return true;
-       if (IsA(node, MinMaxExpr))
+       else if (IsA(node, MinMaxExpr))
                return true;
-       if (IsA(node, XmlExpr))
+       else if (IsA(node, XmlExpr))
                return true;
-       if (IsA(node, NullTest))
-               return true;
-       if (IsA(node, BooleanTest))
+       else if (IsA(node, NullTest))
                return true;
-       if (IsA(node, JsonConstructorExpr))
+       else if (IsA(node, BooleanTest))
                return true;
-
-       /* Check other function-containing nodes */
-       if (check_functions_in_node(node, contain_nonstrict_functions_checker,
-                                                               context))
+       else if (IsA(node, JsonConstructorExpr))
                return true;
+       else
+       {
+               /* Check other function-containing nodes */
+               if (check_functions_in_node(node, contain_nonstrict_functions_checker,
+                                                                       context))
+                       return true;
+       }
 
        return expression_tree_walker(node, contain_nonstrict_functions_walker,
                                                                  context);
@@ -1546,7 +1554,7 @@ find_nonnullable_rels_walker(Node *node, bool top_level)
        {
                ScalarArrayOpExpr *expr = (ScalarArrayOpExpr *) node;
 
-               if (is_strict_saop(expr, true))
+               if (is_strict_saop(expr, top_level))
                        result = find_nonnullable_rels_walker((Node *) expr->args, false);
        }
        else if (IsA(node, BoolExpr))
@@ -1799,7 +1807,7 @@ find_nonnullable_vars_walker(Node *node, bool top_level)
        {
                ScalarArrayOpExpr *expr = (ScalarArrayOpExpr *) node;
 
-               if (is_strict_saop(expr, true))
+               if (is_strict_saop(expr, top_level))
                        result = find_nonnullable_vars_walker((Node *) expr->args, false);
        }
        else if (IsA(node, BoolExpr))