Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit976e584

Browse files
committed
Fix improper uses of canonicalize_qual().
One of the things canonicalize_qual() does is to remove constant-NULLsubexpressions of top-level AND/OR clauses. It does that on the assumptionthat what it's given is a top-level WHERE clause, so that NULL can betreated like FALSE. Although this is documented down inside a subroutineof canonicalize_qual(), it wasn't mentioned in the documentation of thatfunction itself, and some callers hadn't gotten that memo.Notably, commitd007a95 caused get_relation_constraints() to applycanonicalize_qual() to CHECK constraints. That allowed constraintexclusion to misoptimize situations in which a CHECK constraint had aprovably-NULL subclause, as seen in the regression test case added here,in which a child table that should be scanned is not. (Although thisthinko is ancient, the test case doesn't fail before 9.2, for reasonsI've not bothered to track down in detail. There may be related casesthat do fail before that.)More recently, commitf0e4475 added an independent bug by applyingcanonicalize_qual() to index expressions, which is even sillier sincethose might not even be boolean. If they are, though, I think thiscould lead to making incorrect index entries for affected indexexpressions in v10. I haven't attempted to prove that though.To fix, add an "is_check" parameter to canonicalize_qual() to specifywhether it should assume WHERE or CHECK semantics, and make it performNULL-elimination accordingly. Adjust the callers to apply the rightsemantics, or remove the call entirely in cases where it's not knownthat the expression has one or the other semantics. I also removedthe call in some cases involving partition expressions, where it shouldbe a no-op because such expressions should be canonical already ...and was a no-op, independently of whether it could in principle havedone something, because it was being handed the qual in implicit-ANDformat which isn't what it expects. In HEAD, add an Assert to catchthat type of mistake in future.This represents an API break for external callers of canonicalize_qual().While that's intentional in HEAD to make such callers think about whichcase applies to them, it seems like something we probably wouldn't bethanked for in released branches. Hence, in released branches, theextra parameter is added to a new function canonicalize_qual_ext(),and canonicalize_qual() is a wrapper that retains its old behavior.Patch by me with suggestions from Dean Rasheed. Back-patch to allsupported branches.Discussion:https://postgr.es/m/24475.1520635069@sss.pgh.pa.us
1 parent2f09dc1 commit976e584

File tree

8 files changed

+105
-26
lines changed

8 files changed

+105
-26
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -836,7 +836,7 @@ preprocess_expression(PlannerInfo *root, Node *expr, int kind)
836836
*/
837837
if (kind==EXPRKIND_QUAL)
838838
{
839-
expr= (Node*)canonicalize_qual((Expr*)expr);
839+
expr= (Node*)canonicalize_qual_ext((Expr*)expr, false);
840840

841841
#ifdefOPTIMIZER_DEBUG
842842
printf("After canonicalize_qual()\n");

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1721,7 +1721,7 @@ convert_EXISTS_to_ANY(PlannerInfo *root, Query *subselect,
17211721
* subroot.
17221722
*/
17231723
whereClause=eval_const_expressions(root,whereClause);
1724-
whereClause= (Node*)canonicalize_qual((Expr*)whereClause);
1724+
whereClause= (Node*)canonicalize_qual_ext((Expr*)whereClause, false);
17251725
whereClause= (Node*)make_ands_implicit((Expr*)whereClause);
17261726

17271727
/*

‎src/backend/optimizer/prep/prepqual.c

Lines changed: 62 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939

4040
staticList*pull_ands(List*andlist);
4141
staticList*pull_ors(List*orlist);
42-
staticExpr*find_duplicate_ors(Expr*qual);
42+
staticExpr*find_duplicate_ors(Expr*qual,boolis_check);
4343
staticExpr*process_duplicate_ors(List*orlist);
4444

4545

@@ -269,6 +269,24 @@ negate_clause(Node *node)
269269
* canonicalize_qual
270270
* Convert a qualification expression to the most useful form.
271271
*
272+
* Backwards-compatibility wrapper for use by external code that hasn't
273+
* been updated.
274+
*/
275+
Expr*
276+
canonicalize_qual(Expr*qual)
277+
{
278+
returncanonicalize_qual_ext(qual, false);
279+
}
280+
281+
/*
282+
* canonicalize_qual_ext
283+
* Convert a qualification expression to the most useful form.
284+
*
285+
* This is primarily intended to be used on top-level WHERE (or JOIN/ON)
286+
* clauses. It can also be used on top-level CHECK constraints, for which
287+
* pass is_check = true. DO NOT call it on any expression that is not known
288+
* to be one or the other, as it might apply inappropriate simplifications.
289+
*
272290
* The name of this routine is a holdover from a time when it would try to
273291
* force the expression into canonical AND-of-ORs or OR-of-ANDs form.
274292
* Eventually, we recognized that that had more theoretical purity than
@@ -283,7 +301,7 @@ negate_clause(Node *node)
283301
* Returns the modified qualification.
284302
*/
285303
Expr*
286-
canonicalize_qual(Expr*qual)
304+
canonicalize_qual_ext(Expr*qual,boolis_check)
287305
{
288306
Expr*newqual;
289307

@@ -296,7 +314,7 @@ canonicalize_qual(Expr *qual)
296314
* within the top-level AND/OR structure; there's no point in looking
297315
* deeper. Also remove any NULL constants in the top-level structure.
298316
*/
299-
newqual=find_duplicate_ors(qual);
317+
newqual=find_duplicate_ors(qual,is_check);
300318

301319
returnnewqual;
302320
}
@@ -395,16 +413,17 @@ pull_ors(List *orlist)
395413
* Only the top-level AND/OR structure is searched.
396414
*
397415
* While at it, we remove any NULL constants within the top-level AND/OR
398-
* structure, eg "x OR NULL::boolean" is reduced to "x". In general that
399-
* would change the result, so eval_const_expressions can't do it; but at
400-
* top level of WHERE, we don't need to distinguish between FALSE and NULL
401-
* results, so it's valid to treat NULL::boolean the same as FALSE and then
402-
* simplify AND/OR accordingly.
416+
* structure, eg in a WHERE clause, "x OR NULL::boolean" is reduced to "x".
417+
* In general that would change the result, so eval_const_expressions can't
418+
* do it; but at top level of WHERE, we don't need to distinguish between
419+
* FALSE and NULL results, so it's valid to treat NULL::boolean the same
420+
* as FALSE and then simplify AND/OR accordingly. Conversely, in a top-level
421+
* CHECK constraint, we may treat a NULL the same as TRUE.
403422
*
404423
* Returns the modified qualification. AND/OR flatness is preserved.
405424
*/
406425
staticExpr*
407-
find_duplicate_ors(Expr*qual)
426+
find_duplicate_ors(Expr*qual,boolis_check)
408427
{
409428
if (or_clause((Node*)qual))
410429
{
@@ -416,18 +435,29 @@ find_duplicate_ors(Expr *qual)
416435
{
417436
Expr*arg= (Expr*)lfirst(temp);
418437

419-
arg=find_duplicate_ors(arg);
438+
arg=find_duplicate_ors(arg,is_check);
420439

421440
/* Get rid of any constant inputs */
422441
if (arg&&IsA(arg,Const))
423442
{
424443
Const*carg= (Const*)arg;
425444

426-
/* Drop constant FALSE or NULL */
427-
if (carg->constisnull|| !DatumGetBool(carg->constvalue))
428-
continue;
429-
/* constant TRUE, so OR reduces to TRUE */
430-
returnarg;
445+
if (is_check)
446+
{
447+
/* Within OR in CHECK, drop constant FALSE */
448+
if (!carg->constisnull&& !DatumGetBool(carg->constvalue))
449+
continue;
450+
/* Constant TRUE or NULL, so OR reduces to TRUE */
451+
return (Expr*)makeBoolConst(true, false);
452+
}
453+
else
454+
{
455+
/* Within OR in WHERE, drop constant FALSE or NULL */
456+
if (carg->constisnull|| !DatumGetBool(carg->constvalue))
457+
continue;
458+
/* Constant TRUE, so OR reduces to TRUE */
459+
returnarg;
460+
}
431461
}
432462

433463
orlist=lappend(orlist,arg);
@@ -449,18 +479,29 @@ find_duplicate_ors(Expr *qual)
449479
{
450480
Expr*arg= (Expr*)lfirst(temp);
451481

452-
arg=find_duplicate_ors(arg);
482+
arg=find_duplicate_ors(arg,is_check);
453483

454484
/* Get rid of any constant inputs */
455485
if (arg&&IsA(arg,Const))
456486
{
457487
Const*carg= (Const*)arg;
458488

459-
/* Drop constant TRUE */
460-
if (!carg->constisnull&&DatumGetBool(carg->constvalue))
461-
continue;
462-
/* constant FALSE or NULL, so AND reduces to FALSE */
463-
return (Expr*)makeBoolConst(false, false);
489+
if (is_check)
490+
{
491+
/* Within AND in CHECK, drop constant TRUE or NULL */
492+
if (carg->constisnull||DatumGetBool(carg->constvalue))
493+
continue;
494+
/* Constant FALSE, so AND reduces to FALSE */
495+
returnarg;
496+
}
497+
else
498+
{
499+
/* Within AND in WHERE, drop constant TRUE */
500+
if (!carg->constisnull&&DatumGetBool(carg->constvalue))
501+
continue;
502+
/* Constant FALSE or NULL, so AND reduces to FALSE */
503+
return (Expr*)makeBoolConst(false, false);
504+
}
464505
}
465506

466507
andlist=lappend(andlist,arg);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1177,7 +1177,7 @@ get_relation_constraints(PlannerInfo *root,
11771177
*/
11781178
cexpr=eval_const_expressions(root,cexpr);
11791179

1180-
cexpr= (Node*)canonicalize_qual((Expr*)cexpr);
1180+
cexpr= (Node*)canonicalize_qual_ext((Expr*)cexpr, true);
11811181

11821182
/* Fix Vars to have the desired varno */
11831183
if (varno!=1)

‎src/backend/utils/cache/relcache.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4263,7 +4263,8 @@ RelationGetIndexExpressions(Relation relation)
42634263
* Run the expressions through eval_const_expressions. This is not just an
42644264
* optimization, but is necessary, because the planner will be comparing
42654265
* them to similarly-processed qual clauses, and may fail to detect valid
4266-
* matches without this. We don't bother with canonicalize_qual, however.
4266+
* matches without this. We must not use canonicalize_qual, however,
4267+
* since these aren't qual expressions.
42674268
*/
42684269
result= (List*)eval_const_expressions(NULL, (Node*)result);
42694270

@@ -4331,7 +4332,7 @@ RelationGetIndexPredicate(Relation relation)
43314332
*/
43324333
result= (List*)eval_const_expressions(NULL, (Node*)result);
43334334

4334-
result= (List*)canonicalize_qual((Expr*)result);
4335+
result= (List*)canonicalize_qual_ext((Expr*)result, false);
43354336

43364337
/* Also convert to implicit-AND format */
43374338
result=make_ands_implicit((Expr*)result);

‎src/include/optimizer/prep.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ extern Relids get_relids_for_join(PlannerInfo *root, int joinrelid);
3434
*/
3535
externNode*negate_clause(Node*node);
3636
externExpr*canonicalize_qual(Expr*qual);
37+
externExpr*canonicalize_qual_ext(Expr*qual,boolis_check);
3738

3839
/*
3940
* prototypes for prepsecurity.c

‎src/test/regress/expected/inherit.out

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1591,3 +1591,27 @@ FROM generate_series(1, 3) g(i);
15911591
reset enable_seqscan;
15921592
reset enable_indexscan;
15931593
reset enable_bitmapscan;
1594+
--
1595+
-- Check handling of a constant-null CHECK constraint
1596+
--
1597+
create table cnullparent (f1 int);
1598+
create table cnullchild (check (f1 = 1 or f1 = null)) inherits(cnullparent);
1599+
insert into cnullchild values(1);
1600+
insert into cnullchild values(2);
1601+
insert into cnullchild values(null);
1602+
select * from cnullparent;
1603+
f1
1604+
----
1605+
1
1606+
2
1607+
1608+
(3 rows)
1609+
1610+
select * from cnullparent where f1 = 2;
1611+
f1
1612+
----
1613+
2
1614+
(1 row)
1615+
1616+
drop table cnullparent cascade;
1617+
NOTICE: drop cascades to table cnullchild

‎src/test/regress/sql/inherit.sql

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -562,3 +562,15 @@ FROM generate_series(1, 3) g(i);
562562
reset enable_seqscan;
563563
reset enable_indexscan;
564564
reset enable_bitmapscan;
565+
566+
--
567+
-- Check handling of a constant-null CHECK constraint
568+
--
569+
createtablecnullparent (f1int);
570+
createtablecnullchild (check (f1=1or f1=null)) inherits(cnullparent);
571+
insert into cnullchildvalues(1);
572+
insert into cnullchildvalues(2);
573+
insert into cnullchildvalues(null);
574+
select*from cnullparent;
575+
select*from cnullparentwhere f1=2;
576+
droptable cnullparent cascade;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp