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

5 files changed, 92 insertions, 72 deletions

diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 56d13ff0229..a692eb7b098 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -291,12 +291,10 @@ typedef enum NodeTag
 	/*
 	 * TAGS FOR VALUE NODES (value.h)
 	 */
-	T_Value,
 	T_Integer,
 	T_Float,
 	T_String,
 	T_BitString,
-	T_Null,
 
 	/*
 	 * TAGS FOR LIST NODES (pg_list.h)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 743e5aa4f34..45e4f2a16e6 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -218,7 +218,7 @@ typedef struct Query
 typedef struct TypeName
 {
 	NodeTag		type;
-	List	   *names;			/* qualified name (list of Value strings) */
+	List	   *names;			/* qualified name (list of String nodes) */
 	Oid			typeOid;		/* type identified by OID */
 	bool		setof;			/* is a set? */
 	bool		pct_type;		/* %TYPE specified? */
@@ -231,7 +231,7 @@ typedef struct TypeName
 /*
  * ColumnRef - specifies a reference to a column, or possibly a whole tuple
  *
- * The "fields" list must be nonempty.  It can contain string Value nodes
+ * The "fields" list must be nonempty.  It can contain String nodes
  * (representing names) and A_Star nodes (representing occurrence of a '*').
  * Currently, A_Star must appear only as the last list element --- the grammar
  * is responsible for enforcing this!
@@ -244,7 +244,7 @@ typedef struct TypeName
 typedef struct ColumnRef
 {
 	NodeTag		type;
-	List	   *fields;			/* field names (Value strings) or A_Star */
+	List	   *fields;			/* field names (String nodes) or A_Star */
 	int			location;		/* token location, or -1 if unknown */
 } ColumnRef;
 
@@ -295,7 +295,19 @@ typedef struct A_Expr
 typedef struct A_Const
 {
 	NodeTag		type;
-	Value		val;			/* value (includes type info, see value.h) */
+	/*
+	 * Value nodes are inline for performance.  You can treat 'val' as a node,
+	 * as in IsA(&val, Integer).  'val' is not valid if isnull is true.
+	 */
+	union ValUnion
+	{
+		Node		node;
+		Integer		ival;
+		Float		fval;
+		String		sval;
+		BitString	bsval;
+	}			val;
+	bool		isnull;			/* SQL NULL constant */
 	int			location;		/* token location, or -1 if unknown */
 } A_Const;
 
@@ -400,7 +412,7 @@ typedef struct A_Indices
  * A_Indirection - select a field and/or array element from an expression
  *
  * The indirection list can contain A_Indices nodes (representing
- * subscripting), string Value nodes (representing field selection --- the
+ * subscripting), String nodes (representing field selection --- the
  * string value is the name of the field to select), and A_Star nodes
  * (representing selection of all fields of a composite type).
  * For example, a complex selection operation like
@@ -744,7 +756,7 @@ typedef struct DefElem
 	NodeTag		type;
 	char	   *defnamespace;	/* NULL if unqualified name */
 	char	   *defname;
-	Node	   *arg;			/* a (Value *) or a (TypeName *) */
+	Node	   *arg;			/* typically Integer, Float, String, or TypeName */
 	DefElemAction defaction;	/* unspecified action, or SET/ADD/DROP */
 	int			location;		/* token location, or -1 if unknown */
 } DefElem;
@@ -2015,7 +2027,7 @@ typedef struct GrantStmt
 	GrantTargetType targtype;	/* type of the grant target */
 	ObjectType	objtype;		/* kind of object being operated on */
 	List	   *objects;		/* list of RangeVar nodes, ObjectWithArgs
-								 * nodes, or plain names (as Value strings) */
+								 * nodes, or plain names (as String values) */
 	List	   *privileges;		/* list of AccessPriv nodes */
 	/* privileges == NIL denotes ALL PRIVILEGES */
 	List	   *grantees;		/* list of RoleSpec nodes */
@@ -2061,7 +2073,7 @@ typedef struct AccessPriv
 {
 	NodeTag		type;
 	char	   *priv_name;		/* string name of privilege */
-	List	   *cols;			/* list of Value strings */
+	List	   *cols;			/* list of String */
 } AccessPriv;
 
 /* ----------------------
@@ -2070,7 +2082,7 @@ typedef struct AccessPriv
  * Note: because of the parsing ambiguity with the GRANT <privileges>
  * statement, granted_roles is a list of AccessPriv; the execution code
  * should complain if any column lists appear.  grantee_roles is a list
- * of role names, as Value strings.
+ * of role names, as String values.
  * ----------------------
  */
 typedef struct GrantRoleStmt
@@ -2531,7 +2543,7 @@ typedef struct CreateTrigStmt
 	char	   *trigname;		/* TRIGGER's name */
 	RangeVar   *relation;		/* relation trigger is on */
 	List	   *funcname;		/* qual. name of function to call */
-	List	   *args;			/* list of (T_String) Values or NIL */
+	List	   *args;			/* list of String or NIL */
 	bool		row;			/* ROW/STATEMENT */
 	/* timing uses the TRIGGER_TYPE bits defined in catalog/pg_trigger.h */
 	int16		timing;			/* BEFORE, AFTER, or INSTEAD */
@@ -2667,7 +2679,7 @@ typedef struct DefineStmt
 	NodeTag		type;
 	ObjectType	kind;			/* aggregate, operator, type */
 	bool		oldstyle;		/* hack to signal old CREATE AGG syntax */
-	List	   *defnames;		/* qualified name (list of Value strings) */
+	List	   *defnames;		/* qualified name (list of String) */
 	List	   *args;			/* a list of TypeName (if needed) */
 	List	   *definition;		/* a list of DefElem */
 	bool		if_not_exists;	/* just do nothing if it already exists? */
@@ -2681,7 +2693,7 @@ typedef struct DefineStmt
 typedef struct CreateDomainStmt
 {
 	NodeTag		type;
-	List	   *domainname;		/* qualified name (list of Value strings) */
+	List	   *domainname;		/* qualified name (list of String) */
 	TypeName   *typeName;		/* the base type */
 	CollateClause *collClause;	/* untransformed COLLATE spec, if any */
 	List	   *constraints;	/* constraints (list of Constraint nodes) */
@@ -2694,7 +2706,7 @@ typedef struct CreateDomainStmt
 typedef struct CreateOpClassStmt
 {
 	NodeTag		type;
-	List	   *opclassname;	/* qualified name (list of Value strings) */
+	List	   *opclassname;	/* qualified name (list of String) */
 	List	   *opfamilyname;	/* qualified name (ditto); NIL if omitted */
 	char	   *amname;			/* name of index AM opclass is for */
 	TypeName   *datatype;		/* datatype of indexed column */
@@ -2726,7 +2738,7 @@ typedef struct CreateOpClassItem
 typedef struct CreateOpFamilyStmt
 {
 	NodeTag		type;
-	List	   *opfamilyname;	/* qualified name (list of Value strings) */
+	List	   *opfamilyname;	/* qualified name (list of String) */
 	char	   *amname;			/* name of index AM opfamily is for */
 } CreateOpFamilyStmt;
 
@@ -2737,7 +2749,7 @@ typedef struct CreateOpFamilyStmt
 typedef struct AlterOpFamilyStmt
 {
 	NodeTag		type;
-	List	   *opfamilyname;	/* qualified name (list of Value strings) */
+	List	   *opfamilyname;	/* qualified name (list of String) */
 	char	   *amname;			/* name of index AM opfamily is for */
 	bool		isDrop;			/* ADD or DROP the items? */
 	List	   *items;			/* List of CreateOpClassItem nodes */
@@ -2908,8 +2920,8 @@ typedef struct IndexStmt
 typedef struct CreateStatsStmt
 {
 	NodeTag		type;
-	List	   *defnames;		/* qualified name (list of Value strings) */
-	List	   *stat_types;		/* stat types (list of Value strings) */
+	List	   *defnames;		/* qualified name (list of String) */
+	List	   *stat_types;		/* stat types (list of String) */
 	List	   *exprs;			/* expressions to build statistics on */
 	List	   *relations;		/* rels to build stats on (list of RangeVar) */
 	char	   *stxcomment;		/* comment to apply to stats, or NULL */
@@ -2939,7 +2951,7 @@ typedef struct StatsElem
 typedef struct AlterStatsStmt
 {
 	NodeTag		type;
-	List	   *defnames;		/* qualified name (list of Value strings) */
+	List	   *defnames;		/* qualified name (list of String) */
 	int			stxstattarget;	/* statistics target */
 	bool		missing_ok;		/* skip error if statistics object is missing */
 } AlterStatsStmt;
@@ -3061,7 +3073,7 @@ typedef struct AlterObjectDependsStmt
 	ObjectType	objectType;		/* OBJECT_FUNCTION, OBJECT_TRIGGER, etc */
 	RangeVar   *relation;		/* in case a table is involved */
 	Node	   *object;			/* name of the object */
-	Value	   *extname;		/* extension name */
+	String	   *extname;		/* extension name */
 	bool		remove;			/* set true to remove dep rather than add */
 } AlterObjectDependsStmt;
 
@@ -3207,8 +3219,8 @@ typedef struct CompositeTypeStmt
 typedef struct CreateEnumStmt
 {
 	NodeTag		type;
-	List	   *typeName;		/* qualified name (list of Value strings) */
-	List	   *vals;			/* enum values (list of Value strings) */
+	List	   *typeName;		/* qualified name (list of String) */
+	List	   *vals;			/* enum values (list of String) */
 } CreateEnumStmt;
 
 /* ----------------------
@@ -3218,7 +3230,7 @@ typedef struct CreateEnumStmt
 typedef struct CreateRangeStmt
 {
 	NodeTag		type;
-	List	   *typeName;		/* qualified name (list of Value strings) */
+	List	   *typeName;		/* qualified name (list of String) */
 	List	   *params;			/* range parameters (list of DefElem) */
 } CreateRangeStmt;
 
@@ -3229,7 +3241,7 @@ typedef struct CreateRangeStmt
 typedef struct AlterEnumStmt
 {
 	NodeTag		type;
-	List	   *typeName;		/* qualified name (list of Value strings) */
+	List	   *typeName;		/* qualified name (list of String) */
 	char	   *oldVal;			/* old enum value's name, if renaming */
 	char	   *newVal;			/* new enum value's name */
 	char	   *newValNeighbor; /* neighboring enum value, if specified */
@@ -3591,7 +3603,7 @@ typedef struct ReassignOwnedStmt
 typedef struct AlterTSDictionaryStmt
 {
 	NodeTag		type;
-	List	   *dictname;		/* qualified name (list of Value strings) */
+	List	   *dictname;		/* qualified name (list of String) */
 	List	   *options;		/* List of DefElem nodes */
 } AlterTSDictionaryStmt;
 
@@ -3611,14 +3623,14 @@ typedef struct AlterTSConfigurationStmt
 {
 	NodeTag		type;
 	AlterTSConfigType kind;		/* ALTER_TSCONFIG_ADD_MAPPING, etc */
-	List	   *cfgname;		/* qualified name (list of Value strings) */
+	List	   *cfgname;		/* qualified name (list of String) */
 
 	/*
 	 * dicts will be non-NIL if ADD/ALTER MAPPING was specified. If dicts is
 	 * NIL, but tokentype isn't, DROP MAPPING was specified.
 	 */
-	List	   *tokentype;		/* list of Value strings */
-	List	   *dicts;			/* list of list of Value strings */
+	List	   *tokentype;		/* list of String */
+	List	   *dicts;			/* list of list of String */
 	bool		override;		/* if true - remove old variant */
 	bool		replace;		/* if true - replace dictionary by another */
 	bool		missing_ok;		/* for DROP - skip error if missing? */
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index c04282f91fd..7b125904b4d 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -32,7 +32,7 @@
  *	  specifies an alias for a range variable; the alias might also
  *	  specify renaming of columns within the table.
  *
- * Note: colnames is a list of Value nodes (always strings).  In Alias structs
+ * Note: colnames is a list of String nodes.  In Alias structs
  * associated with RTEs, there may be entries corresponding to dropped
  * columns; these are normally empty strings ("").  See parsenodes.h for info.
  */
@@ -76,7 +76,7 @@ typedef struct RangeVar
 /*
  * TableFunc - node for a table function, such as XMLTABLE.
  *
- * Entries in the ns_names list are either string Value nodes containing
+ * Entries in the ns_names list are either String nodes containing
  * literal namespace names, or NULL pointers to represent DEFAULT.
  */
 typedef struct TableFunc
@@ -1227,7 +1227,7 @@ typedef struct XmlExpr
 	XmlExprOp	op;				/* xml function ID */
 	char	   *name;			/* name in xml(NAME foo ...) syntaxes */
 	List	   *named_args;		/* non-XML expressions for xml_attributes */
-	List	   *arg_names;		/* parallel list of Value strings */
+	List	   *arg_names;		/* parallel list of String values */
 	List	   *args;			/* list of expressions */
 	XmlOptionType xmloption;	/* DOCUMENT or CONTENT */
 	Oid			type;			/* target type/typmod for XMLSERIALIZE */
diff --git a/src/include/nodes/value.h b/src/include/nodes/value.h
index b28928de545..8b71b510eb8 100644
--- a/src/include/nodes/value.h
+++ b/src/include/nodes/value.h
@@ -1,7 +1,7 @@
 /*-------------------------------------------------------------------------
  *
  * value.h
- *	  interface for Value nodes
+ *	  interface for value nodes
  *
  *
  * Copyright (c) 2003-2021, PostgreSQL Global Development Group
@@ -16,46 +16,57 @@
 
 #include "nodes/nodes.h"
 
-/*----------------------
- *		Value node
+/*
+ * The node types Integer, Float, String, and BitString are used to represent
+ * literals in the lexer and are also used to pass constants around in the
+ * parser.  One difference between these node types and, say, a plain int or
+ * char * is that the nodes can be put into a List.
  *
- * The same Value struct is used for five node types: T_Integer,
- * T_Float, T_String, T_BitString, T_Null.
- *
- * Integral values are actually represented by a machine integer,
- * but both floats and strings are represented as strings.
- * Using T_Float as the node type simply indicates that
- * the contents of the string look like a valid numeric literal.
- *
- * (Before Postgres 7.0, we used a double to represent T_Float,
- * but that creates loss-of-precision problems when the value is
- * ultimately destined to be converted to NUMERIC.  Since Value nodes
- * are only used in the parsing process, not for runtime data, it's
- * better to use the more general representation.)
- *
- * Note that an integer-looking string will get lexed as T_Float if
- * the value is too large to fit in an 'int'.
+ * (There used to be a Value node, which encompassed all these different node types.  Hence the name of this file.)
+ */
+
+typedef struct Integer
+{
+	NodeTag		type;
+	int			val;
+} Integer;
+
+/*
+ * Float is internally represented as string.  Using T_Float as the node type
+ * simply indicates that the contents of the string look like a valid numeric
+ * literal.  The value might end up being converted to NUMERIC, so we can't
+ * store it internally as a C double, since that could lose precision.  Since
+ * these nodes are generally only used in the parsing process, not for runtime
+ * data, it's better to use the more general representation.
  *
- * Nulls, of course, don't need the value part at all.
- *----------------------
+ * Note that an integer-looking string will get lexed as T_Float if the value
+ * is too large to fit in an 'int'.
  */
-typedef struct Value
+typedef struct Float
 {
-	NodeTag		type;			/* tag appropriately (eg. T_String) */
-	union ValUnion
-	{
-		int			ival;		/* machine integer */
-		char	   *str;		/* string */
-	}			val;
-} Value;
-
-#define intVal(v)		(((Value *)(v))->val.ival)
-#define floatVal(v)		atof(((Value *)(v))->val.str)
-#define strVal(v)		(((Value *)(v))->val.str)
-
-extern Value *makeInteger(int i);
-extern Value *makeFloat(char *numericStr);
-extern Value *makeString(char *str);
-extern Value *makeBitString(char *str);
+	NodeTag		type;
+	char	   *val;
+} Float;
+
+typedef struct String
+{
+	NodeTag		type;
+	char	   *val;
+} String;
+
+typedef struct BitString
+{
+	NodeTag		type;
+	char	   *val;
+} BitString;
+
+#define intVal(v)		(castNode(Integer, v)->val)
+#define floatVal(v)		atof(castNode(Float, v)->val)
+#define strVal(v)		(castNode(String, v)->val)
+
+extern Integer *makeInteger(int i);
+extern Float *makeFloat(char *numericStr);
+extern String *makeString(char *str);
+extern BitString *makeBitString(char *str);
 
 #endif							/* VALUE_H */
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index 1500de2dd08..ee179082cef 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -333,7 +333,6 @@ extern SubscriptingRef *transformContainerSubscripts(ParseState *pstate,
 													 int32 containerTypMod,
 													 List *indirection,
 													 bool isAssignment);
-
-extern Const *make_const(ParseState *pstate, Value *value, int location);
+extern Const *make_const(ParseState *pstate, A_Const *aconst);
 
 #endif							/* PARSE_NODE_H */