Fix assorted fallout from IS [NOT] NULL patch.
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 28 Jul 2016 20:09:15 +0000 (16:09 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 28 Jul 2016 20:09:15 +0000 (16:09 -0400)
Commits 4452000f3 et al established semantics for NullTest.argisrow that
are a bit different from its initial conception: rather than being merely
a cache of whether we've determined the input to have composite type,
the flag now has the further meaning that we should apply field-by-field
testing as per the standard's definition of IS [NOT] NULL.  If argisrow
is false and yet the input has composite type, the construct instead has
the semantics of IS [NOT] DISTINCT FROM NULL.  Update the comments in
primnodes.h to clarify this, and fix ruleutils.c and deparse.c to print
such cases correctly.  In the case of ruleutils.c, this merely results in
cosmetic changes in EXPLAIN output, since the case can't currently arise
in stored rules.  However, it represents a live bug for deparse.c, which
would formerly have sent a remote query that had semantics different
from the local behavior.  (From the user's standpoint, this means that
testing a remote nested-composite column for null-ness could have had
unexpected recursive behavior much like that fixed in 4452000f3.)

In a related but somewhat independent fix, make plancat.c set argisrow
to false in all NullTest expressions constructed to represent "attnotnull"
constructs.  Since attnotnull is actually enforced as a simple null-value
check, this is a more accurate representation of the semantics; we were
previously overpromising what it meant for composite columns, which might
possibly lead to incorrect planner optimizations.  (It seems that what the
SQL spec expects a NOT NULL constraint to mean is an IS NOT NULL test, so
arguably we are violating the spec and should fix attnotnull to do the
other thing.  If we ever do, this part should get reverted.)

Back-patch, same as the previous commit.

Discussion: <10682.1469566308@sss.pgh.pa.us>

contrib/postgres_fdw/deparse.c
src/backend/optimizer/util/plancat.c
src/backend/utils/adt/ruleutils.c
src/include/nodes/primnodes.h
src/test/regress/expected/rowtypes.out

index 33610c4040d6b26cdd7eb2397761835516dac78c..6476005f6d5443eff5f1755a104b2935b5315e2e 100644 (file)
@@ -2347,10 +2347,27 @@ deparseNullTest(NullTest *node, deparse_expr_cxt *context)
 
        appendStringInfoChar(buf, '(');
        deparseExpr(node->arg, context);
-       if (node->nulltesttype == IS_NULL)
-               appendStringInfoString(buf, " IS NULL)");
+
+       /*
+        * For scalar inputs, we prefer to print as IS [NOT] NULL, which is
+        * shorter and traditional.  If it's a rowtype input but we're applying a
+        * scalar test, must print IS [NOT] DISTINCT FROM NULL to be semantically
+        * correct.
+        */
+       if (node->argisrow || !type_is_rowtype(exprType((Node *) node->arg)))
+       {
+               if (node->nulltesttype == IS_NULL)
+                       appendStringInfoString(buf, " IS NULL)");
+               else
+                       appendStringInfoString(buf, " IS NOT NULL)");
+       }
        else
-               appendStringInfoString(buf, " IS NOT NULL)");
+       {
+               if (node->nulltesttype == IS_NULL)
+                       appendStringInfoString(buf, " IS NOT DISTINCT FROM NULL)");
+               else
+                       appendStringInfoString(buf, " IS DISTINCT FROM NULL)");
+       }
 }
 
 /*
index 149bd62dca51f36386f3e35c512937d696324ba8..5d18206b91b3527e1e9f8741545b1fefea6cfde0 100644 (file)
@@ -1198,7 +1198,13 @@ get_relation_constraints(PlannerInfo *root,
                                                                                                  att->attcollation,
                                                                                                  0);
                                        ntest->nulltesttype = IS_NOT_NULL;
-                                       ntest->argisrow = type_is_rowtype(att->atttypid);
+
+                                       /*
+                                        * argisrow=false is correct even for a composite column,
+                                        * because attnotnull does not represent a SQL-spec IS NOT
+                                        * NULL test in such a case, just IS DISTINCT FROM NULL.
+                                        */
+                                       ntest->argisrow = false;
                                        ntest->location = -1;
                                        result = lappend(result, ntest);
                                }
index 2915e217642bc916066ae082096065bfec3c6001..51c765ad8772b2c38e625f5aa5940038bb95860e 100644 (file)
@@ -8047,17 +8047,43 @@ get_rule_expr(Node *node, deparse_context *context,
                                if (!PRETTY_PAREN(context))
                                        appendStringInfoChar(buf, '(');
                                get_rule_expr_paren((Node *) ntest->arg, context, true, node);
-                               switch (ntest->nulltesttype)
+
+                               /*
+                                * For scalar inputs, we prefer to print as IS [NOT] NULL,
+                                * which is shorter and traditional.  If it's a rowtype input
+                                * but we're applying a scalar test, must print IS [NOT]
+                                * DISTINCT FROM NULL to be semantically correct.
+                                */
+                               if (ntest->argisrow ||
+                                       !type_is_rowtype(exprType((Node *) ntest->arg)))
                                {
-                                       case IS_NULL:
-                                               appendStringInfoString(buf, " IS NULL");
-                                               break;
-                                       case IS_NOT_NULL:
-                                               appendStringInfoString(buf, " IS NOT NULL");
-                                               break;
-                                       default:
-                                               elog(ERROR, "unrecognized nulltesttype: %d",
-                                                        (int) ntest->nulltesttype);
+                                       switch (ntest->nulltesttype)
+                                       {
+                                               case IS_NULL:
+                                                       appendStringInfoString(buf, " IS NULL");
+                                                       break;
+                                               case IS_NOT_NULL:
+                                                       appendStringInfoString(buf, " IS NOT NULL");
+                                                       break;
+                                               default:
+                                                       elog(ERROR, "unrecognized nulltesttype: %d",
+                                                                (int) ntest->nulltesttype);
+                                       }
+                               }
+                               else
+                               {
+                                       switch (ntest->nulltesttype)
+                                       {
+                                               case IS_NULL:
+                                                       appendStringInfoString(buf, " IS NOT DISTINCT FROM NULL");
+                                                       break;
+                                               case IS_NOT_NULL:
+                                                       appendStringInfoString(buf, " IS DISTINCT FROM NULL");
+                                                       break;
+                                               default:
+                                                       elog(ERROR, "unrecognized nulltesttype: %d",
+                                                                (int) ntest->nulltesttype);
+                                       }
                                }
                                if (!PRETTY_PAREN(context))
                                        appendStringInfoChar(buf, ')');
index 057cc2ca85e969c671a22195352f722a904219ed..df2d27d77ca6ca8270dd3a3d85f448fb5e25f022 100644 (file)
@@ -1099,8 +1099,16 @@ typedef struct XmlExpr
  * NullTest represents the operation of testing a value for NULLness.
  * The appropriate test is performed and returned as a boolean Datum.
  *
- * NOTE: the semantics of this for rowtype inputs are noticeably different
- * from the scalar case.  We provide an "argisrow" flag to reflect that.
+ * When argisrow is false, this simply represents a test for the null value.
+ *
+ * When argisrow is true, the input expression must yield a rowtype, and
+ * the node implements "row IS [NOT] NULL" per the SQL standard.  This
+ * includes checking individual fields for NULLness when the row datum
+ * itself isn't NULL.
+ *
+ * NOTE: the combination of a rowtype input and argisrow==false does NOT
+ * correspond to the SQL notation "row IS [NOT] NULL"; instead, this case
+ * represents the SQL notation "row IS [NOT] DISTINCT FROM NULL".
  * ----------------
  */
 
@@ -1114,7 +1122,7 @@ typedef struct NullTest
        Expr            xpr;
        Expr       *arg;                        /* input expression */
        NullTestType nulltesttype;      /* IS NULL, IS NOT NULL */
-       bool            argisrow;               /* T if input is of a composite type */
+       bool            argisrow;               /* T to perform field-by-field null checks */
        int                     location;               /* token location, or -1 if unknown */
 } NullTest;
 
index 2971640b4bd28b1cfccc2111b8232e526773ccb8..25b08281c85b781c6176a81caf5e6055aa1f6532 100644 (file)
@@ -664,10 +664,10 @@ explain (verbose, costs off)
 select r, r is null as isnull, r is not null as isnotnull
 from (values (1,row(1,2)), (1,row(null,null)), (1,null),
              (null,row(1,2)), (null,row(null,null)), (null,null) ) r(a,b);
-                                                                                           QUERY PLAN                                                                                            
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+                                                                                                         QUERY PLAN                                                                                                          
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Values Scan on "*VALUES*"
-   Output: ROW("*VALUES*".column1, "*VALUES*".column2), (("*VALUES*".column1 IS NULL) AND ("*VALUES*".column2 IS NULL)), (("*VALUES*".column1 IS NOT NULL) AND ("*VALUES*".column2 IS NOT NULL))
+   Output: ROW("*VALUES*".column1, "*VALUES*".column2), (("*VALUES*".column1 IS NULL) AND ("*VALUES*".column2 IS NOT DISTINCT FROM NULL)), (("*VALUES*".column1 IS NOT NULL) AND ("*VALUES*".column2 IS DISTINCT FROM NULL))
 (2 rows)
 
 select r, r is null as isnull, r is not null as isnotnull