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

5 files changed, 51 insertions, 39 deletions

diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 7811ad5d52..36bf1dc92b 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4412,18 +4412,6 @@ _copyAlterPolicyStmt(const AlterPolicyStmt *from)
 	return newnode;
 }
 
-static PartitionSpec *
-_copyPartitionSpec(const PartitionSpec *from)
-{
-	PartitionSpec *newnode = makeNode(PartitionSpec);
-
-	COPY_STRING_FIELD(strategy);
-	COPY_NODE_FIELD(partParams);
-	COPY_LOCATION_FIELD(location);
-
-	return newnode;
-}
-
 static PartitionElem *
 _copyPartitionElem(const PartitionElem *from)
 {
@@ -4438,6 +4426,18 @@ _copyPartitionElem(const PartitionElem *from)
 	return newnode;
 }
 
+static PartitionSpec *
+_copyPartitionSpec(const PartitionSpec *from)
+{
+	PartitionSpec *newnode = makeNode(PartitionSpec);
+
+	COPY_STRING_FIELD(strategy);
+	COPY_NODE_FIELD(partParams);
+	COPY_LOCATION_FIELD(location);
+
+	return newnode;
+}
+
 static PartitionBoundSpec *
 _copyPartitionBoundSpec(const PartitionBoundSpec *from)
 {
@@ -5509,12 +5509,12 @@ copyObjectImpl(const void *from)
 		case T_TriggerTransition:
 			retval = _copyTriggerTransition(from);
 			break;
-		case T_PartitionSpec:
-			retval = _copyPartitionSpec(from);
-			break;
 		case T_PartitionElem:
 			retval = _copyPartitionElem(from);
 			break;
+		case T_PartitionSpec:
+			retval = _copyPartitionSpec(from);
+			break;
 		case T_PartitionBoundSpec:
 			retval = _copyPartitionBoundSpec(from);
 			break;
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index c9a8c34892..5bcf0317dc 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2813,22 +2813,22 @@ _equalTriggerTransition(const TriggerTransition *a, const TriggerTransition *b)
 }
 
 static bool
-_equalPartitionSpec(const PartitionSpec *a, const PartitionSpec *b)
+_equalPartitionElem(const PartitionElem *a, const PartitionElem *b)
 {
-	COMPARE_STRING_FIELD(strategy);
-	COMPARE_NODE_FIELD(partParams);
+	COMPARE_STRING_FIELD(name);
+	COMPARE_NODE_FIELD(expr);
+	COMPARE_NODE_FIELD(collation);
+	COMPARE_NODE_FIELD(opclass);
 	COMPARE_LOCATION_FIELD(location);
 
 	return true;
 }
 
 static bool
-_equalPartitionElem(const PartitionElem *a, const PartitionElem *b)
+_equalPartitionSpec(const PartitionSpec *a, const PartitionSpec *b)
 {
-	COMPARE_STRING_FIELD(name);
-	COMPARE_NODE_FIELD(expr);
-	COMPARE_NODE_FIELD(collation);
-	COMPARE_NODE_FIELD(opclass);
+	COMPARE_STRING_FIELD(strategy);
+	COMPARE_NODE_FIELD(partParams);
 	COMPARE_LOCATION_FIELD(location);
 
 	return true;
@@ -3660,12 +3660,12 @@ equal(const void *a, const void *b)
 		case T_TriggerTransition:
 			retval = _equalTriggerTransition(a, b);
 			break;
-		case T_PartitionSpec:
-			retval = _equalPartitionSpec(a, b);
-			break;
 		case T_PartitionElem:
 			retval = _equalPartitionElem(a, b);
 			break;
+		case T_PartitionSpec:
+			retval = _equalPartitionSpec(a, b);
+			break;
 		case T_PartitionBoundSpec:
 			retval = _equalPartitionBoundSpec(a, b);
 			break;
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 95c1d3efbb..41f3408cfc 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -1560,6 +1560,12 @@ exprLocation(const Node *expr)
 			/* just use nested expr's location */
 			loc = exprLocation((Node *) ((const InferenceElem *) expr)->expr);
 			break;
+		case T_PartitionElem:
+			loc = ((const PartitionElem *) expr)->location;
+			break;
+		case T_PartitionSpec:
+			loc = ((const PartitionSpec *) expr)->location;
+			break;
 		case T_PartitionBoundSpec:
 			loc = ((const PartitionBoundSpec *) expr)->location;
 			break;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 4949d58864..9189c8d43f 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -3516,16 +3516,6 @@ _outForeignKeyCacheInfo(StringInfo str, const ForeignKeyCacheInfo *node)
 }
 
 static void
-_outPartitionSpec(StringInfo str, const PartitionSpec *node)
-{
-	WRITE_NODE_TYPE("PARTITIONBY");
-
-	WRITE_STRING_FIELD(strategy);
-	WRITE_NODE_FIELD(partParams);
-	WRITE_LOCATION_FIELD(location);
-}
-
-static void
 _outPartitionElem(StringInfo str, const PartitionElem *node)
 {
 	WRITE_NODE_TYPE("PARTITIONELEM");
@@ -3538,6 +3528,16 @@ _outPartitionElem(StringInfo str, const PartitionElem *node)
 }
 
 static void
+_outPartitionSpec(StringInfo str, const PartitionSpec *node)
+{
+	WRITE_NODE_TYPE("PARTITIONBY");
+
+	WRITE_STRING_FIELD(strategy);
+	WRITE_NODE_FIELD(partParams);
+	WRITE_LOCATION_FIELD(location);
+}
+
+static void
 _outPartitionBoundSpec(StringInfo str, const PartitionBoundSpec *node)
 {
 	WRITE_NODE_TYPE("PARTITIONBOUND");
@@ -3546,6 +3546,7 @@ _outPartitionBoundSpec(StringInfo str, const PartitionBoundSpec *node)
 	WRITE_NODE_FIELD(listdatums);
 	WRITE_NODE_FIELD(lowerdatums);
 	WRITE_NODE_FIELD(upperdatums);
+	/* XXX somebody forgot location field; too late to change for v10 */
 }
 
 static void
@@ -3555,6 +3556,7 @@ _outPartitionRangeDatum(StringInfo str, const PartitionRangeDatum *node)
 
 	WRITE_BOOL_FIELD(infinite);
 	WRITE_NODE_FIELD(value);
+	/* XXX somebody forgot location field; too late to change for v10 */
 }
 
 /*
@@ -4184,12 +4186,12 @@ outNode(StringInfo str, const void *obj)
 			case T_TriggerTransition:
 				_outTriggerTransition(str, obj);
 				break;
-			case T_PartitionSpec:
-				_outPartitionSpec(str, obj);
-				break;
 			case T_PartitionElem:
 				_outPartitionElem(str, obj);
 				break;
+			case T_PartitionSpec:
+				_outPartitionSpec(str, obj);
+				break;
 			case T_PartitionBoundSpec:
 				_outPartitionBoundSpec(str, obj);
 				break;
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index e24f5d6726..b59ebd63ec 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -2376,6 +2376,8 @@ _readPartitionBoundSpec(void)
 	READ_NODE_FIELD(listdatums);
 	READ_NODE_FIELD(lowerdatums);
 	READ_NODE_FIELD(upperdatums);
+	/* XXX somebody forgot location field; too late to change for v10 */
+	local_node->location = -1;
 
 	READ_DONE();
 }
@@ -2390,6 +2392,8 @@ _readPartitionRangeDatum(void)
 
 	READ_BOOL_FIELD(infinite);
 	READ_NODE_FIELD(value);
+	/* XXX somebody forgot location field; too late to change for v10 */
+	local_node->location = -1;
 
 	READ_DONE();
 }