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

8 files changed, 100 insertions, 105 deletions

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 6a0f46505c5..e3068a374ee 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -166,7 +166,7 @@ static Node *makeIntConst(int val, int location);
 static Node *makeFloatConst(char *str, int location);
 static Node *makeBitStringConst(char *str, int location);
 static Node *makeNullAConst(int location);
-static Node *makeAConst(Value *v, int location);
+static Node *makeAConst(Node *v, int location);
 static Node *makeBoolAConst(bool state, int location);
 static RoleSpec *makeRoleSpec(RoleSpecType type, int location);
 static void check_qualified_name(List *names, core_yyscan_t yyscanner);
@@ -183,7 +183,7 @@ static void insertSelectOptions(SelectStmt *stmt,
 								core_yyscan_t yyscanner);
 static Node *makeSetOp(SetOperation op, bool all, Node *larg, Node *rarg);
 static Node *doNegate(Node *n, int location);
-static void doNegateFloat(Value *v);
+static void doNegateFloat(Float *v);
 static Node *makeAndExpr(Node *lexpr, Node *rexpr, int location);
 static Node *makeOrExpr(Node *lexpr, Node *rexpr, int location);
 static Node *makeNotExpr(Node *expr, int location);
@@ -228,7 +228,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 	OnCommitAction		oncommit;
 	List				*list;
 	Node				*node;
-	Value				*value;
 	ObjectType			objtype;
 	TypeName			*typnam;
 	FunctionParameter   *fun_param;
@@ -351,7 +350,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type <boolean> TriggerForSpec TriggerForType
 %type <ival>	TriggerActionTime
 %type <list>	TriggerEvents TriggerOneEvent
-%type <value>	TriggerFuncArg
+%type <node>	TriggerFuncArg
 %type <node>	TriggerWhen
 %type <str>		TransitionRelName
 %type <boolean>	TransitionRowOrTable TransitionOldOrNew
@@ -508,7 +507,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type <list>	when_clause_list
 %type <node>	opt_search_clause opt_cycle_clause
 %type <ival>	sub_type opt_materialized
-%type <value>	NumericOnly
+%type <node>	NumericOnly
 %type <list>	NumericOnly_list
 %type <alias>	alias_clause opt_alias_clause opt_alias_clause_for_join_using
 %type <list>	func_alias_clause
@@ -1696,7 +1695,7 @@ zone_value:
 					if ($3 != NIL)
 					{
 						A_Const *n = (A_Const *) linitial($3);
-						if ((n->val.val.ival & ~(INTERVAL_MASK(HOUR) | INTERVAL_MASK(MINUTE))) != 0)
+						if ((n->val.ival.val & ~(INTERVAL_MASK(HOUR) | INTERVAL_MASK(MINUTE))) != 0)
 							ereport(ERROR,
 									(errcode(ERRCODE_SYNTAX_ERROR),
 									 errmsg("time zone interval must be HOUR or HOUR TO MINUTE"),
@@ -4459,14 +4458,15 @@ opt_by:		BY
 	  ;
 
 NumericOnly:
-			FCONST								{ $$ = makeFloat($1); }
-			| '+' FCONST						{ $$ = makeFloat($2); }
+			FCONST								{ $$ = (Node *) makeFloat($1); }
+			| '+' FCONST						{ $$ = (Node *) makeFloat($2); }
 			| '-' FCONST
 				{
-					$$ = makeFloat($2);
-					doNegateFloat($$);
+					Float *f = makeFloat($2);
+					doNegateFloat(f);
+					$$ = (Node *) f;
 				}
-			| SignedIconst						{ $$ = makeInteger($1); }
+			| SignedIconst						{ $$ = (Node *) makeInteger($1); }
 		;
 
 NumericOnly_list:	NumericOnly						{ $$ = list_make1($1); }
@@ -5535,11 +5535,11 @@ TriggerFuncArgs:
 TriggerFuncArg:
 			Iconst
 				{
-					$$ = makeString(psprintf("%d", $1));
+					$$ = (Node *) makeString(psprintf("%d", $1));
 				}
-			| FCONST								{ $$ = makeString($1); }
-			| Sconst								{ $$ = makeString($1); }
-			| ColLabel								{ $$ = makeString($1); }
+			| FCONST								{ $$ = (Node *) makeString($1); }
+			| Sconst								{ $$ = (Node *) makeString($1); }
+			| ColLabel								{ $$ = (Node *) makeString($1); }
 		;
 
 OptConstrFromTable:
@@ -7773,8 +7773,8 @@ aggr_arg:	func_arg
  *
  * The return value of this production is a two-element list, in which the
  * first item is a sublist of FunctionParameter nodes (with any duplicate
- * VARIADIC item already dropped, as per above) and the second is an integer
- * Value node, containing -1 if there was no ORDER BY and otherwise the number
+ * VARIADIC item already dropped, as per above) and the second is an Integer
+ * node, containing -1 if there was no ORDER BY and otherwise the number
  * of argument declarations before the ORDER BY.  (If this number is equal
  * to the first sublist's length, then we dropped a duplicate VARIADIC item.)
  * This representation is passed as-is to CREATE AGGREGATE; for operations
@@ -16520,11 +16520,11 @@ makeStringConst(char *str, int location)
 {
 	A_Const *n = makeNode(A_Const);
 
-	n->val.type = T_String;
-	n->val.val.str = str;
+	n->val.sval.type = T_String;
+	n->val.sval.val = str;
 	n->location = location;
 
-	return (Node *)n;
+   return (Node *)n;
 }
 
 static Node *
@@ -16540,11 +16540,11 @@ makeIntConst(int val, int location)
 {
 	A_Const *n = makeNode(A_Const);
 
-	n->val.type = T_Integer;
-	n->val.val.ival = val;
+	n->val.ival.type = T_Integer;
+	n->val.ival.val = val;
 	n->location = location;
 
-	return (Node *)n;
+   return (Node *)n;
 }
 
 static Node *
@@ -16552,11 +16552,11 @@ makeFloatConst(char *str, int location)
 {
 	A_Const *n = makeNode(A_Const);
 
-	n->val.type = T_Float;
-	n->val.val.str = str;
+	n->val.fval.type = T_Float;
+	n->val.fval.val = str;
 	n->location = location;
 
-	return (Node *)n;
+   return (Node *)n;
 }
 
 static Node *
@@ -16564,11 +16564,11 @@ makeBitStringConst(char *str, int location)
 {
 	A_Const *n = makeNode(A_Const);
 
-	n->val.type = T_BitString;
-	n->val.val.str = str;
+	n->val.bsval.type = T_BitString;
+	n->val.bsval.val = str;
 	n->location = location;
 
-	return (Node *)n;
+   return (Node *)n;
 }
 
 static Node *
@@ -16576,30 +16576,30 @@ makeNullAConst(int location)
 {
 	A_Const *n = makeNode(A_Const);
 
-	n->val.type = T_Null;
+	n->isnull = true;
 	n->location = location;
 
 	return (Node *)n;
 }
 
 static Node *
-makeAConst(Value *v, int location)
+makeAConst(Node *v, int location)
 {
 	Node *n;
 
 	switch (v->type)
 	{
 		case T_Float:
-			n = makeFloatConst(v->val.str, location);
+			n = makeFloatConst(castNode(Float, v)->val, location);
 			break;
 
 		case T_Integer:
-			n = makeIntConst(v->val.ival, location);
+			n = makeIntConst(castNode(Integer, v)->val, location);
 			break;
 
 		case T_String:
 		default:
-			n = makeStringConst(v->val.str, location);
+			n = makeStringConst(castNode(String, v)->val, location);
 			break;
 	}
 
@@ -16612,13 +16612,9 @@ makeAConst(Value *v, int location)
 static Node *
 makeBoolAConst(bool state, int location)
 {
-	A_Const *n = makeNode(A_Const);
-
-	n->val.type = T_String;
-	n->val.val.str = (state ? "t" : "f");
-	n->location = location;
-
-	return makeTypeCast((Node *)n, SystemTypeName("bool"), -1);
+	return makeStringConstCast((state ? "t" : "f"),
+							   location,
+							   SystemTypeName("bool"));
 }
 
 /* makeRoleSpec
@@ -16733,7 +16729,7 @@ makeOrderedSetArgs(List *directargs, List *orderedargs,
 				   core_yyscan_t yyscanner)
 {
 	FunctionParameter *lastd = (FunctionParameter *) llast(directargs);
-	Value	   *ndirectargs;
+	Integer	   *ndirectargs;
 
 	/* No restriction unless last direct arg is VARIADIC */
 	if (lastd->mode == FUNC_PARAM_VARIADIC)
@@ -16890,14 +16886,14 @@ doNegate(Node *n, int location)
 		/* report the constant's location as that of the '-' sign */
 		con->location = location;
 
-		if (con->val.type == T_Integer)
+		if (IsA(&con->val, Integer))
 		{
-			con->val.val.ival = -con->val.val.ival;
+			con->val.ival.val = -con->val.ival.val;
 			return n;
 		}
-		if (con->val.type == T_Float)
+		if (IsA(&con->val, Float))
 		{
-			doNegateFloat(&con->val);
+			doNegateFloat(&con->val.fval);
 			return n;
 		}
 	}
@@ -16906,17 +16902,16 @@ doNegate(Node *n, int location)
 }
 
 static void
-doNegateFloat(Value *v)
+doNegateFloat(Float *v)
 {
-	char   *oldval = v->val.str;
+	char   *oldval = v->val;
 
-	Assert(IsA(v, Float));
 	if (*oldval == '+')
 		oldval++;
 	if (*oldval == '-')
-		v->val.str = oldval+1;	/* just strip the '-' */
+		v->val = oldval+1;	/* just strip the '-' */
 	else
-		v->val.str = psprintf("-%s", oldval);
+		v->val = psprintf("-%s", oldval);
 }
 
 static Node *
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index b3f151d33b5..078029ba1f7 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -852,7 +852,7 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf)
 			{
 				foreach(lc2, ns_names)
 				{
-					Value	   *ns_node = (Value *) lfirst(lc2);
+					String	   *ns_node = lfirst_node(String, lc2);
 
 					if (ns_node == NULL)
 						continue;
@@ -1240,7 +1240,7 @@ transformFromClauseItem(ParseState *pstate, Node *n,
 			foreach(lx, l_colnames)
 			{
 				char	   *l_colname = strVal(lfirst(lx));
-				Value	   *m_name = NULL;
+				String	   *m_name = NULL;
 
 				if (l_colname[0] == '\0')
 					continue;	/* ignore dropped columns */
@@ -1785,7 +1785,7 @@ transformLimitClause(ParseState *pstate, Node *clause,
 	 * unadorned NULL that's not accepted back by the grammar.
 	 */
 	if (exprKind == EXPR_KIND_LIMIT && limitOption == LIMIT_OPTION_WITH_TIES &&
-		IsA(clause, A_Const) && ((A_Const *) clause)->val.type == T_Null)
+		IsA(clause, A_Const) && castNode(A_Const, clause)->isnull)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_ROW_COUNT_IN_LIMIT_CLAUSE),
 				 errmsg("row count cannot be null in FETCH FIRST ... WITH TIES clause")));
@@ -1998,20 +1998,19 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist,
 	}
 	if (IsA(node, A_Const))
 	{
-		Value	   *val = &((A_Const *) node)->val;
-		int			location = ((A_Const *) node)->location;
+		A_Const	   *aconst = castNode(A_Const, node);
 		int			targetlist_pos = 0;
 		int			target_pos;
 
-		if (!IsA(val, Integer))
+		if (!IsA(&aconst->val, Integer))
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
 			/* translator: %s is name of a SQL construct, eg ORDER BY */
 					 errmsg("non-integer constant in %s",
 							ParseExprKindName(exprKind)),
-					 parser_errposition(pstate, location)));
+					 parser_errposition(pstate, aconst->location)));
 
-		target_pos = intVal(val);
+		target_pos = intVal(&aconst->val);
 		foreach(tl, *tlist)
 		{
 			TargetEntry *tle = (TargetEntry *) lfirst(tl);
@@ -2031,7 +2030,7 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist,
 		/* translator: %s is name of a SQL construct, eg ORDER BY */
 				 errmsg("%s position %d is not in select list",
 						ParseExprKindName(exprKind), target_pos),
-				 parser_errposition(pstate, location)));
+				 parser_errposition(pstate, aconst->location)));
 	}
 
 	/*
diff --git a/src/backend/parser/parse_cte.c b/src/backend/parser/parse_cte.c
index f6ae96333af..2f51caf76cf 100644
--- a/src/backend/parser/parse_cte.c
+++ b/src/backend/parser/parse_cte.c
@@ -393,7 +393,7 @@ analyzeCTE(ParseState *pstate, CommonTableExpr *cte)
 
 		foreach(lc, cte->search_clause->search_col_list)
 		{
-			Value	   *colname = lfirst(lc);
+			String	   *colname = lfirst_node(String, lc);
 
 			if (!list_member(cte->ctecolnames, colname))
 				ereport(ERROR,
@@ -428,7 +428,7 @@ analyzeCTE(ParseState *pstate, CommonTableExpr *cte)
 
 		foreach(lc, cte->cycle_clause->cycle_col_list)
 		{
-			Value	   *colname = lfirst(lc);
+			String	   *colname = lfirst_node(String, lc);
 
 			if (!list_member(cte->ctecolnames, colname))
 				ereport(ERROR,
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index f928c323113..2d1a4771540 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -130,13 +130,8 @@ transformExprRecurse(ParseState *pstate, Node *expr)
 			break;
 
 		case T_A_Const:
-			{
-				A_Const    *con = (A_Const *) expr;
-				Value	   *val = &con->val;
-
-				result = (Node *) make_const(pstate, val, con->location);
-				break;
-			}
+			result = (Node *) make_const(pstate, (A_Const *) expr);
+			break;
 
 		case T_A_Indirection:
 			result = transformIndirection(pstate, (A_Indirection *) expr);
@@ -855,7 +850,7 @@ exprIsNullConstant(Node *arg)
 	{
 		A_Const    *con = (A_Const *) arg;
 
-		if (con->val.type == T_Null)
+		if (con->isnull)
 			return true;
 	}
 	return false;
@@ -1626,7 +1621,7 @@ transformCaseExpr(ParseState *pstate, CaseExpr *c)
 	{
 		A_Const    *n = makeNode(A_Const);
 
-		n->val.type = T_Null;
+		n->isnull = true;
 		n->location = -1;
 		defresult = (Node *) n;
 	}
diff --git a/src/backend/parser/parse_node.c b/src/backend/parser/parse_node.c
index 17c900da31b..8cfe6f67c04 100644
--- a/src/backend/parser/parse_node.c
+++ b/src/backend/parser/parse_node.c
@@ -333,7 +333,7 @@ transformContainerSubscripts(ParseState *pstate,
 /*
  * make_const
  *
- *	Convert a Value node (as returned by the grammar) to a Const node
+ *	Convert an A_Const node (as returned by the grammar) to a Const node
  *	of the "natural" type for the constant.  Note that this routine is
  *	only used when there is no explicit cast for the constant, so we
  *	have to guess what type is wanted.
@@ -349,7 +349,7 @@ transformContainerSubscripts(ParseState *pstate,
  *	too many examples that fail if we try.
  */
 Const *
-make_const(ParseState *pstate, Value *value, int location)
+make_const(ParseState *pstate, A_Const *aconst)
 {
 	Const	   *con;
 	Datum		val;
@@ -359,10 +359,24 @@ make_const(ParseState *pstate, Value *value, int location)
 	bool		typebyval;
 	ParseCallbackState pcbstate;
 
-	switch (nodeTag(value))
+	if (aconst->isnull)
+	{
+		/* return a null const */
+		con = makeConst(UNKNOWNOID,
+						-1,
+						InvalidOid,
+						-2,
+						(Datum) 0,
+						true,
+						false);
+		con->location = aconst->location;
+		return con;
+	}
+
+	switch (nodeTag(&aconst->val))
 	{
 		case T_Integer:
-			val = Int32GetDatum(intVal(value));
+			val = Int32GetDatum(aconst->val.ival.val);
 
 			typeid = INT4OID;
 			typelen = sizeof(int32);
@@ -371,7 +385,7 @@ make_const(ParseState *pstate, Value *value, int location)
 
 		case T_Float:
 			/* could be an oversize integer as well as a float ... */
-			if (scanint8(strVal(value), true, &val64))
+			if (scanint8(aconst->val.fval.val, true, &val64))
 			{
 				/*
 				 * It might actually fit in int32. Probably only INT_MIN can
@@ -399,9 +413,9 @@ make_const(ParseState *pstate, Value *value, int location)
 			else
 			{
 				/* arrange to report location if numeric_in() fails */
-				setup_parser_errposition_callback(&pcbstate, pstate, location);
+				setup_parser_errposition_callback(&pcbstate, pstate, aconst->location);
 				val = DirectFunctionCall3(numeric_in,
-										  CStringGetDatum(strVal(value)),
+										  CStringGetDatum(aconst->val.fval.val),
 										  ObjectIdGetDatum(InvalidOid),
 										  Int32GetDatum(-1));
 				cancel_parser_errposition_callback(&pcbstate);
@@ -418,7 +432,7 @@ make_const(ParseState *pstate, Value *value, int location)
 			 * We assume here that UNKNOWN's internal representation is the
 			 * same as CSTRING
 			 */
-			val = CStringGetDatum(strVal(value));
+			val = CStringGetDatum(aconst->val.sval.val);
 
 			typeid = UNKNOWNOID;	/* will be coerced later */
 			typelen = -2;		/* cstring-style varwidth type */
@@ -427,9 +441,9 @@ make_const(ParseState *pstate, Value *value, int location)
 
 		case T_BitString:
 			/* arrange to report location if bit_in() fails */
-			setup_parser_errposition_callback(&pcbstate, pstate, location);
+			setup_parser_errposition_callback(&pcbstate, pstate, aconst->location);
 			val = DirectFunctionCall3(bit_in,
-									  CStringGetDatum(strVal(value)),
+									  CStringGetDatum(aconst->val.bsval.val),
 									  ObjectIdGetDatum(InvalidOid),
 									  Int32GetDatum(-1));
 			cancel_parser_errposition_callback(&pcbstate);
@@ -438,20 +452,8 @@ make_const(ParseState *pstate, Value *value, int location)
 			typebyval = false;
 			break;
 
-		case T_Null:
-			/* return a null const */
-			con = makeConst(UNKNOWNOID,
-							-1,
-							InvalidOid,
-							-2,
-							(Datum) 0,
-							true,
-							false);
-			con->location = location;
-			return con;
-
 		default:
-			elog(ERROR, "unrecognized node type: %d", (int) nodeTag(value));
+			elog(ERROR, "unrecognized node type: %d", (int) nodeTag(&aconst->val));
 			return NULL;		/* keep compiler quiet */
 	}
 
@@ -462,7 +464,7 @@ make_const(ParseState *pstate, Value *value, int location)
 					val,
 					false,
 					typebyval);
-	con->location = location;
+	con->location = aconst->location;
 
 	return con;
 }
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 74659190447..c5c3f26ecf1 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1140,7 +1140,7 @@ buildRelationAliases(TupleDesc tupdesc, Alias *alias, Alias *eref)
 	for (varattno = 0; varattno < maxattrs; varattno++)
 	{
 		Form_pg_attribute attr = TupleDescAttr(tupdesc, varattno);
-		Value	   *attrname;
+		String	   *attrname;
 
 		if (attr->attisdropped)
 		{
@@ -1153,7 +1153,7 @@ buildRelationAliases(TupleDesc tupdesc, Alias *alias, Alias *eref)
 		else if (aliaslc)
 		{
 			/* Use the next user-supplied alias */
-			attrname = (Value *) lfirst(aliaslc);
+			attrname = lfirst_node(String, aliaslc);
 			aliaslc = lnext(aliaslist, aliaslc);
 			alias->colnames = lappend(alias->colnames, attrname);
 		}
@@ -3052,7 +3052,7 @@ expandNSItemVars(ParseNamespaceItem *nsitem,
 	colindex = 0;
 	foreach(lc, nsitem->p_names->colnames)
 	{
-		Value	   *colnameval = (Value *) lfirst(lc);
+		String	   *colnameval = lfirst(lc);
 		const char *colname = strVal(colnameval);
 		ParseNamespaceColumn *nscol = nsitem->p_nscolumns + colindex;
 
diff --git a/src/backend/parser/parse_type.c b/src/backend/parser/parse_type.c
index abe131ebebf..31b07ad5aed 100644
--- a/src/backend/parser/parse_type.c
+++ b/src/backend/parser/parse_type.c
@@ -382,13 +382,17 @@ typenameTypeMod(ParseState *pstate, const TypeName *typeName, Type typ)
 
 			if (IsA(&ac->val, Integer))
 			{
-				cstr = psprintf("%ld", (long) ac->val.val.ival);
+				cstr = psprintf("%ld", (long) ac->val.ival.val);
 			}
-			else if (IsA(&ac->val, Float) ||
-					 IsA(&ac->val, String))
+			else if (IsA(&ac->val, Float))
 			{
-				/* we can just use the str field directly. */
-				cstr = ac->val.val.str;
+				/* we can just use the string representation directly. */
+				cstr = ac->val.fval.val;
+			}
+			else if (IsA(&ac->val, String))
+			{
+				/* we can just use the string representation directly. */
+				cstr = ac->val.sval.val;
 			}
 		}
 		else if (IsA(tm, ColumnRef))
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index e5eefdbd43c..1d3ee53244d 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -602,8 +602,8 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column)
 		 */
 		qstring = quote_qualified_identifier(snamespace, sname);
 		snamenode = makeNode(A_Const);
-		snamenode->val.type = T_String;
-		snamenode->val.val.str = qstring;
+		snamenode->val.node.type = T_String;
+		snamenode->val.sval.val = qstring;
 		snamenode->location = -1;
 		castnode = makeNode(TypeCast);
 		castnode->typeName = SystemTypeName("regclass");