From 2b7ec402c41f6112087b1bf2171872d58151cd45 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 30 Nov 2002 21:25:08 +0000 Subject: [PATCH] Code review for IS DISTINCT FROM patch. Fix incorrect constant-folding 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 | 5 +- src/backend/executor/execQual.c | 27 +++--- src/backend/optimizer/plan/setrefs.c | 12 ++- src/backend/optimizer/util/clauses.c | 124 +++++++-------------------- src/backend/parser/parse_expr.c | 4 +- src/include/nodes/primnodes.h | 10 ++- src/pl/plpgsql/src/pl_exec.c | 3 +- 7 files changed, 70 insertions(+), 115 deletions(-) diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index d313ec2253..d159ceba37 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -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; diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c index d41ae779e4..90a38f31ee 100644 --- a/src/backend/executor/execQual.c +++ b/src/backend/executor/execQual.c @@ -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; } /* ---------------------------------------------------------------- diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 4239d9c3c1..3db73e87f0 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -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); } diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 8ba8483a95..14705fadd8 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -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; + /* all nulls? then not distinct */ + if (all_null_input) + return MAKEBOOLCONST(false, false); - Oper *oper = (Oper *) expr->oper; + /* one null? then distinct */ + if (has_null_input) + return MAKEBOOLCONST(true, false); - 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); + /* 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; diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index fd2cd4f34b..b620c7116b 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -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; diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 089f7362a6..11e74a8d92 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -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 diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 549264107f..2bdf24116c 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -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: