On Tue, 9 Apr 2024 at 21:55, Richard Guo <guofengli...@gmail.com> wrote:
>
> In b262ad440e we introduced an optimization that drops IS NOT NULL quals
> on a NOT NULL column, and reduces IS NULL quals on a NOT NULL column to
> constant-FALSE. I happened to notice that this is not working correctly
> for traditional inheritance parents. Traditional inheritance parents
> might have NOT NULL constraints marked NO INHERIT, while their child
> tables do not have NOT NULL constraints. In such a case, we would have
> problems when we have removed redundant IS NOT NULL restriction clauses
> of the parent rel, as this could cause NULL values from child tables to
> not be filtered out, or when we have reduced IS NULL restriction clauses
> of the parent rel to constant-FALSE, as this could cause NULL values
> from child tables to not be selected out.
hmm, yeah, inheritance tables were overlooked.
I looked at the patch and I don't think it's a good idea to skip
recording NOT NULL constraints to fix based on the fact that it
happens to result in this particular optimisation working correctly.
It seems that just makes this work in favour of possibly being wrong
for some future optimisation where we have something else that looks
at the RelOptInfo.notnullattnums and makes some decision that assumes
the lack of corresponding notnullattnums member means the column is
NULLable.
I think a better fix is just to not apply the optimisation for
inheritance RTEs in add_base_clause_to_rel(). If we do it this way,
it's only the inh==true RTE that we skip. Remember that there are two
RangeTblEntries for an inheritance parent. The other one will have
inh==false, and we can still have the optimisation as that's the one
that'll be used in the final plan. It'll be the inh==true one that we
copy the quals from in apply_child_basequals(), so we've no need to
worry about missing baserestrictinfos when applying the base quals to
the child.
For partitioned tables, there's only a single RTE with inh==true.
We're free to include the redundant quals there to be applied or
skipped in apply_child_basequals(). The corresponding RangeTblEntry
is not going to be scanned in the final plan, so it does not matter
about the extra qual.
The revised patch is attached.
David
diff --git a/src/backend/optimizer/plan/initsplan.c
b/src/backend/optimizer/plan/initsplan.c
index d3868b628d..1de965b3ad 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -2629,32 +2629,45 @@ 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.
*/
- if (restriction_is_always_false(root, restrictinfo))
+ if (!rte->inh)
{
- 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..761ad101b3 100644
--- a/src/backend/optimizer/util/inherit.c
+++ b/src/backend/optimizer/util/inherit.c
@@ -875,6 +875,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 +887,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/relnode.c
b/src/backend/optimizer/util/relnode.c
index d791c4108d..2bbb9aabb6 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -372,6 +372,13 @@ 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
@@ -393,9 +400,6 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo
*parent)
}
}
- /* Save the finished struct in the query's simple_rel_array */
- root->simple_rel_array[relid] = rel;
-
return rel;
}
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;
