Skip to content

Commit 2b7ec40

Browse files
tglsfdctglsfdc
committed
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.
1 parent 36c356e commit 2b7ec40

File tree

7 files changed

+72
-117
lines changed

7 files changed

+72
-117
lines changed

‎src/backend/catalog/dependency.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
1010
* IDENTIFICATION
11-
* $Header: /cvsroot/pgsql/src/backend/catalog/dependency.c,v 1.12 2002/09/22 00:37:09 tgl Exp $
11+
* $Header: /cvsroot/pgsql/src/backend/catalog/dependency.c,v 1.13 2002/11/30 21:25:04 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -803,7 +803,8 @@ find_expr_references_walker(Node *node,
803803
{
804804
Expr *expr = (Expr *) node;
805805

806-
if (expr->opType == OP_EXPR)
806+
if (expr->opType == OP_EXPR ||
807+
expr->opType == DISTINCT_EXPR)
807808
{
808809
Oper *oper = (Oper *) expr->oper;
809810

‎src/backend/executor/execQual.c

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $Header: /cvsroot/pgsql/src/backend/executor/execQual.c,v 1.110 2002/11/25 21:29:35 tgl Exp $
11+
* $Header: /cvsroot/pgsql/src/backend/executor/execQual.c,v 1.111 2002/11/30 21:25:04 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -1144,7 +1144,7 @@ ExecEvalFunc(Expr *funcClause,
11441144
* they are NULL; if either is NULL then the result is already
11451145
* known. If neither is NULL, then proceed to evaluate the
11461146
* function. Note that this is *always* derived from the equals
1147-
* operator, but since we've already evaluated the arguments
1147+
* operator, but since we need special processing of the arguments
11481148
* we can not simply reuse ExecEvalOper() or ExecEvalFunc().
11491149
* ----------------------------------------------------------------
11501150
*/
@@ -1154,18 +1154,15 @@ ExecEvalDistinct(Expr *opClause,
11541154
bool *isNull,
11551155
ExprDoneCond *isDone)
11561156
{
1157-
bool result;
1157+
Datum result;
11581158
FunctionCachePtr fcache;
11591159
FunctionCallInfoData fcinfo;
11601160
ExprDoneCond argDone;
11611161
Oper *op;
11621162
List *argList;
11631163

11641164
/*
1165-
* we extract the oid of the function associated with the op and then
1166-
* pass the work onto ExecMakeFunctionResult which evaluates the
1167-
* arguments and returns the result of calling the function on the
1168-
* evaluated arguments.
1165+
* extract info from opClause
11691166
*/
11701167
op = (Oper *) opClause->oper;
11711168
argList = opClause->args;
@@ -1181,34 +1178,36 @@ ExecEvalDistinct(Expr *opClause,
11811178
econtext->ecxt_per_query_memory);
11821179
op->op_fcache = fcache;
11831180
}
1184-
Assert(fcache->func.fn_retset == FALSE);
1181+
Assert(!fcache->func.fn_retset);
11851182

11861183
/* Need to prep callinfo structure */
11871184
MemSet(&fcinfo, 0, sizeof(fcinfo));
11881185
fcinfo.flinfo = &(fcache->func);
11891186
argDone = ExecEvalFuncArgs(&fcinfo, argList, econtext);
1187+
if (argDone != ExprSingleResult)
1188+
elog(ERROR, "IS DISTINCT FROM does not support set arguments");
11901189
Assert(fcinfo.nargs == 2);
11911190

11921191
if (fcinfo.argnull[0] && fcinfo.argnull[1])
11931192
{
11941193
/* Both NULL? Then is not distinct... */
1195-
result = FALSE;
1194+
result = BoolGetDatum(FALSE);
11961195
}
11971196
else if (fcinfo.argnull[0] || fcinfo.argnull[1])
11981197
{
1199-
/* One is NULL? Then is distinct... */
1200-
result = TRUE;
1198+
/* Only one is NULL? Then is distinct... */
1199+
result = BoolGetDatum(TRUE);
12011200
}
12021201
else
12031202
{
12041203
fcinfo.isnull = false;
12051204
result = FunctionCallInvoke(&fcinfo);
12061205
*isNull = fcinfo.isnull;
1207-
1208-
result = (!DatumGetBool(result));
1206+
/* Must invert result of "=" */
1207+
result = BoolGetDatum(!DatumGetBool(result));
12091208
}
12101209

1211-
return BoolGetDatum(result);
1210+
return result;
12121211
}
12131212

12141213
/* ----------------------------------------------------------------

‎src/backend/optimizer/plan/setrefs.c

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
*
1010
*
1111
* IDENTIFICATION
12-
* $Header: /cvsroot/pgsql/src/backend/optimizer/plan/setrefs.c,v 1.82 2002/11/19 23:21:59 tgl Exp $
12+
* $Header: /cvsroot/pgsql/src/backend/optimizer/plan/setrefs.c,v 1.83 2002/11/30 21:25:04 tgl Exp $
1313
*
1414
*-------------------------------------------------------------------------
1515
*/
@@ -575,7 +575,13 @@ fix_opids_walker(Node *node, void *context)
575575
{
576576
if (node == NULL)
577577
return false;
578-
if (is_opclause(node))
579-
replace_opid((Oper *) ((Expr *) node)->oper);
578+
if (IsA(node, Expr))
579+
{
580+
Expr *expr = (Expr *) node;
581+
582+
if (expr->opType == OP_EXPR ||
583+
expr->opType == DISTINCT_EXPR)
584+
replace_opid((Oper *) expr->oper);
585+
}
580586
return expression_tree_walker(node, fix_opids_walker, context);
581587
}

‎src/backend/optimizer/util/clauses.c

Lines changed: 34 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.113 2002/11/26 03:01:58 tgl Exp $
11+
* $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.114 2002/11/30 21:25:04 tgl Exp $
1212
*
1313
* HISTORY
1414
* AUTHOR DATE MAJOR EVENT
@@ -98,25 +98,19 @@ make_clause(int type, Node *oper, List *args)
9898
*
9999
* Returns t iff the clause is an operator clause:
100100
* (op expr expr) or (op expr).
101-
*
102-
* [historical note: is_clause has the exact functionality and is used
103-
* throughout the code. They're renamed to is_opclause for clarity.
104-
* - ay 10/94.]
105101
*/
106102
bool
107103
is_opclause(Node *clause)
108104
{
109105
return (clause != NULL &&
110106
IsA(clause, Expr) &&
111-
((((Expr *) clause)->opType == OP_EXPR) ||
112-
((Expr *) clause)->opType == DISTINCT_EXPR));
107+
((Expr *) clause)->opType == OP_EXPR);
113108
}
114109

115110
/*
116111
* make_opclause
117-
* Creates a clause given its operator left operand and right
118-
* operand (if it is non-null).
119-
*
112+
* Creates a clause given its operator, left operand, and right
113+
* operand (pass NULL to create single-operand clause).
120114
*/
121115
Expr *
122116
make_opclause(Oper *op, Var *leftop, Var *rightop)
@@ -1191,10 +1185,10 @@ eval_const_expressions_mutator(Node *node, void *context)
11911185
bool has_nonconst_input = false;
11921186

11931187
/*
1194-
* Check for constant inputs and especially
1195-
* constant-NULL inputs.
1188+
* We must do our own check for NULLs because
1189+
* DISTINCT_EXPR has different results for NULL input
1190+
* than the underlying operator does.
11961191
*/
1197-
Assert(length(args) == 2);
11981192
foreach(arg, args)
11991193
{
12001194
if (IsA(lfirst(arg), Const))
@@ -1205,82 +1199,28 @@ eval_const_expressions_mutator(Node *node, void *context)
12051199
else
12061200
has_nonconst_input = true;
12071201
}
1208-
/* all nulls? then not distinct */
1209-
if (all_null_input)
1210-
return MAKEBOOLCONST(false, false);
12111202

1212-
/* one null? then distinct */
1213-
if (has_null_input)
1214-
return MAKEBOOLCONST(true, false);
1215-
1216-
/* all constants? then optimize this out */
1203+
/* all constants? then can optimize this out */
12171204
if (!has_nonconst_input)
12181205
{
1219-
Oid result_typeid;
1220-
int16 resultTypLen;
1221-
bool resultTypByVal;
1222-
ExprContext *econtext;
1223-
Datum const_val;
1224-
bool const_is_null;
1225-
1226-
Oper *oper = (Oper *) expr->oper;
1227-
1228-
replace_opid(oper); /* OK to scribble on input
1229-
* to this extent */
1230-
result_typeid = oper->opresulttype;
1231-
1232-
/*
1233-
* OK, looks like we can simplify this
1234-
* operator/function.
1235-
*
1236-
* We use the executor's routine ExecEvalExpr() to
1237-
* avoid duplication of code and ensure we get the
1238-
* same result as the executor would get.
1239-
*
1240-
* Build a new Expr node containing the
1241-
* already-simplified arguments. The only other
1242-
* setup needed here is the replace_opid() that we
1243-
* already did for the OP_EXPR case.
1244-
*/
1245-
newexpr = makeNode(Expr);
1246-
newexpr->typeOid = expr->typeOid;
1247-
newexpr->opType = expr->opType;
1248-
newexpr->oper = expr->oper;
1249-
newexpr->args = args;
1250-
1251-
/* Get info needed about result datatype */
1252-
get_typlenbyval(result_typeid, &resultTypLen, &resultTypByVal);
1253-
1254-
/*
1255-
* It is OK to pass a dummy econtext because none
1256-
* of the ExecEvalExpr() code used in this
1257-
* situation will use econtext. That might seem
1258-
* fortuitous, but it's not so unreasonable --- a
1259-
* constant expression does not depend on context,
1260-
* by definition, n'est ce pas?
1261-
*/
1262-
econtext = MakeExprContext(NULL, CurrentMemoryContext);
1263-
1264-
const_val = ExecEvalExprSwitchContext((Node *) newexpr, econtext,
1265-
&const_is_null, NULL);
1266-
1267-
/*
1268-
* Must copy result out of sub-context used by
1269-
* expression eval
1270-
*/
1271-
if (!const_is_null)
1272-
const_val = datumCopy(const_val, resultTypByVal, resultTypLen);
1273-
1274-
FreeExprContext(econtext);
1275-
pfree(newexpr);
1276-
1277-
/*
1278-
* Make the constant result node.
1279-
*/
1280-
return (Node *) makeConst(result_typeid, resultTypLen,
1281-
const_val, const_is_null,
1282-
resultTypByVal);
1206+
/* all nulls? then not distinct */
1207+
if (all_null_input)
1208+
return MAKEBOOLCONST(false, false);
1209+
1210+
/* one null? then distinct */
1211+
if (has_null_input)
1212+
return MAKEBOOLCONST(true, false);
1213+
1214+
/* otherwise try to evaluate the '=' operator */
1215+
newexpr = simplify_op_or_func(expr, args);
1216+
if (newexpr) /* successfully simplified it */
1217+
return (Node *) newexpr;
12831218
}
1219+
1220+
/*
1221+
* else fall out to build new Expr node with simplified
1222+
* args
1223+
*/
12841224
break;
12851225
}
12861226
case OR_EXPR:
@@ -1624,21 +1564,21 @@ simplify_op_or_func(Expr *expr, List *args)
16241564
* Note we take the result type from the Oper or Func node, not the
16251565
* pg_proc tuple; probably necessary for binary-compatibility cases.
16261566
*/
1627-
if (expr->opType == OP_EXPR)
1567+
if (expr->opType == FUNC_EXPR)
1568+
{
1569+
Func *func = (Func *) expr->oper;
1570+
1571+
funcid = func->funcid;
1572+
result_typeid = func->funcresulttype;
1573+
}
1574+
else /* OP_EXPR or DISTINCT_EXPR */
16281575
{
16291576
Oper *oper = (Oper *) expr->oper;
16301577

16311578
replace_opid(oper); /* OK to scribble on input to this extent */
16321579
funcid = oper->opid;
16331580
result_typeid = oper->opresulttype;
16341581
}
1635-
else
1636-
{
1637-
Func *func = (Func *) expr->oper;
1638-
1639-
funcid = func->funcid;
1640-
result_typeid = func->funcresulttype;
1641-
}
16421582

16431583
/*
16441584
* we could use func_volatile() here, but we need several fields out
@@ -1693,7 +1633,7 @@ simplify_op_or_func(Expr *expr, List *args)
16931633
*
16941634
* Build a new Expr node containing the already-simplified arguments. The
16951635
* only other setup needed here is the replace_opid() that we already
1696-
* did for the OP_EXPR case.
1636+
* did for the OP_EXPR/DISTINCT_EXPR case.
16971637
*/
16981638
newexpr = makeNode(Expr);
16991639
newexpr->typeOid = expr->typeOid;

‎src/backend/parser/parse_expr.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $Header: /cvsroot/pgsql/src/backend/parser/parse_expr.c,v 1.131 2002/11/26 03:01:58 tgl Exp $
11+
* $Header: /cvsroot/pgsql/src/backend/parser/parse_expr.c,v 1.132 2002/11/30 21:25:05 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -277,6 +277,8 @@ transformExpr(ParseState *pstate, Node *expr, ConstraintTestValue *domVal)
277277
result = (Node *) make_op(a->name,
278278
lexpr,
279279
rexpr);
280+
if (((Expr *) result)->typeOid != BOOLOID)
281+
elog(ERROR, "IS DISTINCT FROM requires = operator to yield boolean");
280282
((Expr *) result)->opType = DISTINCT_EXPR;
281283
}
282284
break;

‎src/include/nodes/primnodes.h

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
* Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
1111
* Portions Copyright (c) 1994, Regents of the University of California
1212
*
13-
* $Id: primnodes.h,v 1.70 2002/11/26 03:01:59 tgl Exp $
13+
* $Id: primnodes.h,v 1.71 2002/11/30 21:25:06 tgl Exp $
1414
*
1515
*-------------------------------------------------------------------------
1616
*/
@@ -165,6 +165,12 @@ typedef enum CoercionForm
165165

166166
/*
167167
* Expr
168+
*
169+
* Note: DISTINCT_EXPR implements the "x IS DISTINCT FROM y" construct.
170+
* This is similar to an OP_EXPR, except for its handling of NULL inputs.
171+
* The oper field is always an Oper node for the "=" operator for x and y.
172+
* (We use "=", not the more obvious "<>", because more datatypes have "="
173+
* than "<>". This means the executor must invert the operator result.)
168174
*/
169175
typedef enum OpType
170176
{
@@ -183,7 +189,7 @@ typedef struct Expr
183189
} Expr;
184190

185191
/*
186-
* Oper - Expr subnode for an OP_EXPR
192+
* Oper - Expr subnode for an OP_EXPR (or DISTINCT_EXPR)
187193
*
188194
* NOTE: in the good old days 'opno' used to be both (or either, or
189195
* neither) the pg_operator oid, and/or the pg_proc oid depending

‎src/pl/plpgsql/src/pl_exec.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* procedural language
44
*
55
* IDENTIFICATION
6-
* $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.70 2002/11/23 03:59:09 momjian Exp $
6+
* $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.71 2002/11/30 21:25:08 tgl Exp $
77
*
88
* This software is copyrighted by Jan Wieck - Hamburg.
99
*
@@ -3509,6 +3509,7 @@ exec_simple_check_node(Node *node)
35093509
switch (expr->opType)
35103510
{
35113511
case OP_EXPR:
3512+
case DISTINCT_EXPR:
35123513
case FUNC_EXPR:
35133514
case OR_EXPR:
35143515
case AND_EXPR:

0 commit comments

Comments
 (0)