Remove Value node struct

The Value node struct is a weird construct.  It is its own node type,
but most of the time, it actually has a node type of Integer, Float,
String, or BitString.  As a consequence, the struct name and the node
type don't match most of the time, and so it has to be treated
specially a lot.  There doesn't seem to be any value in the special
construct.  There is very little code that wants to accept all Value
variants but nothing else (and even if it did, this doesn't provide
any convenient way to check it), and most code wants either just one
particular node type (usually String), or it accepts a broader set of
node types besides just Value.

This change removes the Value struct and node type and replaces them
by separate Integer, Float, String, and BitString node types that are
proper node types and structs of their own and behave mostly like
normal node types.

Also, this removes the T_Null node tag, which was previously also a
possible variant of Value but wasn't actually used outside of the
Value contained in A_Const.  Replace that by an isnull field in
A_Const.

Reviewed-by: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/5ba6bc5b-3f95-04f2-2419-f8ddb4c046fb@enterprisedb.com

src/backend/nodes/README

@@ -28,7 +28,7 @@ FILES IN THIS DIRECTORY (src/backend/nodes/)
 	list.c		- generic list support
 	params.c	- Param support
 	tidbitmap.c	- TIDBitmap support
-	value.c		- support for Value nodes
+	value.c		- support for value nodes
 
 FILES IN src/include/nodes/
 

src/backend/nodes/copyfuncs.c

@@ -2729,25 +2729,30 @@ _copyA_Const(const A_Const *from)
 {
 	A_Const    *newnode = makeNode(A_Const);
 
-	/* This part must duplicate _copyValue */
-	COPY_SCALAR_FIELD(val.type);
-	switch (from->val.type)
+	COPY_SCALAR_FIELD(isnull);
+	if (!from->isnull)
 	{
-		case T_Integer:
-			COPY_SCALAR_FIELD(val.val.ival);
-			break;
-		case T_Float:
-		case T_String:
-		case T_BitString:
-			COPY_STRING_FIELD(val.val.str);
-			break;
-		case T_Null:
-			/* nothing to do */
-			break;
-		default:
-			elog(ERROR, "unrecognized node type: %d",
-				 (int) from->val.type);
-			break;
+		/* This part must duplicate other _copy*() functions. */
+		COPY_SCALAR_FIELD(val.node.type);
+		switch (nodeTag(&from->val))
+		{
+			case T_Integer:
+				COPY_SCALAR_FIELD(val.ival.val);
+				break;
+			case T_Float:
+				COPY_STRING_FIELD(val.fval.val);
+				break;
+			case T_String:
+				COPY_STRING_FIELD(val.sval.val);
+				break;
+			case T_BitString:
+				COPY_STRING_FIELD(val.bsval.val);
+				break;
+			default:
+				elog(ERROR, "unrecognized node type: %d",
+					 (int) nodeTag(&from->val));
+				break;
+		}
 	}
 
 	COPY_LOCATION_FIELD(location);
@@ -4892,32 +4897,43 @@ _copyExtensibleNode(const ExtensibleNode *from)
  *					value.h copy functions
  * ****************************************************************
  */
-static Value *
-_copyValue(const Value *from)
+static Integer *
+_copyInteger(const Integer *from)
 {
-	Value	   *newnode = makeNode(Value);
+	Integer	   *newnode = makeNode(Integer);
 
-	/* See also _copyAConst when changing this code! */
+	COPY_SCALAR_FIELD(val);
+
+	return newnode;
+}
+
+static Float *
+_copyFloat(const Float *from)
+{
+	Float	   *newnode = makeNode(Float);
+
+	COPY_STRING_FIELD(val);
+
+	return newnode;
+}
+
+static String *
+_copyString(const String *from)
+{
+	String	   *newnode = makeNode(String);
+
+	COPY_STRING_FIELD(val);
+
+	return newnode;
+}
+
+static BitString *
+_copyBitString(const BitString *from)
+{
+	BitString   *newnode = makeNode(BitString);
+
+	COPY_STRING_FIELD(val);
 
-	COPY_SCALAR_FIELD(type);
-	switch (from->type)
-	{
-		case T_Integer:
-			COPY_SCALAR_FIELD(val.ival);
-			break;
-		case T_Float:
-		case T_String:
-		case T_BitString:
-			COPY_STRING_FIELD(val.str);
-			break;
-		case T_Null:
-			/* nothing to do */
-			break;
-		default:
-			elog(ERROR, "unrecognized node type: %d",
-				 (int) from->type);
-			break;
-	}
 	return newnode;
 }
 
@@ -5314,11 +5330,16 @@ copyObjectImpl(const void *from)
 			 * VALUE NODES
 			 */
 		case T_Integer:
+			retval = _copyInteger(from);
+			break;
 		case T_Float:
+			retval = _copyFloat(from);
+			break;
 		case T_String:
+			retval = _copyString(from);
+			break;
 		case T_BitString:
-		case T_Null:
-			retval = _copyValue(from);
+			retval = _copyBitString(from);
 			break;
 
 			/*

src/backend/nodes/equalfuncs.c

@@ -2409,7 +2409,7 @@ _equalParamRef(const ParamRef *a, const ParamRef *b)
 static bool
 _equalA_Const(const A_Const *a, const A_Const *b)
 {
-	if (!equal(&a->val, &b->val))	/* hack for in-line Value field */
+	if (!equal(&a->val, &b->val))	/* hack for in-line val field */
 		return false;
 	COMPARE_LOCATION_FIELD(location);
 
@@ -3089,27 +3089,33 @@ _equalList(const List *a, const List *b)
  */
 
 static bool
-_equalValue(const Value *a, const Value *b)
+_equalInteger(const Integer *a, const Integer *b)
 {
-	COMPARE_SCALAR_FIELD(type);
+	COMPARE_SCALAR_FIELD(val);
 
-	switch (a->type)
-	{
-		case T_Integer:
-			COMPARE_SCALAR_FIELD(val.ival);
-			break;
-		case T_Float:
-		case T_String:
-		case T_BitString:
-			COMPARE_STRING_FIELD(val.str);
-			break;
-		case T_Null:
-			/* nothing to do */
-			break;
-		default:
-			elog(ERROR, "unrecognized node type: %d", (int) a->type);
-			break;
-	}
+	return true;
+}
+
+static bool
+_equalFloat(const Float *a, const Float *b)
+{
+	COMPARE_STRING_FIELD(val);
+
+	return true;
+}
+
+static bool
+_equalString(const String *a, const String *b)
+{
+	COMPARE_STRING_FIELD(val);
+
+	return true;
+}
+
+static bool
+_equalBitString(const BitString *a, const BitString *b)
+{
+	COMPARE_STRING_FIELD(val);
 
 	return true;
 }
@@ -3337,11 +3343,16 @@ equal(const void *a, const void *b)
 			break;
 
 		case T_Integer:
+			retval = _equalInteger(a, b);
+			break;
 		case T_Float:
+			retval = _equalFloat(a, b);
+			break;
 		case T_String:
+			retval = _equalString(a, b);
+			break;
 		case T_BitString:
-		case T_Null:
-			retval = _equalValue(a, b);
+			retval = _equalBitString(a, b);
 			break;
 
 			/*

src/backend/nodes/nodeFuncs.c

@@ -3537,7 +3537,6 @@ raw_expression_tree_walker(Node *node,
 		case T_Float:
 		case T_String:
 		case T_BitString:
-		case T_Null:
 		case T_ParamRef:
 		case T_A_Const:
 		case T_A_Star:

src/backend/nodes/outfuncs.c

@@ -3414,44 +3414,39 @@ _outA_Expr(StringInfo str, const A_Expr *node)
 }
 
 static void
-_outValue(StringInfo str, const Value *node)
+_outInteger(StringInfo str, const Integer *node)
 {
-	switch (node->type)
-	{
-		case T_Integer:
-			appendStringInfo(str, "%d", node->val.ival);
-			break;
-		case T_Float:
+	appendStringInfo(str, "%d", node->val);
+}
 
-			/*
-			 * We assume the value is a valid numeric literal and so does not
-			 * need quoting.
-			 */
-			appendStringInfoString(str, node->val.str);
-			break;
-		case T_String:
+static void
+_outFloat(StringInfo str, const Float *node)
+{
+	/*
+	 * We assume the value is a valid numeric literal and so does not
+	 * need quoting.
+	 */
+	appendStringInfoString(str, node->val);
+}
 
-			/*
-			 * We use outToken to provide escaping of the string's content,
-			 * but we don't want it to do anything with an empty string.
-			 */
-			appendStringInfoChar(str, '"');
-			if (node->val.str[0] != '\0')
-				outToken(str, node->val.str);
-			appendStringInfoChar(str, '"');
-			break;
-		case T_BitString:
-			/* internal representation already has leading 'b' */
-			appendStringInfoString(str, node->val.str);
-			break;
-		case T_Null:
-			/* this is seen only within A_Const, not in transformed trees */
-			appendStringInfoString(str, "NULL");
-			break;
-		default:
-			elog(ERROR, "unrecognized node type: %d", (int) node->type);
-			break;
-	}
+static void
+_outString(StringInfo str, const String *node)
+{
+	/*
+	 * We use outToken to provide escaping of the string's content,
+	 * but we don't want it to do anything with an empty string.
+	 */
+	appendStringInfoChar(str, '"');
+	if (node->val[0] != '\0')
+		outToken(str, node->val);
+	appendStringInfoChar(str, '"');
+}
+
+static void
+_outBitString(StringInfo str, const BitString *node)
+{
+	/* internal representation already has leading 'b' */
+	appendStringInfoString(str, node->val);
 }
 
 static void
@@ -3491,8 +3486,13 @@ _outA_Const(StringInfo str, const A_Const *node)
 {
 	WRITE_NODE_TYPE("A_CONST");
 
-	appendStringInfoString(str, " :val ");
-	_outValue(str, &(node->val));
+	if (node->isnull)
+		appendStringInfoString(str, "NULL");
+	else
+	{
+		appendStringInfoString(str, " :val ");
+		outNode(str, &node->val);
+	}
 	WRITE_LOCATION_FIELD(location);
 }
 
@@ -3835,14 +3835,15 @@ outNode(StringInfo str, const void *obj)
 		appendStringInfoString(str, "<>");
 	else if (IsA(obj, List) || IsA(obj, IntList) || IsA(obj, OidList))
 		_outList(str, obj);
-	else if (IsA(obj, Integer) ||
-			 IsA(obj, Float) ||
-			 IsA(obj, String) ||
-			 IsA(obj, BitString))
-	{
-		/* nodeRead does not want to see { } around these! */
-		_outValue(str, obj);
-	}
+	/* nodeRead does not want to see { } around these! */
+	else if (IsA(obj, Integer))
+		_outInteger(str, (Integer *) obj);
+	else if (IsA(obj, Float))
+		_outFloat(str, (Float *) obj);
+	else if (IsA(obj, String))
+		_outString(str, (String *) obj);
+	else if (IsA(obj, BitString))
+		_outBitString(str, (BitString *) obj);
 	else
 	{
 		appendStringInfoChar(str, '{');

src/backend/nodes/value.c

@@ -1,7 +1,7 @@
 /*-------------------------------------------------------------------------
  *
  * value.c
- *	  implementation of Value nodes
+ *	  implementation of value nodes
  *
  *
  * Copyright (c) 2003-2021, PostgreSQL Global Development Group
@@ -14,18 +14,17 @@
  */
 #include "postgres.h"
 
-#include "nodes/parsenodes.h"
+#include "nodes/value.h"
 
 /*
  *	makeInteger
  */
-Value *
+Integer *
 makeInteger(int i)
 {
-	Value	   *v = makeNode(Value);
+	Integer	   *v = makeNode(Integer);
 
-	v->type = T_Integer;
-	v->val.ival = i;
+	v->val = i;
 	return v;
 }
 
@@ -34,13 +33,12 @@ makeInteger(int i)
  *
  * Caller is responsible for passing a palloc'd string.
  */
-Value *
+Float *
 makeFloat(char *numericStr)
 {
-	Value	   *v = makeNode(Value);
+	Float	   *v = makeNode(Float);
 
-	v->type = T_Float;
-	v->val.str = numericStr;
+	v->val = numericStr;
 	return v;
 }
 
@@ -49,13 +47,12 @@ makeFloat(char *numericStr)
  *
  * Caller is responsible for passing a palloc'd string.
  */
-Value *
+String *
 makeString(char *str)
 {
-	Value	   *v = makeNode(Value);
+	String	   *v = makeNode(String);
 
-	v->type = T_String;
-	v->val.str = str;
+	v->val = str;
 	return v;
 }
 
@@ -64,12 +61,11 @@ makeString(char *str)
  *
  * Caller is responsible for passing a palloc'd string.
  */
-Value *
+BitString *
 makeBitString(char *str)
 {
-	Value	   *v = makeNode(Value);
+	BitString  *v = makeNode(BitString);
 
-	v->type = T_BitString;
-	v->val.str = str;
+	v->val = str;
 	return v;
 }