From 3e96cdc5cad955afd79e59fb23056d23dc37811c Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Thu, 11 Apr 2024 13:56:05 +0800 Subject: [PATCH v4] Fix NOT NULL optimization for inheritance tables --- src/backend/optimizer/plan/initsplan.c | 55 ++++++++++++++++--------- src/backend/optimizer/util/inherit.c | 36 ++++++++++------ src/backend/optimizer/util/plancat.c | 37 +++++++++++------ src/backend/optimizer/util/relnode.c | 23 +++++++---- src/include/nodes/pathnodes.h | 6 ++- src/test/regress/expected/predicate.out | 48 +++++++++++++++++++++ src/test/regress/sql/predicate.sql | 27 ++++++++++++ 7 files changed, 178 insertions(+), 54 deletions(-) diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index d3868b628d..92947b4096 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -14,6 +14,7 @@ */ #include "postgres.h" +#include "catalog/pg_class.h" #include "catalog/pg_type.h" #include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" @@ -2629,32 +2630,46 @@ add_base_clause_to_rel(PlannerInfo *root, Index relid, RestrictInfo *restrictinfo) { RelOptInfo *rel = find_base_rel(root, relid); + RangeTblEntry *rte = root->simple_rte_array[relid]; Assert(bms_membership(restrictinfo->required_relids) == BMS_SINGLETON); - /* Don't add the clause if it is always true */ - if (restriction_is_always_true(root, restrictinfo)) - return; - /* - * Substitute the origin qual with constant-FALSE if it is provably always - * false. Note that we keep the same rinfo_serial. + * For inheritance parent tables, we must always record the + * RestrictInfo in baserestrictinfo as is. If we were to transform or + * skip adding it, then the original wouldn't be available in + * apply_child_basequals. Since there are two RangeTblEntries for + * inheritance parents, one with inh==true and the other with inh==false, + * we're still able to apply this optimization to the inh==false one. The + * inh==true one is what apply_child_basequals() sees, whereas the + * inh==false one is what's used for the scan node in the final plan. */ - if (restriction_is_always_false(root, restrictinfo)) + if (!rte->inh || rte->relkind == RELKIND_PARTITIONED_TABLE) { - int save_rinfo_serial = restrictinfo->rinfo_serial; - - restrictinfo = make_restrictinfo(root, - (Expr *) makeBoolConst(false, false), - restrictinfo->is_pushed_down, - restrictinfo->has_clone, - restrictinfo->is_clone, - restrictinfo->pseudoconstant, - 0, /* security_level */ - restrictinfo->required_relids, - restrictinfo->incompatible_relids, - restrictinfo->outer_relids); - restrictinfo->rinfo_serial = save_rinfo_serial; + /* Don't add the clause if it is always true */ + if (restriction_is_always_true(root, restrictinfo)) + return; + + /* + * Substitute the origin qual with constant-FALSE if it is provably always + * false. Note that we keep the same rinfo_serial. + */ + if (restriction_is_always_false(root, restrictinfo)) + { + int save_rinfo_serial = restrictinfo->rinfo_serial; + + restrictinfo = make_restrictinfo(root, + (Expr *) makeBoolConst(false, false), + restrictinfo->is_pushed_down, + restrictinfo->has_clone, + restrictinfo->is_clone, + restrictinfo->pseudoconstant, + 0, /* security_level */ + restrictinfo->required_relids, + restrictinfo->incompatible_relids, + restrictinfo->outer_relids); + restrictinfo->rinfo_serial = save_rinfo_serial; + } } /* Add clause to rel's restriction list */ diff --git a/src/backend/optimizer/util/inherit.c b/src/backend/optimizer/util/inherit.c index 5c7acf8a90..4794ad5999 100644 --- a/src/backend/optimizer/util/inherit.c +++ b/src/backend/optimizer/util/inherit.c @@ -824,9 +824,12 @@ expand_appendrel_subquery(PlannerInfo *root, RelOptInfo *rel, * Populate childrel's base restriction quals from parent rel's quals, * translating them using appinfo. * - * If any of the resulting clauses evaluate to constant false or NULL, we - * return false and don't apply any quals. Caller should mark the relation as - * a dummy rel in this case, since it doesn't need to be scanned. + * If any of the resulting clauses evaluate to constant false or NULL, or are + * proven always false, we return false and don't apply any quals. Caller + * should mark the relation as a dummy rel in this case, since it doesn't need + * to be scanned. + * + * For resulting clauses that are proven always true, we just drop them. */ bool apply_child_basequals(PlannerInfo *root, RelOptInfo *parentrel, @@ -875,6 +878,7 @@ apply_child_basequals(PlannerInfo *root, RelOptInfo *parentrel, { Node *onecq = (Node *) lfirst(lc2); bool pseudoconstant; + RestrictInfo *childrinfo; /* check for pseudoconstant (no Vars or volatile functions) */ pseudoconstant = @@ -886,15 +890,23 @@ apply_child_basequals(PlannerInfo *root, RelOptInfo *parentrel, root->hasPseudoConstantQuals = true; } /* reconstitute RestrictInfo with appropriate properties */ - childquals = lappend(childquals, - make_restrictinfo(root, - (Expr *) onecq, - rinfo->is_pushed_down, - rinfo->has_clone, - rinfo->is_clone, - pseudoconstant, - rinfo->security_level, - NULL, NULL, NULL)); + childrinfo = make_restrictinfo(root, + (Expr *) onecq, + rinfo->is_pushed_down, + rinfo->has_clone, + rinfo->is_clone, + pseudoconstant, + rinfo->security_level, + NULL, NULL, NULL); + + /* Restriction is proven always false */ + if (restriction_is_always_false(root, childrinfo)) + return false; + /* Restriction is proven always true, so drop it */ + if (restriction_is_always_true(root, childrinfo)) + continue; + + childquals = lappend(childquals, childrinfo); /* track minimum security level among child quals */ cq_min_security = Min(cq_min_security, rinfo->security_level); } diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 6bb53e4346..130f838629 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -161,22 +161,33 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent, rel->attr_widths = (int32 *) palloc0((rel->max_attr - rel->min_attr + 1) * sizeof(int32)); - /* record which columns are defined as NOT NULL */ - for (int i = 0; i < relation->rd_att->natts; i++) + /* + * Record which columns are defined as NOT NULL. We leave this + * unpopulated for non-partitioned inheritance parent relations as it's + * ambiguous as to what it means. Some child tables may have a NOT NULL + * constraint for a column while others may not. We could work harder and + * build a unioned set of all child relations notnullattnums, but there's + * currently no need. The RelOptInfo corresponding to the !inh + * RangeTblEntry does get populated. + */ + if (!inhparent || relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) { - FormData_pg_attribute *attr = &relation->rd_att->attrs[i]; - - if (attr->attnotnull) + for (int i = 0; i < relation->rd_att->natts; i++) { - rel->notnullattnums = bms_add_member(rel->notnullattnums, - attr->attnum); + FormData_pg_attribute *attr = &relation->rd_att->attrs[i]; - /* - * Per RemoveAttributeById(), dropped columns will have their - * attnotnull unset, so we needn't check for dropped columns in - * the above condition. - */ - Assert(!attr->attisdropped); + if (attr->attnotnull) + { + rel->notnullattnums = bms_add_member(rel->notnullattnums, + attr->attnum); + + /* + * Per RemoveAttributeById(), dropped columns will have their + * attnotnull unset, so we needn't check for dropped columns + * in the above condition. + */ + Assert(!attr->attisdropped); + } } } diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c index d791c4108d..427a32bde5 100644 --- a/src/backend/optimizer/util/relnode.c +++ b/src/backend/optimizer/util/relnode.c @@ -372,11 +372,20 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent) break; } + /* + * We must apply the partially filled in RelOptInfo before calling + * apply_child_basequals due to some transformations within that function + * which require the RelOptInfo to be available in the simple_rel_array. + */ + root->simple_rel_array[relid] = rel; + /* * Copy the parent's quals to the child, with appropriate substitution of - * variables. If any constant false or NULL clauses turn up, we can mark - * the child as dummy right away. (We must do this immediately so that - * pruning works correctly when recursing in expand_partitioned_rtentry.) + * variables. If there are any resulting clauses that are constant false + * or NULL, or proven always false, we can mark the child as dummy right + * away. (We must do this immediately so that pruning works correctly when + * recursing in expand_partitioned_rtentry.) For resulting clauses that + * are proven always true, we just drop them. */ if (parent) { @@ -386,16 +395,14 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent) if (!apply_child_basequals(root, parent, rel, rte, appinfo)) { /* - * Some restriction clause reduced to constant FALSE or NULL after - * substitution, so this child need not be scanned. + * Some restriction clause reduced to constant FALSE or NULL, or + * was proven always false after substitution, so this child need + * not be scanned. */ mark_dummy_rel(rel); } } - /* Save the finished struct in the query's simple_rel_array */ - root->simple_rel_array[relid] = rel; - return rel; } diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 0ab25d9ce7..6c71098f2d 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -918,7 +918,11 @@ typedef struct RelOptInfo Relids *attr_needed pg_node_attr(read_write_ignore); /* array indexed [min_attr .. max_attr] */ int32 *attr_widths pg_node_attr(read_write_ignore); - /* zero-based set containing attnums of NOT NULL columns */ + + /* + * Zero-based set containing attnums of NOT NULL columns. Not populated + * for rels corresponding to non-partitioned inh==true RTEs. + */ Bitmapset *notnullattnums; /* relids of outer joins that can null this baserel */ Relids nulling_relids; diff --git a/src/test/regress/expected/predicate.out b/src/test/regress/expected/predicate.out index 60bf3e37aa..57511718d1 100644 --- a/src/test/regress/expected/predicate.out +++ b/src/test/regress/expected/predicate.out @@ -242,3 +242,51 @@ SELECT * FROM pred_tab t1 (9 rows) DROP TABLE pred_tab; +-- Validate we handle IS NULL and IS NOT NULL quals correctly with inheritance +-- parents. +CREATE TABLE pred_parent (a int, b int); +CREATE TABLE pred_child () INHERITS (pred_parent); +ALTER TABLE ONLY pred_parent ALTER a SET NOT NULL; +-- Ensure that the scan on pred_child contains the IS NOT NULL qual. +EXPLAIN (COSTS OFF) +SELECT * FROM pred_parent WHERE a IS NOT NULL; + QUERY PLAN +--------------------------------------------- + Append + -> Seq Scan on pred_parent pred_parent_1 + -> Seq Scan on pred_child pred_parent_2 + Filter: (a IS NOT NULL) +(4 rows) + +-- Ensure we only scan pred_child and not pred_parent +EXPLAIN (COSTS OFF) +SELECT * FROM pred_parent WHERE a IS NULL; + QUERY PLAN +------------------------------------ + Seq Scan on pred_child pred_parent + Filter: (a IS NULL) +(2 rows) + +ALTER TABLE pred_parent ALTER a DROP NOT NULL; +ALTER TABLE pred_child ALTER a SET NOT NULL; +-- Ensure the IS NOT NULL qual is removed from the pred_child scan. +EXPLAIN (COSTS OFF) +SELECT * FROM pred_parent WHERE a IS NOT NULL; + QUERY PLAN +--------------------------------------------- + Append + -> Seq Scan on pred_parent pred_parent_1 + Filter: (a IS NOT NULL) + -> Seq Scan on pred_child pred_parent_2 +(4 rows) + +-- Ensure we only scan pred_parent and not pred_child +EXPLAIN (COSTS OFF) +SELECT * FROM pred_parent WHERE a IS NULL; + QUERY PLAN +------------------------- + Seq Scan on pred_parent + Filter: (a IS NULL) +(2 rows) + +DROP TABLE pred_parent, pred_child; diff --git a/src/test/regress/sql/predicate.sql b/src/test/regress/sql/predicate.sql index 563e395fde..a09ec38828 100644 --- a/src/test/regress/sql/predicate.sql +++ b/src/test/regress/sql/predicate.sql @@ -120,3 +120,30 @@ SELECT * FROM pred_tab t1 LEFT JOIN pred_tab t3 ON t2.a IS NULL OR t2.c IS NULL; DROP TABLE pred_tab; + +-- Validate we handle IS NULL and IS NOT NULL quals correctly with inheritance +-- parents. +CREATE TABLE pred_parent (a int, b int); +CREATE TABLE pred_child () INHERITS (pred_parent); +ALTER TABLE ONLY pred_parent ALTER a SET NOT NULL; + +-- Ensure that the scan on pred_child contains the IS NOT NULL qual. +EXPLAIN (COSTS OFF) +SELECT * FROM pred_parent WHERE a IS NOT NULL; + +-- Ensure we only scan pred_child and not pred_parent +EXPLAIN (COSTS OFF) +SELECT * FROM pred_parent WHERE a IS NULL; + +ALTER TABLE pred_parent ALTER a DROP NOT NULL; +ALTER TABLE pred_child ALTER a SET NOT NULL; + +-- Ensure the IS NOT NULL qual is removed from the pred_child scan. +EXPLAIN (COSTS OFF) +SELECT * FROM pred_parent WHERE a IS NOT NULL; + +-- Ensure we only scan pred_parent and not pred_child +EXPLAIN (COSTS OFF) +SELECT * FROM pred_parent WHERE a IS NULL; + +DROP TABLE pred_parent, pred_child; -- 2.31.0