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

Commit4a4e244

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 parentfedabe1 commit4a4e244

File tree

11 files changed

+127
-49
lines changed

11 files changed

+127
-49
lines changed

‎src/backend/catalog/partition.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3204,12 +3204,14 @@ get_proposed_default_constraint(List *new_part_constraints)
32043204
defPartConstraint=makeBoolExpr(NOT_EXPR,
32053205
list_make1(defPartConstraint),
32063206
-1);
3207+
3208+
/* Simplify, to put the negated expression into canonical form */
32073209
defPartConstraint=
32083210
(Expr*)eval_const_expressions(NULL,
32093211
(Node*)defPartConstraint);
3210-
defPartConstraint=canonicalize_qual(defPartConstraint);
3212+
defPartConstraint=canonicalize_qual(defPartConstraint, true);
32113213

3212-
returnlist_make1(defPartConstraint);
3214+
returnmake_ands_implicit(defPartConstraint);
32133215
}
32143216

32153217
/*

‎src/backend/commands/tablecmds.c

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13719,7 +13719,7 @@ PartConstraintImpliedByRelConstraint(Relation scanrel,
1371913719
* fail to detect valid matches without this.
1372013720
*/
1372113721
cexpr=eval_const_expressions(NULL,cexpr);
13722-
cexpr= (Node*)canonicalize_qual((Expr*)cexpr);
13722+
cexpr= (Node*)canonicalize_qual((Expr*)cexpr, true);
1372313723

1372413724
existConstraint=list_concat(existConstraint,
1372513725
make_ands_implicit((Expr*)cexpr));
@@ -14058,10 +14058,18 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
1405814058
/* Skip validation if there are no constraints to validate. */
1405914059
if (partConstraint)
1406014060
{
14061+
/*
14062+
* Run the partition quals through const-simplification similar to
14063+
* check constraints. We skip canonicalize_qual, though, because
14064+
* partition quals should be in canonical form already; also, since
14065+
* the qual is in implicit-AND format, we'd have to explicitly convert
14066+
* it to explicit-AND format and back again.
14067+
*/
1406114068
partConstraint=
1406214069
(List*)eval_const_expressions(NULL,
1406314070
(Node*)partConstraint);
14064-
partConstraint= (List*)canonicalize_qual((Expr*)partConstraint);
14071+
14072+
/* XXX this sure looks wrong */
1406514073
partConstraint=list_make1(make_ands_explicit(partConstraint));
1406614074

1406714075
/*

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -988,7 +988,7 @@ preprocess_expression(PlannerInfo *root, Node *expr, int kind)
988988
*/
989989
if (kind==EXPRKIND_QUAL)
990990
{
991-
expr= (Node*)canonicalize_qual((Expr*)expr);
991+
expr= (Node*)canonicalize_qual((Expr*)expr, false);
992992

993993
#ifdefOPTIMIZER_DEBUG
994994
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
@@ -1740,7 +1740,7 @@ convert_EXISTS_to_ANY(PlannerInfo *root, Query *subselect,
17401740
* subroot.
17411741
*/
17421742
whereClause=eval_const_expressions(root,whereClause);
1743-
whereClause= (Node*)canonicalize_qual((Expr*)whereClause);
1743+
whereClause= (Node*)canonicalize_qual((Expr*)whereClause, false);
17441744
whereClause= (Node*)make_ands_implicit((Expr*)whereClause);
17451745

17461746
/*

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

Lines changed: 52 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,11 @@ negate_clause(Node *node)
269269
* canonicalize_qual
270270
* Convert a qualification expression to the most useful form.
271271
*
272+
* This is primarily intended to be used on top-level WHERE (or JOIN/ON)
273+
* clauses. It can also be used on top-level CHECK constraints, for which
274+
* pass is_check = true. DO NOT call it on any expression that is not known
275+
* to be one or the other, as it might apply inappropriate simplifications.
276+
*
272277
* The name of this routine is a holdover from a time when it would try to
273278
* force the expression into canonical AND-of-ORs or OR-of-ANDs form.
274279
* Eventually, we recognized that that had more theoretical purity than
@@ -283,20 +288,23 @@ negate_clause(Node *node)
283288
* Returns the modified qualification.
284289
*/
285290
Expr*
286-
canonicalize_qual(Expr*qual)
291+
canonicalize_qual(Expr*qual,boolis_check)
287292
{
288293
Expr*newqual;
289294

290295
/* Quick exit for empty qual */
291296
if (qual==NULL)
292297
returnNULL;
293298

299+
/* This should not be invoked on quals in implicit-AND format */
300+
Assert(!IsA(qual,List));
301+
294302
/*
295303
* Pull up redundant subclauses in OR-of-AND trees. We do this only
296304
* within the top-level AND/OR structure; there's no point in looking
297305
* deeper. Also remove any NULL constants in the top-level structure.
298306
*/
299-
newqual=find_duplicate_ors(qual);
307+
newqual=find_duplicate_ors(qual,is_check);
300308

301309
returnnewqual;
302310
}
@@ -395,16 +403,17 @@ pull_ors(List *orlist)
395403
* Only the top-level AND/OR structure is searched.
396404
*
397405
* 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.
406+
* structure, eg in a WHERE clause, "x OR NULL::boolean" is reduced to "x".
407+
* In general that would change the result, so eval_const_expressions can't
408+
* do it; but at top level of WHERE, we don't need to distinguish between
409+
* FALSE and NULL results, so it's valid to treat NULL::boolean the same
410+
* as FALSE and then simplify AND/OR accordingly. Conversely, in a top-level
411+
* CHECK constraint, we may treat a NULL the same as TRUE.
403412
*
404413
* Returns the modified qualification. AND/OR flatness is preserved.
405414
*/
406415
staticExpr*
407-
find_duplicate_ors(Expr*qual)
416+
find_duplicate_ors(Expr*qual,boolis_check)
408417
{
409418
if (or_clause((Node*)qual))
410419
{
@@ -416,18 +425,29 @@ find_duplicate_ors(Expr *qual)
416425
{
417426
Expr*arg= (Expr*)lfirst(temp);
418427

419-
arg=find_duplicate_ors(arg);
428+
arg=find_duplicate_ors(arg,is_check);
420429

421430
/* Get rid of any constant inputs */
422431
if (arg&&IsA(arg,Const))
423432
{
424433
Const*carg= (Const*)arg;
425434

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;
435+
if (is_check)
436+
{
437+
/* Within OR in CHECK, drop constant FALSE */
438+
if (!carg->constisnull&& !DatumGetBool(carg->constvalue))
439+
continue;
440+
/* Constant TRUE or NULL, so OR reduces to TRUE */
441+
return (Expr*)makeBoolConst(true, false);
442+
}
443+
else
444+
{
445+
/* Within OR in WHERE, drop constant FALSE or NULL */
446+
if (carg->constisnull|| !DatumGetBool(carg->constvalue))
447+
continue;
448+
/* Constant TRUE, so OR reduces to TRUE */
449+
returnarg;
450+
}
431451
}
432452

433453
orlist=lappend(orlist,arg);
@@ -449,18 +469,29 @@ find_duplicate_ors(Expr *qual)
449469
{
450470
Expr*arg= (Expr*)lfirst(temp);
451471

452-
arg=find_duplicate_ors(arg);
472+
arg=find_duplicate_ors(arg,is_check);
453473

454474
/* Get rid of any constant inputs */
455475
if (arg&&IsA(arg,Const))
456476
{
457477
Const*carg= (Const*)arg;
458478

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);
479+
if (is_check)
480+
{
481+
/* Within AND in CHECK, drop constant TRUE or NULL */
482+
if (carg->constisnull||DatumGetBool(carg->constvalue))
483+
continue;
484+
/* Constant FALSE, so AND reduces to FALSE */
485+
returnarg;
486+
}
487+
else
488+
{
489+
/* Within AND in WHERE, drop constant TRUE */
490+
if (!carg->constisnull&&DatumGetBool(carg->constvalue))
491+
continue;
492+
/* Constant FALSE or NULL, so AND reduces to FALSE */
493+
return (Expr*)makeBoolConst(false, false);
494+
}
464495
}
465496

466497
andlist=lappend(andlist,arg);

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1209,7 +1209,7 @@ get_relation_constraints(PlannerInfo *root,
12091209
*/
12101210
cexpr=eval_const_expressions(root,cexpr);
12111211

1212-
cexpr= (Node*)canonicalize_qual((Expr*)cexpr);
1212+
cexpr= (Node*)canonicalize_qual((Expr*)cexpr, true);
12131213

12141214
/* Fix Vars to have the desired varno */
12151215
if (varno!=1)
@@ -1262,11 +1262,13 @@ get_relation_constraints(PlannerInfo *root,
12621262
if (pcqual)
12631263
{
12641264
/*
1265-
* Run each expression through const-simplification and
1266-
* canonicalization similar to check constraints.
1265+
* Run the partition quals through const-simplification similar to
1266+
* check constraints. We skip canonicalize_qual, though, because
1267+
* partition quals should be in canonical form already; also, since
1268+
* the qual is in implicit-AND format, we'd have to explicitly convert
1269+
* it to explicit-AND format and back again.
12671270
*/
12681271
pcqual= (List*)eval_const_expressions(root, (Node*)pcqual);
1269-
pcqual= (List*)canonicalize_qual((Expr*)pcqual);
12701272

12711273
/* Fix Vars to have the desired varno */
12721274
if (varno!=1)

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -900,8 +900,9 @@ RelationBuildPartitionKey(Relation relation)
900900
* will be comparing them to similarly-processed qual clause operands,
901901
* and may fail to detect valid matches without this step; fix
902902
* opfuncids while at it. We don't need to bother with
903-
* canonicalize_qual() though, because partition expressions are not
904-
* full-fledged qualification clauses.
903+
* canonicalize_qual() though, because partition expressions should be
904+
* in canonical form already (ie, no need for OR-merging or constant
905+
* elimination).
905906
*/
906907
expr=eval_const_expressions(NULL,expr);
907908
fix_opfuncids(expr);
@@ -4713,12 +4714,11 @@ RelationGetIndexExpressions(Relation relation)
47134714
* Run the expressions through eval_const_expressions. This is not just an
47144715
* optimization, but is necessary, because the planner will be comparing
47154716
* them to similarly-processed qual clauses, and may fail to detect valid
4716-
* matches without this. We don't bother with canonicalize_qual, however.
4717+
* matches without this. We must not use canonicalize_qual, however,
4718+
* since these aren't qual expressions.
47174719
*/
47184720
result= (List*)eval_const_expressions(NULL, (Node*)result);
47194721

4720-
result= (List*)canonicalize_qual((Expr*)result);
4721-
47224722
/* May as well fix opfuncids too */
47234723
fix_opfuncids((Node*)result);
47244724

@@ -4783,7 +4783,7 @@ RelationGetIndexPredicate(Relation relation)
47834783
*/
47844784
result= (List*)eval_const_expressions(NULL, (Node*)result);
47854785

4786-
result= (List*)canonicalize_qual((Expr*)result);
4786+
result= (List*)canonicalize_qual((Expr*)result, false);
47874787

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

‎src/include/optimizer/prep.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ extern Relids get_relids_for_join(PlannerInfo *root, int joinrelid);
3333
* prototypes for prepqual.c
3434
*/
3535
externNode*negate_clause(Node*node);
36-
externExpr*canonicalize_qual(Expr*qual);
36+
externExpr*canonicalize_qual(Expr*qual,boolis_check);
3737

3838
/*
3939
* prototypes for preptlist.c

‎src/test/modules/test_predtest/test_predtest.c

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
#include"funcapi.h"
2020
#include"optimizer/clauses.h"
2121
#include"optimizer/predtest.h"
22-
#include"optimizer/prep.h"
2322
#include"utils/builtins.h"
2423

2524
PG_MODULE_MAGIC;
@@ -137,18 +136,18 @@ test_predtest(PG_FUNCTION_ARGS)
137136

138137
/*
139138
* Because the clauses are in the SELECT list, preprocess_expression did
140-
* not pass them through canonicalize_qual nor make_ands_implicit. We can
141-
* do that here, though, and should do so to match the planner's normal
142-
* usage of the predicate proof functions.
139+
* not pass them through canonicalize_qual nor make_ands_implicit.
143140
*
144-
* This still does not exactly duplicate the normal usage of the proof
145-
* functions, in that they are often given qual clauses containing
146-
* RestrictInfo nodes. But since predtest.c just looks through those
147-
* anyway, it seems OK to not worry about that point.
141+
* We can't do canonicalize_qual here, since it's unclear whether the
142+
* expressions ought to be treated as WHERE or CHECK clauses. Fortunately,
143+
* useful test expressions wouldn't be affected by those transformations
144+
* anyway. We should do make_ands_implicit, though.
145+
*
146+
* Another way in which this does not exactly duplicate the normal usage
147+
* of the proof functions is that they are often given qual clauses
148+
* containing RestrictInfo nodes. But since predtest.c just looks through
149+
* those anyway, it seems OK to not worry about that point.
148150
*/
149-
clause1=canonicalize_qual(clause1);
150-
clause2=canonicalize_qual(clause2);
151-
152151
clause1= (Expr*)make_ands_implicit(clause1);
153152
clause2= (Expr*)make_ands_implicit(clause2);
154153

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1661,6 +1661,30 @@ reset enable_seqscan;
16611661
reset enable_indexscan;
16621662
reset enable_bitmapscan;
16631663
--
1664+
-- Check handling of a constant-null CHECK constraint
1665+
--
1666+
create table cnullparent (f1 int);
1667+
create table cnullchild (check (f1 = 1 or f1 = null)) inherits(cnullparent);
1668+
insert into cnullchild values(1);
1669+
insert into cnullchild values(2);
1670+
insert into cnullchild values(null);
1671+
select * from cnullparent;
1672+
f1
1673+
----
1674+
1
1675+
2
1676+
1677+
(3 rows)
1678+
1679+
select * from cnullparent where f1 = 2;
1680+
f1
1681+
----
1682+
2
1683+
(1 row)
1684+
1685+
drop table cnullparent cascade;
1686+
NOTICE: drop cascades to table cnullchild
1687+
--
16641688
-- Check that constraint exclusion works correctly with partitions using
16651689
-- implicit constraints generated from the partition bound information.
16661690
--

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -611,6 +611,18 @@ reset enable_seqscan;
611611
reset enable_indexscan;
612612
reset enable_bitmapscan;
613613

614+
--
615+
-- Check handling of a constant-null CHECK constraint
616+
--
617+
createtablecnullparent (f1int);
618+
createtablecnullchild (check (f1=1or f1=null)) inherits(cnullparent);
619+
insert into cnullchildvalues(1);
620+
insert into cnullchildvalues(2);
621+
insert into cnullchildvalues(null);
622+
select*from cnullparent;
623+
select*from cnullparentwhere f1=2;
624+
droptable cnullparent cascade;
625+
614626
--
615627
-- Check that constraint exclusion works correctly with partitions using
616628
-- implicit constraints generated from the partition bound information.

0 commit comments

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