Fix IS [NOT] NULL qual optimization for inheritance tables

david-rowley · david-rowley · commit 3af7040985b6 · 2024-04-12T20:07:53.000+12:00
b262ad4 added code to have the planner remove redundant IS NOT NULL quals and eliminate needless scans for IS NULL quals on tables where the qual's column has a NOT NULL constraint. That commit failed to consider that an inheritance parent table could have differing NOT NULL constraints between the parent and the child. This caused issues as if we eliminated a qual on the parent, when applying the quals to child tables in apply_child_basequals(), the qual might not have been added to the parent's baserestrictinfo. Here we fix this by not applying the optimization to remove redundant quals to RelOptInfos belonging to inheritance parents and applying the optimization again in apply_child_basequals(). Effectively, this means that the parent and child are considered independently as the parent has both an inh=true and inh=false RTE and we still apply the optimization to the RelOptInfo corresponding to the inh=false RTE. We're able to still apply the optimization in add_base_clause_to_rel() for partitioned tables as the NULLability of partitions must match that of their parent. And, if we ever expand restriction_is_always_false() and restriction_is_always_true() to handle partition constraints then we can apply the same logic as, even in multi-level partitioned tables, there's no way to route values to a partition when the qual does not match the partition qual of the partitioned table's parent partition. The same is true for CHECK constraints as those must also match between arent partitioned tables and their partitions. Author: Richard Guo, David Rowley Discussion: https://postgr.es/m/CAMbWs4930gQSZmjR7aANzEapdy61gCg6z8dT-kAEYD0sYWKPdQ@mail.gmail.com
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
@@ -31,6 +31,7 @@
 #include "parser/analyze.h"
 #include "rewrite/rewriteManip.h"
 #include "utils/lsyscache.h"
+#include "utils/rel.h"
 #include "utils/typcache.h"
 
 /* These parameters are set by GUC */
@@ -2629,32 +2630,51 @@ 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.
+	 *
+	 * We make an exception to this is for partitioned tables.  For these, we
+	 * always apply the constant-TRUE and constant-FALSE transformations.  A
+	 * qual which is either of these for a partitioned table must also be that
+	 * for all of its child partitions.
 	 */
-	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
@@ -822,11 +822,13 @@ expand_appendrel_subquery(PlannerInfo *root, RelOptInfo *rel,
 /*
  * apply_child_basequals
  *		Populate childrel's base restriction quals from parent rel's quals,
- *		translating them using appinfo.
+ *		translating Vars using appinfo and re-checking for quals which are
+ *		constant-TRUE or constant-FALSE when applied to this child relation.
  *
  * 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.
+ * a dummy rel in this case, since it doesn't need to be scanned.  Constant
+ * true quals are ignored.
  */
 bool
 apply_child_basequals(PlannerInfo *root, RelOptInfo *parentrel,
@@ -875,6 +877,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 +889,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
@@ -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
@@ -373,10 +373,20 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent)
 	}
 
 	/*
-	 * 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.)
+	 * 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;
+
+	/*
+	 * Apply the parent's quals to the child, with appropriate substitution of
+	 * variables.  If the resulting clause is constant-FALSE or NULL after
+	 * applying transformations, apply_child_basequals returns false to
+	 * indicate that scanning this relation won't yield any rows.  In this
+	 * case, we mark the child as dummy right away.  (We must do this
+	 * immediately so that pruning works correctly when recursing in
+	 * expand_partitioned_rtentry.)
 	 */
 	if (parent)
 	{
@@ -386,16 +396,13 @@ 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.
+			 * Restriction clause reduced to constant FALSE or NULL.  Mark as
+			 * dummy so we won't scan this relation.
 			 */
 			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
@@ -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
@@ -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);
+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
@@ -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);
+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;
