Code review focused on new node types added by partitioning support.

Fix failure to check that we got a plain Const from const-simplification of
a coercion request.  This is the cause of bug #14666 from Tian Bing: there
is an int4 to money cast, but it's only stable not immutable (because of
dependence on lc_monetary), resulting in a FuncExpr that the code was
miserably unequipped to deal with, or indeed even to notice that it was
failing to deal with.  Add test cases around this coercion behavior.

In view of the above, sprinkle the code liberally with castNode() macros,
in hope of catching the next such bug a bit sooner.  Also, change some
functions that were randomly declared to take Node* to take more specific
pointer types.  And change some struct fields that were declared Node*
but could be given more specific types, allowing removal of assorted
explicit casts.

Place PARTITION_MAX_KEYS check a bit closer to the code it's protecting.
Likewise check only-one-key-for-list-partitioning restriction in a less
random place.

Avoid not-per-project-style usages like !strcmp(...).

Fix assorted failures to avoid scribbling on the input of parse
transformation.  I'm not sure how necessary this is, but it's entirely
silly for these functions to be expending cycles to avoid that and not
getting it right.

Add guards against partitioning on system columns.

Put backend/nodes/ support code into an order that matches handling
of these node types elsewhere.

Annotate the fact that somebody added location fields to PartitionBoundSpec
and PartitionRangeDatum but forgot to handle them in
outfuncs.c/readfuncs.c.  This is fairly harmless for production purposes
(since readfuncs.c would just substitute -1 anyway) but it's still bogus.
It's not worth forcing a post-beta1 initdb just to fix this, but if we
have another reason to force initdb before 10.0, we should go back and
clean this up.

Contrariwise, somebody added location fields to PartitionElem and
PartitionSpec but forgot to teach exprLocation() about them.

Consolidate duplicative code in transformPartitionBound().

Improve a couple of error messages.

Improve assorted commentary.

Re-pgindent the files touched by this patch; this affects a few comment
blocks that must have been added quite recently.

Report: https://postgr.es/m/20170524024550.29935.14396@wrigleys.postgresql.org

1 files changed, 65 insertions, 25 deletions

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fb961e46c4a..7959120f53e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -756,7 +756,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 	/* Process and store partition bound, if any. */
 	if (stmt->partbound)
 	{
-		Node	   *bound;
+		PartitionBoundSpec *bound;
 		ParseState *pstate;
 		Oid			parentId = linitial_oid(inheritOids);
 		Relation	parent;
@@ -777,6 +777,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		/* Tranform the bound values */
 		pstate = make_parsestate(NULL);
 		pstate->p_sourcetext = queryString;
+
 		bound = transformPartitionBound(pstate, parent, stmt->partbound);
 
 		/*
@@ -812,6 +813,15 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		Oid			partcollation[PARTITION_MAX_KEYS];
 		List	   *partexprs = NIL;
 
+		partnatts = list_length(stmt->partspec->partParams);
+
+		/* Protect fixed-size arrays here and in executor */
+		if (partnatts > PARTITION_MAX_KEYS)
+			ereport(ERROR,
+					(errcode(ERRCODE_TOO_MANY_COLUMNS),
+					 errmsg("cannot partition using more than %d columns",
+							PARTITION_MAX_KEYS)));
+
 		/*
 		 * We need to transform the raw parsetrees corresponding to partition
 		 * expressions into executable expression trees.  Like column defaults
@@ -820,11 +830,11 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 		 */
 		stmt->partspec = transformPartitionSpec(rel, stmt->partspec,
 												&strategy);
+
 		ComputePartitionAttrs(rel, stmt->partspec->partParams,
 							  partattrs, &partexprs, partopclass,
 							  partcollation);
 
-		partnatts = list_length(stmt->partspec->partParams);
 		StorePartitionKey(rel, strategy, partnatts, partattrs, partexprs,
 						  partopclass, partcollation);
 	}
@@ -13109,6 +13119,8 @@ RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid, Oid oldrelid,
 
 /*
  * Transform any expressions present in the partition key
+ *
+ * Returns a transformed PartitionSpec, as well as the strategy code
  */
 static PartitionSpec *
 transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
@@ -13121,13 +13133,13 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
 	newspec = makeNode(PartitionSpec);
 
 	newspec->strategy = partspec->strategy;
-	newspec->location = partspec->location;
 	newspec->partParams = NIL;
+	newspec->location = partspec->location;
 
 	/* Parse partitioning strategy name */
-	if (!pg_strcasecmp(partspec->strategy, "list"))
+	if (pg_strcasecmp(partspec->strategy, "list") == 0)
 		*strategy = PARTITION_STRATEGY_LIST;
-	else if (!pg_strcasecmp(partspec->strategy, "range"))
+	else if (pg_strcasecmp(partspec->strategy, "range") == 0)
 		*strategy = PARTITION_STRATEGY_RANGE;
 	else
 		ereport(ERROR,
@@ -13135,6 +13147,13 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
 				 errmsg("unrecognized partitioning strategy \"%s\"",
 						partspec->strategy)));
 
+	/* Check valid number of columns for strategy */
+	if (*strategy == PARTITION_STRATEGY_LIST &&
+		list_length(partspec->partParams) != 1)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+				 errmsg("cannot use \"list\" partition strategy with more than one column")));
+
 	/*
 	 * Create a dummy ParseState and insert the target relation as its sole
 	 * rangetable entry.  We need a ParseState for transformExpr.
@@ -13146,16 +13165,16 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
 	/* take care of any partition expressions */
 	foreach(l, partspec->partParams)
 	{
+		PartitionElem *pelem = castNode(PartitionElem, lfirst(l));
 		ListCell   *lc;
-		PartitionElem *pelem = (PartitionElem *) lfirst(l);
 
 		/* Check for PARTITION BY ... (foo, foo) */
 		foreach(lc, newspec->partParams)
 		{
-			PartitionElem *pparam = (PartitionElem *) lfirst(lc);
+			PartitionElem *pparam = castNode(PartitionElem, lfirst(lc));
 
 			if (pelem->name && pparam->name &&
-				!strcmp(pelem->name, pparam->name))
+				strcmp(pelem->name, pparam->name) == 0)
 				ereport(ERROR,
 						(errcode(ERRCODE_DUPLICATE_COLUMN),
 						 errmsg("column \"%s\" appears more than once in partition key",
@@ -13165,6 +13184,9 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
 
 		if (pelem->expr)
 		{
+			/* Copy, to avoid scribbling on the input */
+			pelem = copyObject(pelem);
+
 			/* Now do parse transformation of the expression */
 			pelem->expr = transformExpr(pstate, pelem->expr,
 										EXPR_KIND_PARTITION_EXPRESSION);
@@ -13180,7 +13202,8 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
 }
 
 /*
- * Compute per-partition-column information from a list of PartitionElem's
+ * Compute per-partition-column information from a list of PartitionElems.
+ * Expressions in the PartitionElems must be parse-analyzed already.
  */
 static void
 ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
@@ -13192,7 +13215,7 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
 	attn = 0;
 	foreach(lc, partParams)
 	{
-		PartitionElem *pelem = (PartitionElem *) lfirst(lc);
+		PartitionElem *pelem = castNode(PartitionElem, lfirst(lc));
 		Oid			atttype;
 		Oid			attcollation;
 
@@ -13202,7 +13225,8 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
 			HeapTuple	atttuple;
 			Form_pg_attribute attform;
 
-			atttuple = SearchSysCacheAttName(RelationGetRelid(rel), pelem->name);
+			atttuple = SearchSysCacheAttName(RelationGetRelid(rel),
+											 pelem->name);
 			if (!HeapTupleIsValid(atttuple))
 				ereport(ERROR,
 						(errcode(ERRCODE_UNDEFINED_COLUMN),
@@ -13212,7 +13236,7 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
 
 			if (attform->attnum <= 0)
 				ereport(ERROR,
-						(errcode(ERRCODE_UNDEFINED_COLUMN),
+						(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
 				   errmsg("cannot use system column \"%s\" in partition key",
 						  pelem->name)));
 
@@ -13220,8 +13244,6 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
 			atttype = attform->atttypid;
 			attcollation = attform->attcollation;
 			ReleaseSysCache(atttuple);
-
-			/* Note that whole-row references can't happen here; see below */
 		}
 		else
 		{
@@ -13240,7 +13262,7 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
 				expr = (Node *) ((CollateExpr *) expr)->arg;
 
 			if (IsA(expr, Var) &&
-				((Var *) expr)->varattno != InvalidAttrNumber)
+				((Var *) expr)->varattno > 0)
 			{
 				/*
 				 * User wrote "(column)" or "(column COLLATE something)".
@@ -13251,11 +13273,18 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
 			else
 			{
 				Bitmapset  *expr_attrs = NULL;
+				int			i;
 
 				partattrs[attn] = 0;	/* marks the column as expression */
 				*partexprs = lappend(*partexprs, expr);
 
 				/*
+				 * Try to simplify the expression before checking for
+				 * mutability.  The main practical value of doing it in this
+				 * order is that an inline-able SQL-language function will be
+				 * accepted if its expansion is immutable, whether or not the
+				 * function itself is marked immutable.
+				 *
 				 * Note that expression_planner does not change the passed in
 				 * expression destructively and we have already saved the
 				 * expression to be stored into the catalog above.
@@ -13274,27 +13303,38 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
 							 errmsg("functions in partition key expression must be marked IMMUTABLE")));
 
 				/*
-				 * While it is not exactly *wrong* for an expression to be a
-				 * constant value, it seems better to prevent such input.
-				 */
-				if (IsA(expr, Const))
-					ereport(ERROR,
-							(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
-							 errmsg("cannot use constant expression as partition key")));
-
-				/*
 				 * transformPartitionSpec() should have already rejected
 				 * subqueries, aggregates, window functions, and SRFs, based
 				 * on the EXPR_KIND_ for partition expressions.
 				 */
 
-				/* Cannot have expressions containing whole-row references */
+				/*
+				 * Cannot have expressions containing whole-row references or
+				 * system column references.
+				 */
 				pull_varattnos(expr, 1, &expr_attrs);
 				if (bms_is_member(0 - FirstLowInvalidHeapAttributeNumber,
 								  expr_attrs))
 					ereport(ERROR,
 							(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
 							 errmsg("partition key expressions cannot contain whole-row references")));
+				for (i = FirstLowInvalidHeapAttributeNumber; i < 0; i++)
+				{
+					if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber,
+									  expr_attrs))
+						ereport(ERROR,
+								(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+								 errmsg("partition key expressions cannot contain system column references")));
+				}
+
+				/*
+				 * While it is not exactly *wrong* for a partition expression
+				 * to be a constant, it seems better to reject such keys.
+				 */
+				if (IsA(expr, Const))
+					ereport(ERROR,
+							(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+							 errmsg("cannot use constant expression as partition key")));
 			}
 		}