]> granicus.if.org Git - postgresql/commitdiff
Code review for IS DISTINCT FROM patch. Fix incorrect constant-folding
authorTom Lane <tgl@sss.pgh.pa.us>
Sat, 30 Nov 2002 21:25:08 +0000 (21:25 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Sat, 30 Nov 2002 21:25:08 +0000 (21:25 +0000)
logic, dissuade planner from thinking that 'x IS DISTINCT FROM 42' may
be optimized into 'x = 42' (!!), cause dependency on = operator to be
recorded correctly, minor other improvements.

src/backend/catalog/dependency.c
src/backend/executor/execQual.c
src/backend/optimizer/plan/setrefs.c
src/backend/optimizer/util/clauses.c
src/backend/parser/parse_expr.c
src/include/nodes/primnodes.h
src/pl/plpgsql/src/pl_exec.c

index d313ec22531436beb0cc75f8043a82a600a40579..d159ceba371773eda7086376eec9ad7c9207af94 100644 (file)
@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/catalog/dependency.c,v 1.12 2002/09/22 00:37:09 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/catalog/dependency.c,v 1.13 2002/11/30 21:25:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -803,7 +803,8 @@ find_expr_references_walker(Node *node,
        {
                Expr       *expr = (Expr *) node;
 
-               if (expr->opType == OP_EXPR)
+               if (expr->opType == OP_EXPR ||
+                       expr->opType == DISTINCT_EXPR)
                {
                        Oper       *oper = (Oper *) expr->oper;
 
index d41ae779e4704cb1cae09381e76b88171218b058..90a38f31ee2ee50f3d8ebbbe9974e393707085db 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/executor/execQual.c,v 1.110 2002/11/25 21:29:35 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/executor/execQual.c,v 1.111 2002/11/30 21:25:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1144,7 +1144,7 @@ ExecEvalFunc(Expr *funcClause,
  * they are NULL; if either is NULL then the result is already
  * known. If neither is NULL, then proceed to evaluate the
  * function. Note that this is *always* derived from the equals
- * operator, but since we've already evaluated the arguments
+ * operator, but since we need special processing of the arguments
  * we can not simply reuse ExecEvalOper() or ExecEvalFunc().
  * ----------------------------------------------------------------
  */
@@ -1154,7 +1154,7 @@ ExecEvalDistinct(Expr *opClause,
                                 bool *isNull,
                                 ExprDoneCond *isDone)
 {
-       bool            result;
+       Datum           result;
        FunctionCachePtr fcache;
        FunctionCallInfoData fcinfo;
        ExprDoneCond argDone;
@@ -1162,10 +1162,7 @@ ExecEvalDistinct(Expr *opClause,
        List       *argList;
 
        /*
-        * we extract the oid of the function associated with the op and then
-        * pass the work onto ExecMakeFunctionResult which evaluates the
-        * arguments and returns the result of calling the function on the
-        * evaluated arguments.
+        * extract info from opClause
         */
        op = (Oper *) opClause->oper;
        argList = opClause->args;
@@ -1181,34 +1178,36 @@ ExecEvalDistinct(Expr *opClause,
                                                         econtext->ecxt_per_query_memory);
                op->op_fcache = fcache;
        }
-       Assert(fcache->func.fn_retset == FALSE);
+       Assert(!fcache->func.fn_retset);
 
        /* Need to prep callinfo structure */
        MemSet(&fcinfo, 0, sizeof(fcinfo));
        fcinfo.flinfo = &(fcache->func);
        argDone = ExecEvalFuncArgs(&fcinfo, argList, econtext);
+       if (argDone != ExprSingleResult)
+               elog(ERROR, "IS DISTINCT FROM does not support set arguments");
        Assert(fcinfo.nargs == 2);
 
        if (fcinfo.argnull[0] && fcinfo.argnull[1])
        {
                /* Both NULL? Then is not distinct... */
-               result = FALSE;
+               result = BoolGetDatum(FALSE);
        }
        else if (fcinfo.argnull[0] || fcinfo.argnull[1])
        {
-               /* One is NULL? Then is distinct... */
-               result = TRUE;
+               /* Only one is NULL? Then is distinct... */
+               result = BoolGetDatum(TRUE);
        }
        else
        {
                fcinfo.isnull = false;
                result = FunctionCallInvoke(&fcinfo);
                *isNull = fcinfo.isnull;
-
-               result = (!DatumGetBool(result));
+               /* Must invert result of "=" */
+               result = BoolGetDatum(!DatumGetBool(result));
        }
 
-       return BoolGetDatum(result);
+       return result;
 }
 
 /* ----------------------------------------------------------------
index 4239d9c3c12cef8dfccba4ac5f623df92167c74c..3db73e87f0bf00555b7b3b929be76172098ebf0e 100644 (file)
@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/setrefs.c,v 1.82 2002/11/19 23:21:59 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/setrefs.c,v 1.83 2002/11/30 21:25:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -575,7 +575,13 @@ fix_opids_walker(Node *node, void *context)
 {
        if (node == NULL)
                return false;
-       if (is_opclause(node))
-               replace_opid((Oper *) ((Expr *) node)->oper);
+       if (IsA(node, Expr))
+       {
+               Expr   *expr = (Expr *) node;
+
+               if (expr->opType == OP_EXPR ||
+                       expr->opType == DISTINCT_EXPR)
+                       replace_opid((Oper *) expr->oper);
+       }
        return expression_tree_walker(node, fix_opids_walker, context);
 }
index 8ba8483a9573d536357faaef3f24379e18293551..14705fadd8e63cb0fbc899d7d31c54ee76e820e2 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.113 2002/11/26 03:01:58 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.114 2002/11/30 21:25:04 tgl Exp $
  *
  * HISTORY
  *       AUTHOR                        DATE                    MAJOR EVENT
@@ -98,25 +98,19 @@ make_clause(int type, Node *oper, List *args)
  *
  * Returns t iff the clause is an operator clause:
  *                             (op expr expr) or (op expr).
- *
- * [historical note: is_clause has the exact functionality and is used
- *             throughout the code. They're renamed to is_opclause for clarity.
- *                                                                                             - ay 10/94.]
  */
 bool
 is_opclause(Node *clause)
 {
        return (clause != NULL &&
                        IsA(clause, Expr) &&
-                       ((((Expr *) clause)->opType == OP_EXPR) ||
-                        ((Expr *) clause)->opType == DISTINCT_EXPR));
+                       ((Expr *) clause)->opType == OP_EXPR);
 }
 
 /*
  * make_opclause
- *       Creates a clause given its operator left operand and right
- *       operand (if it is non-null).
- *
+ *       Creates a clause given its operator, left operand, and right
+ *       operand (pass NULL to create single-operand clause).
  */
 Expr *
 make_opclause(Oper *op, Var *leftop, Var *rightop)
@@ -1191,10 +1185,10 @@ eval_const_expressions_mutator(Node *node, void *context)
                                        bool            has_nonconst_input = false;
 
                                        /*
-                                        * Check for constant inputs and especially
-                                        * constant-NULL inputs.
+                                        * We must do our own check for NULLs because
+                                        * DISTINCT_EXPR has different results for NULL input
+                                        * than the underlying operator does.
                                         */
-                                       Assert(length(args) == 2);
                                        foreach(arg, args)
                                        {
                                                if (IsA(lfirst(arg), Const))
@@ -1205,82 +1199,28 @@ eval_const_expressions_mutator(Node *node, void *context)
                                                else
                                                        has_nonconst_input = true;
                                        }
-                                       /* all nulls? then not distinct */
-                                       if (all_null_input)
-                                               return MAKEBOOLCONST(false, false);
 
-                                       /* one null? then distinct */
-                                       if (has_null_input)
-                                               return MAKEBOOLCONST(true, false);
-
-                                       /* all constants? then optimize this out */
+                                       /* all constants? then can optimize this out */
                                        if (!has_nonconst_input)
                                        {
-                                               Oid                     result_typeid;
-                                               int16           resultTypLen;
-                                               bool            resultTypByVal;
-                                               ExprContext *econtext;
-                                               Datum           const_val;
-                                               bool            const_is_null;
-
-                                               Oper       *oper = (Oper *) expr->oper;
-
-                                               replace_opid(oper);             /* OK to scribble on input
-                                                                                                * to this extent */
-                                               result_typeid = oper->opresulttype;
-
-                                               /*
-                                                * OK, looks like we can simplify this
-                                                * operator/function.
-                                                *
-                                                * We use the executor's routine ExecEvalExpr() to
-                                                * avoid duplication of code and ensure we get the
-                                                * same result as the executor would get.
-                                                *
-                                                * Build a new Expr node containing the
-                                                * already-simplified arguments. The only other
-                                                * setup needed here is the replace_opid() that we
-                                                * already did for the OP_EXPR case.
-                                                */
-                                               newexpr = makeNode(Expr);
-                                               newexpr->typeOid = expr->typeOid;
-                                               newexpr->opType = expr->opType;
-                                               newexpr->oper = expr->oper;
-                                               newexpr->args = args;
-
-                                               /* Get info needed about result datatype */
-                                               get_typlenbyval(result_typeid, &resultTypLen, &resultTypByVal);
-
-                                               /*
-                                                * It is OK to pass a dummy econtext because none
-                                                * of the ExecEvalExpr() code used in this
-                                                * situation will use econtext.  That might seem
-                                                * fortuitous, but it's not so unreasonable --- a
-                                                * constant expression does not depend on context,
-                                                * by definition, n'est ce pas?
-                                                */
-                                               econtext = MakeExprContext(NULL, CurrentMemoryContext);
-
-                                               const_val = ExecEvalExprSwitchContext((Node *) newexpr, econtext,
-                                                                                                  &const_is_null, NULL);
-
-                                               /*
-                                                * Must copy result out of sub-context used by
-                                                * expression eval
-                                                */
-                                               if (!const_is_null)
-                                                       const_val = datumCopy(const_val, resultTypByVal, resultTypLen);
-
-                                               FreeExprContext(econtext);
-                                               pfree(newexpr);
-
-                                               /*
-                                                * Make the constant result node.
-                                                */
-                                               return (Node *) makeConst(result_typeid, resultTypLen,
-                                                                                                 const_val, const_is_null,
-                                                                                                 resultTypByVal);
+                                               /* all nulls? then not distinct */
+                                               if (all_null_input)
+                                                       return MAKEBOOLCONST(false, false);
+
+                                               /* one null? then distinct */
+                                               if (has_null_input)
+                                                       return MAKEBOOLCONST(true, false);
+
+                                               /* otherwise try to evaluate the '=' operator */
+                                               newexpr = simplify_op_or_func(expr, args);
+                                               if (newexpr)    /* successfully simplified it */
+                                                       return (Node *) newexpr;
                                        }
+
+                                       /*
+                                        * else fall out to build new Expr node with simplified
+                                        * args
+                                        */
                                        break;
                                }
                        case OR_EXPR:
@@ -1624,7 +1564,14 @@ simplify_op_or_func(Expr *expr, List *args)
         * Note we take the result type from the Oper or Func node, not the
         * pg_proc tuple; probably necessary for binary-compatibility cases.
         */
-       if (expr->opType == OP_EXPR)
+       if (expr->opType == FUNC_EXPR)
+       {
+               Func       *func = (Func *) expr->oper;
+
+               funcid = func->funcid;
+               result_typeid = func->funcresulttype;
+       }
+       else                                            /* OP_EXPR or DISTINCT_EXPR */
        {
                Oper       *oper = (Oper *) expr->oper;
 
@@ -1632,13 +1579,6 @@ simplify_op_or_func(Expr *expr, List *args)
                funcid = oper->opid;
                result_typeid = oper->opresulttype;
        }
-       else
-       {
-               Func       *func = (Func *) expr->oper;
-
-               funcid = func->funcid;
-               result_typeid = func->funcresulttype;
-       }
 
        /*
         * we could use func_volatile() here, but we need several fields out
@@ -1693,7 +1633,7 @@ simplify_op_or_func(Expr *expr, List *args)
         *
         * Build a new Expr node containing the already-simplified arguments. The
         * only other setup needed here is the replace_opid() that we already
-        * did for the OP_EXPR case.
+        * did for the OP_EXPR/DISTINCT_EXPR case.
         */
        newexpr = makeNode(Expr);
        newexpr->typeOid = expr->typeOid;
index fd2cd4f34b3427da83ff319678d523d1e2776088..b620c7116bc0c56813c7e680ec7ec18605db99df 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/parser/parse_expr.c,v 1.131 2002/11/26 03:01:58 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/parser/parse_expr.c,v 1.132 2002/11/30 21:25:05 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -277,6 +277,8 @@ transformExpr(ParseState *pstate, Node *expr, ConstraintTestValue *domVal)
                                                        result = (Node *) make_op(a->name,
                                                                                                          lexpr,
                                                                                                          rexpr);
+                                                       if (((Expr *) result)->typeOid != BOOLOID)
+                                                               elog(ERROR, "IS DISTINCT FROM requires = operator to yield boolean");
                                                        ((Expr *) result)->opType = DISTINCT_EXPR;
                                                }
                                                break;
index 089f7362a6b42a3e6163d5d630ec4f54812bcea0..11e74a8d92b83b4205ca34975b12af4866cf3e4d 100644 (file)
@@ -10,7 +10,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: primnodes.h,v 1.70 2002/11/26 03:01:59 tgl Exp $
+ * $Id: primnodes.h,v 1.71 2002/11/30 21:25:06 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -165,6 +165,12 @@ typedef enum CoercionForm
 
 /*
  * Expr
+ *
+ * Note: DISTINCT_EXPR implements the "x IS DISTINCT FROM y" construct.
+ * This is similar to an OP_EXPR, except for its handling of NULL inputs.
+ * The oper field is always an Oper node for the "=" operator for x and y.
+ * (We use "=", not the more obvious "<>", because more datatypes have "="
+ * than "<>".  This means the executor must invert the operator result.)
  */
 typedef enum OpType
 {
@@ -183,7 +189,7 @@ typedef struct Expr
 } Expr;
 
 /*
- * Oper - Expr subnode for an OP_EXPR
+ * Oper - Expr subnode for an OP_EXPR (or DISTINCT_EXPR)
  *
  * NOTE: in the good old days 'opno' used to be both (or either, or
  * neither) the pg_operator oid, and/or the pg_proc oid depending
index 549264107faf2e7291d34cad3fbb29365612a8c5..2bdf24116c9b07489036535106cc20fa59ad8385 100644 (file)
@@ -3,7 +3,7 @@
  *                       procedural language
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.70 2002/11/23 03:59:09 momjian Exp $
+ *       $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.71 2002/11/30 21:25:08 tgl Exp $
  *
  *       This software is copyrighted by Jan Wieck - Hamburg.
  *
@@ -3509,6 +3509,7 @@ exec_simple_check_node(Node *node)
                                switch (expr->opType)
                                {
                                        case OP_EXPR:
+                                       case DISTINCT_EXPR:
                                        case FUNC_EXPR:
                                        case OR_EXPR:
                                        case AND_EXPR: