Re: Remove Value node struct
On 08.09.21 04:04, Kyotaro Horiguchi wrote: At Tue, 7 Sep 2021 11:22:24 +0200, Peter Eisentraut wrote in On 30.08.21 04:13, Kyotaro Horiguchi wrote: + else if (IsA(obj, Integer)) + _outInteger(str, (Integer *) obj); + else if (IsA(obj, Float)) + _outFloat(str, (Float *) obj); I felt that the type enames are a bit confusing as they might be too generic, or too close with the corresponding binary types. - Node *arg;/* a (Value *) or a (TypeName *) */ + Node *arg; Mmm. It's a bit pity that we lose the generic name for the value nodes. Not sure what you mean here. The member arg loses the information on what kind of nodes are to be stored there. Concretely it just removes the comment "a (Value *) or a (TypeName *)". If the (Value *) were expanded in a straight way, the comment would be "a (Integer *), (Float *), (String *), (BitString *), or (TypeName *)". I supposed that the member loses the comment because it become too long. Ok, I added the comment back in in a modified form. The patches have been committed now. Thanks.
Re: Remove Value node struct
At Tue, 7 Sep 2021 11:22:24 +0200, Peter Eisentraut wrote in > On 30.08.21 04:13, Kyotaro Horiguchi wrote: > > + else if (IsA(obj, Integer)) > > + _outInteger(str, (Integer *) obj); > > + else if (IsA(obj, Float)) > > + _outFloat(str, (Float *) obj); > > I felt that the type enames are a bit confusing as they might be too > > generic, or too close with the corresponding binary types. > > - Node *arg;/* a (Value *) or a (TypeName > > *) */ > > + Node *arg; > > Mmm. It's a bit pity that we lose the generic name for the value > > nodes. > > Not sure what you mean here. The member arg loses the information on what kind of nodes are to be stored there. Concretely it just removes the comment "a (Value *) or a (TypeName *)". If the (Value *) were expanded in a straight way, the comment would be "a (Integer *), (Float *), (String *), (BitString *), or (TypeName *)". I supposed that the member loses the comment because it become too long. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Remove Value node struct
+ char *type = strVal(lfirst(cell));
if (strcmp(type, "ndistinct") == 0)
{
--
2.33.0
From 6eb5ddb6b46446731df38ac5288df2feb17efd7c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 7 Sep 2021 11:11:28 +0200
Subject: [PATCH v2 2/2] 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.
---
contrib/postgres_fdw/postgres_fdw.c | 12 +--
src/backend/catalog/namespace.c | 4 +-
src/backend/catalog/objectaddress.c | 6 +-
src/backend/catalog/pg_enum.c| 2 +-
src/backend/commands/copy.c | 5 +-
src/backend/commands/define.c| 9 +-
src/backend/commands/tsearchcmds.c | 4 +-
src/backend/executor/nodeTableFuncscan.c | 2 +-
src/backend/nodes/README | 2 +-
src/backend/nodes/copyfuncs.c| 107 ++-
src/backend/nodes/equalfuncs.c | 55 +++-
src/backend/nodes/nodeFuncs.c| 1 -
src/backend/nodes/outfuncs.c | 93 ++--
src/backend/nodes/value.c| 32 +++
src/backend/parser/gram.y| 97 ++--
src/backend/parser/parse_clause.c| 17 ++--
src/backend/parser/parse_cte.c | 4 +-
src/backend/parser/parse_expr.c | 13 +--
src/backend/parser/parse_node.c | 50 ++-
src/backend/parser/parse_relation.c | 6 +-
src/backend/parser/parse_type.c | 14 +--
src/backend/parser/parse_utilcmd.c | 4 +-
src/backend/utils/adt/oid.c | 2 +-
src/backend/utils/adt/ruleutils.c| 2 +-
src/backend/utils/misc/guc.c | 3 +-
src/include/nodes/nodes.h| 2 -
src/include/nodes/parsenodes.h | 66 --
src/include/nodes/primnodes.h| 6 +-
src/include/nodes/value.h| 87 ++
src/include/parser/parse_node.h | 3 +-
30 files changed, 373 insertions(+), 337 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c
b/contrib/postgres_fdw/postgres_fdw.c
index 4bdab30a73..76d4fea21c 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -101,9 +101,9 @@ enum FdwModifyPrivateIndex
FdwModifyPrivateUpdateSql,
/* Integer list of target attribute numbers for INSERT/UPDATE */
FdwModifyPrivateTargetAttnums,
- /* Length till the end of VALUES clause (as an integer Value node) */
+ /* Length till the end of VALUES clause (as an Integer node) */
FdwModifyPrivateLen,
- /* has-returning flag (as an integer Value node) */
+ /* has-returning flag (as an Integer node) */
FdwModifyPrivateHasReturning,
/* Integer list of attribute numbers retrieved by RETURNING */
FdwModifyPrivateRetrievedAttrs
@@ -122,11 +122,11 @@ enum FdwDirectModifyPrivateIndex
{
/* SQL statement to execute remotely (as a String node) */
FdwDirectModifyPrivateUpdateSql,
- /* has-returning flag (as an integer Value node) */
+ /* has-returning flag (as an Integer node) */
FdwDirectModifyPrivateHasReturning,
/* Integer list of attribute numbers retrieved by RETURNING */
FdwDirectModifyPrivateRetrievedAttrs,
- /* set-processed flag (as an integer Value node) */
+ /* set-processed flag (as an Integer node) */
FdwDirectModifyPrivateSetProcessed
};
@@ -280,9 +280,9 @@ typedef struct PgFdwAnalyzeState
*/
enum FdwPathPrivateIndex
{
- /* has-final-sort flag (as an integer Value node) */
+ /* has-final-sort flag (as an Integer node) */
FdwPathPrivateHasFinalSort,
- /* has-limit flag (as an integer Value node) */
+ /* has-limit flag (as an Integer node) */
FdwPathPrivateHasLimit
};
diff --git a/src/backend/catalog/namesp
Re: Remove Value node struct
Agree to the motive and +1 for the concept.
At Wed, 25 Aug 2021 15:00:13 +0100, Dagfinn Ilmari Mannsåker
wrote in
> However, the patch adds:
>
> > +typedef struct Null
> > +{
> > + NodeTag type;
> > + char *val;
> > +} Null;
>
> which doesn't seem to be used anywhere. Is that a leftoverf from an
> intermediate development stage?
+1 Looks like so, it can be simply removed.
0001 looks fine to me.
0002:
there's an "integer Value node" in gram.y: 7776.
- n = makeFloatConst(v->val.str, location);
+ n = (Node *) makeFloatConst(castNode(Float, v)->val,
location);
makeFloatConst is Node* so the cast doesn't seem needed. The same can
be said for Int and String Consts. This looks like a confustion with
makeInteger and friends.
+ else if (IsA(obj, Integer))
+ _outInteger(str, (Integer *) obj);
+ else if (IsA(obj, Float))
+ _outFloat(str, (Float *) obj);
I felt that the type enames are a bit confusing as they might be too
generic, or too close with the corresponding binary types.
- Node *arg;/* a (Value *) or a (TypeName
*) */
+ Node *arg;
Mmm. It's a bit pity that we lose the generic name for the value nodes.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Remove Value node struct
On Wed, Aug 25, 2021 at 9:49 PM Robert Haas wrote: > > On Wed, Aug 25, 2021 at 9:20 AM Peter Eisentraut > wrote: > > 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. > > +1. I noticed this years ago and never thought of doing anything about > it. I'm glad you did think of it... +1, it also bothered me in the past.
Re: Remove Value node struct
Peter Eisentraut writes:
> While trying to refactor the node support in various ways, the Value
> node is always annoying.
[…]
> 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.
This looks like a nice cleanup overall, independent of any future
refactoring.
> 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.
However, the patch adds:
> +typedef struct Null
> +{
> + NodeTag type;
> + char *val;
> +} Null;
which doesn't seem to be used anywhere. Is that a leftoverf from an
intermediate development stage?
- ilmari
Re: Remove Value node struct
On Wed, Aug 25, 2021 at 9:20 AM Peter Eisentraut wrote: > 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. +1. I noticed this years ago and never thought of doing anything about it. I'm glad you did think of it... -- Robert Haas EDB: http://www.enterprisedb.com
Remove Value node struct
newowner);
/* Generic cases */
diff --git a/src/backend/commands/comment.c b/src/backend/commands/comment.c
index 834f1a2a3f..d4943e374a 100644
--- a/src/backend/commands/comment.c
+++ b/src/backend/commands/comment.c
@@ -52,7 +52,7 @@ CommentObject(CommentStmt *stmt)
*/
if (stmt->objtype == OBJECT_DATABASE)
{
- char *database = strVal((Value *) stmt->object);
+ char *database = strVal(stmt->object);
if (!OidIsValid(get_database_oid(database, true)))
{
diff --git a/src/backend/commands/dropcmds.c b/src/backend/commands/dropcmds.c
index 97e5e9a765..4e545adf95 100644
--- a/src/backend/commands/dropcmds.c
+++ b/src/backend/commands/dropcmds.c
@@ -255,7 +255,7 @@ does_not_exist_skipping(ObjectType objtype, Node *object)
{
case OBJECT_ACCESS_METHOD:
msg = gettext_noop("access method \"%s\" does not
exist, skipping");
- name = strVal((Value *) object);
+ name = strVal(object);
break;
case OBJECT_TYPE:
case OBJECT_DOMAIN:
@@ -285,7 +285,7 @@ does_not_exist_skipping(ObjectType objtype, Node *object)
break;
case OBJECT_SCHEMA:
msg = gettext_noop("schema \"%s\" does not exist,
skipping");
- name = strVal((Value *) object);
+ name = strVal(object);
break;
case OBJECT_STATISTIC_EXT:
if (!schema_does_not_exist_skipping(castNode(List,
object), &msg, &name))
@@ -324,7 +324,7 @@ does_not_exist_skipping(ObjectType objtype, Node *object)
break;
case OBJECT_EXTENSION:
msg = gettext_noop("extension \"%s\" does not exist,
skipping");
- name = strVal((Value *) object);
+ name = strVal(object);
break;
case OBJECT_FUNCTION:
{
@@ -392,7 +392,7 @@ does_not_exist_skipping(ObjectType objtype, Node *object)
}
case OBJECT_LANGUAGE:
msg = gettext_noop("language \"%s\" does not exist,
skipping");
- name = strVal((Value *) object);
+ name = strVal(object);
break;
case OBJECT_CAST:
{
@@ -434,7 +434,7 @@ does_not_exist_skipping(ObjectType objtype, Node *object)
break;
case OBJECT_EVENT_TRIGGER:
msg = gettext_noop("event trigger \"%s\" does not
exist, skipping");
- name = strVal((Value *) object);
+ name = strVal(object);
break;
case OBJECT_RULE:
if (!owningrel_does_not_exist_skipping(castNode(List,
object), &msg, &name))
@@ -447,11 +447,11 @@ does_not_exist_skipping(ObjectType objtype, Node *object)
break;
case OBJECT_FDW:
msg = gettext_noop("foreign-data wrapper \"%s\" does
not exist, skipping");
- name = strVal((Value *) object);
+ name = strVal(object);
break;
case OBJECT_FOREIGN_SERVER:
msg = gettext_noop("server \"%s\" does not exist,
skipping");
- name = strVal((Value *) object);
+ name = strVal(object);
break;
case OBJECT_OPCLASS:
{
@@ -479,7 +479,7 @@ does_not_exist_skipping(ObjectType objtype, Node *object)
break;
case OBJECT_PUBLICATION:
msg = gettext_noop("publication \"%s\" does not exist,
skipping");
- name = strVal((Value *) object);
+ name = strVal(object);
break;
default:
elog(ERROR, "unrecognized object type: %d", (int)
objtype);
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index 4856f4b41d..bac4dd76ec 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -310,7 +310,7 @@ CreateStatistics(CreateStatsStmt *stmt)
build_mcv = false;
foreach(cell, stmt->stat_types)
{
- char *type = strVal((Value *) lfirst(cell));
+ char *type = strVal(lfirst(cell));
