On Wed, 10 Apr 2024 at 19:12, Richard Guo <guofengli...@gmail.com> wrote:
> And I think recording NOT NULL columns for traditional inheritance
> parents can be error-prone for some future optimization where we look
> at an inheritance parent's notnullattnums and make decisions based on
> the assumption that the included columns are non-nullable. The issue
> discussed here serves as an example of this potential problem.
I admit that it seems more likely that having a member set in
notnullattnums for an inheritance parent is more likely to cause
future bugs than if we just leave them blank. But I also don't
believe leaving them all blank is the right thing unless we document
that the field isn't populated for traditional inheritance parent
tables and if the code needs to not the NOT NULL status of a column
for that table ONLY, then the code should look at the RelOptInfo
corresponding to the inh==false RangeTblEntry for that relation. If we
don't document the fact that we don't set the notnullattnums field
then someone might write some code thinking we correctly populate it.
If the parent and all children have NOT NULL constraints for a
column, then unless we document we don't populate notnullattnums, it
seems reasonable to assume that's a bug.
If we skip populating notnullattnums for inh==true non-partitioned
tables, I think we also still need to skip applying the NOT NULL qual
optimisation for inh==true RTEs as my version of the code did.
Reasons being: 1) it's a pointless exercise since we'll always end up
adding the RestrictInfo without modification to the RelOptInfo's
baserestrictinfo, and 2) The optimisation in question would be looking
at the notnullattnums that isn't correctly populated.
Resulting patch attached.
David
diff --git a/src/backend/optimizer/plan/initsplan.c
b/src/backend/optimizer/plan/initsplan.c
index d3868b628d..8981031d5d 100644
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -2629,32 +2629,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)
{
- 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/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..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/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;
