Don't explicitly mark range partitioning columns NOT NULL.

This seemed like a good idea originally because there's no way to mark
a range partition as accepting NULL, but that now seems more like a
current limitation than something we want to lock down for all time.
For example, there's a proposal to add the notion of a default
partition which accepts all rows not otherwise routed, which directly
conflicts with the idea that a range-partitioned table should never
allow nulls anywhere.  So let's change this while we still can, by
putting the NOT NULL test into the partition constraint instead of
changing the column properties.

Amit Langote and Robert Haas, reviewed by Amit Kapila

Discussion: http://postgr.es/m/8e2dd63d-c6fb-bb74-3c2b-ed6d63629c9d@lab.ntt.co.jp

2 files changed, 45 insertions, 83 deletions

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index acd9f2d273e..7304f6c29ab 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1449,17 +1449,18 @@ get_range_key_properties(PartitionKey key, int keynum,
  * as the lower bound tuple and (au, bu, cu) as the upper bound tuple, we
  * generate an expression tree of the following form:
  *
+ *  (a IS NOT NULL) and (b IS NOT NULL) and (c IS NOT NULL)
+ *		AND
  *	(a > al OR (a = al AND b > bl) OR (a = al AND b = bl AND c >= cl))
  *		AND
  *	(a < au OR (a = au AND b < bu) OR (a = au AND b = bu AND c < cu))
  *
- * If, say, b were an expression key instead of a simple column, we also
- * append (b IS NOT NULL) to the AND's argument list.
- *
  * It is often the case that a prefix of lower and upper bound tuples contains
  * the same values, for example, (al = au), in which case, we will emit an
  * expression tree of the following form:
  *
+ *  (a IS NOT NULL) and (b IS NOT NULL) and (c IS NOT NULL)
+ *		AND
  *	(a = al)
  *		AND
  *	(b > bl OR (b = bl AND c >= cl))
@@ -1472,11 +1473,11 @@ get_range_key_properties(PartitionKey key, int keynum,
  *	(b < bu) OR (b = bu), which is simplified to (b <= bu)
  *
  * In most common cases with only one partition column, say a, the following
- * expression tree will be generated: a >= al AND a < au
+ * expression tree will be generated: a IS NOT NULL AND a >= al AND a < au
  *
  * If all values of both lower and upper bounds are UNBOUNDED, the partition
  * does not really have a constraint, except the IS NOT NULL constraint for
- * any expression keys.
+ * partition keys.
  *
  * If we end up with an empty result list, we append return a single-member
  * list containing a constant-true expression in that case, because callers
@@ -1512,32 +1513,37 @@ get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec)
 	num_or_arms = key->partnatts;
 
 	/*
-	 * A range-partitioned table does not allow partition keys to be null. For
-	 * simple columns, their NOT NULL constraint suffices for the enforcement
-	 * of non-nullability.  But for the expression keys, which are still
-	 * nullable, we must emit a IS NOT NULL expression.  Collect them in
-	 * result first.
+	 * A range-partitioned table does not currently allow partition keys to
+	 * be null, so emit an IS NOT NULL expression for each key column.
 	 */
 	partexprs_item = list_head(key->partexprs);
 	for (i = 0; i < key->partnatts; i++)
 	{
-		if (key->partattrs[i] == 0)
-		{
-			Expr	   *keyCol;
+		Expr	   *keyCol;
 
+		if (key->partattrs[i] != 0)
+		{
+			keyCol = (Expr *) makeVar(1,
+									  key->partattrs[i],
+									  key->parttypid[i],
+									  key->parttypmod[i],
+									  key->parttypcoll[i],
+									  0);
+		}
+		else
+		{
 			if (partexprs_item == NULL)
 				elog(ERROR, "wrong number of partition key expressions");
-			keyCol = lfirst(partexprs_item);
+			keyCol = copyObject(lfirst(partexprs_item));
 			partexprs_item = lnext(partexprs_item);
-			Assert(!IsA(keyCol, Var));
-
-			nulltest = makeNode(NullTest);
-			nulltest->arg = keyCol;
-			nulltest->nulltesttype = IS_NOT_NULL;
-			nulltest->argisrow = false;
-			nulltest->location = -1;
-			result = lappend(result, nulltest);
 		}
+
+		nulltest = makeNode(NullTest);
+		nulltest->arg = keyCol;
+		nulltest->nulltesttype = IS_NOT_NULL;
+		nulltest->argisrow = false;
+		nulltest->location = -1;
+		result = lappend(result, nulltest);
 	}
 
 	/*
@@ -1948,7 +1954,8 @@ get_partition_for_tuple(PartitionDispatch *pd,
 		{
 			*failed_at = parent;
 			*failed_slot = slot;
-			return -1;
+			result = -1;
+			goto error_exit;
 		}
 
 		/*
@@ -1964,12 +1971,21 @@ get_partition_for_tuple(PartitionDispatch *pd,
 
 		if (key->strategy == PARTITION_STRATEGY_RANGE)
 		{
-			/* Disallow nulls in the range partition key of the tuple */
+			/*
+			 * Since we cannot route tuples with NULL partition keys through
+			 * a range-partitioned table, simply return that no partition
+			 * exists
+			 */
 			for (i = 0; i < key->partnatts; i++)
+			{
 				if (isnull[i])
-					ereport(ERROR,
-							(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
-						errmsg("range partition key of row contains null")));
+				{
+					*failed_at = parent;
+					*failed_slot = slot;
+					result = -1;
+					goto error_exit;
+				}
+			}
 		}
 
 		/*
@@ -2032,6 +2048,7 @@ get_partition_for_tuple(PartitionDispatch *pd,
 			parent = pd[-parent->indexes[cur_index]];
 	}
 
+error_exit:
 	ecxt->ecxt_scantuple = ecxt_scantuple_old;
 	return result;
 }
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7319aa597e7..99c51b812d4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -805,13 +805,11 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	if (stmt->partspec)
 	{
 		char		strategy;
-		int			partnatts,
-					i;
+		int			partnatts;
 		AttrNumber	partattrs[PARTITION_MAX_KEYS];
 		Oid			partopclass[PARTITION_MAX_KEYS];
 		Oid			partcollation[PARTITION_MAX_KEYS];
 		List	   *partexprs = NIL;
-		List	   *cmds = NIL;
 
 		/*
 		 * We need to transform the raw parsetrees corresponding to partition
@@ -828,33 +826,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		partnatts = list_length(stmt->partspec->partParams);
 		StorePartitionKey(rel, strategy, partnatts, partattrs, partexprs,
 						  partopclass, partcollation);
-
-		/* Force key columns to be NOT NULL when using range partitioning */
-		if (strategy == PARTITION_STRATEGY_RANGE)
-		{
-			for (i = 0; i < partnatts; i++)
-			{
-				AttrNumber	partattno = partattrs[i];
-				Form_pg_attribute attform = descriptor->attrs[partattno - 1];
-
-				if (partattno != 0 && !attform->attnotnull)
-				{
-					/* Add a subcommand to make this one NOT NULL */
-					AlterTableCmd *cmd = makeNode(AlterTableCmd);
-
-					cmd->subtype = AT_SetNotNull;
-					cmd->name = pstrdup(NameStr(attform->attname));
-					cmds = lappend(cmds, cmd);
-				}
-			}
-
-			/*
-			 * Although, there cannot be any partitions yet, we still need to
-			 * pass true for recurse; ATPrepSetNotNull() complains if we don't
-			 */
-			if (cmds != NIL)
-				AlterTableInternal(RelationGetRelid(rel), cmds, true);
-		}
 	}
 
 	/*
@@ -5703,32 +5674,6 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
 	}
 
 	/*
-	 * If the table is a range partitioned table, check that the column is not
-	 * in the partition key.
-	 */
-	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-	{
-		PartitionKey key = RelationGetPartitionKey(rel);
-
-		if (get_partition_strategy(key) == PARTITION_STRATEGY_RANGE)
-		{
-			int			partnatts = get_partition_natts(key),
-						i;
-
-			for (i = 0; i < partnatts; i++)
-			{
-				AttrNumber	partattnum = get_partition_col_attnum(key, i);
-
-				if (partattnum == attnum)
-					ereport(ERROR,
-							(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
-							 errmsg("column \"%s\" is in range partition key",
-									colName)));
-			}
-		}
-	}
-
-	/*
 	 * Okay, actually perform the catalog change ... if needed
 	 */
 	if (((Form_pg_attribute) GETSTRUCT(tuple))->attnotnull)