automating RangeTblEntry node support
I have been looking into what it would take to get rid of the
custom_read_write and custom_query_jumble for the RangeTblEntry node
type. This is one of the larger and more complex exceptions left.
(Similar considerations would also apply to the Constraint node type.)
Allegedly, only certain fields of RangeTblEntry are valid based on
rtekind. But exactly which ones seems to be documented and handled
inconsistently. It seems that over time, new RTE kinds have "borrowed"
fields that notionally belong to other RTE kinds, which is technically
not a problem but creates a bit of a mess when trying to understand all
this.
I have some WIP patches to accompany this discussion.
Let's start with the jumble function. I suppose that this was just
carried over from the pg_stat_statements-specific code without any
detailed review. For example, the "inh" field is documented to be valid
in all RTEs, but it's only jumbled for RTE_RELATION. The "lateral"
field isn't looked at at all. I wouldn't be surprised if there are more
cases like this.
In the first attached patch, I remove _jumbleRangeTblEntry() and instead
add per-field query_jumble_ignore annotations to approximately match the
behavior of the previous custom code. The pg_stat_statements test suite
has some coverage of this. I get rid of switch on rtekind; this should
be technically correct, since we do the equal and copy functions like
this also. So for example the "inh" field is now considered in each
case. But I left "lateral" alone. I suspect several of these new
query_jumble_ignore should actually be dropped because the code was
wrong before.
In the second patch, I'm removing the switch on rtekind from
range_table_mutator_impl(). This should be fine because all the
subroutines can handle unset/NULL fields. And it removes one more place
that needs to track knowledge about which fields are valid when.
In the third patch, I'm removing the custom read/write functions for
RangeTblEntry. Those functions wanted to have a few fields at the front
to make the dump more legible; I'm doing that now by moving the fields
up in the actual struct.
Not done here, but something we should do is restructure the
documentation of RangeTblEntry itself. I'm still thinking about the
best way to structure this, but I'm thinking more like noting for each
field when it's used, instead by block like it is now, which makes it
awkward if a new RTE wants to borrow some fields.
Now one could probably rightfully complain that having all these unused
fields dumped would make the RangeTblEntry serialization bigger. I'm
not sure who big of a problem that actually is, considering how many
often-unset fields other node types have. But it deserves some
consideration. I think the best way to work around that would be to
have a mode that omits fields that have their default value (zero).
This would be more generally useful; for example Query also has a bunch
of fields that are not often set. I think this would be pretty easy to
implement, for example like
#define WRITE_INT_FIELD(fldname) \
if (full_mode || node->fldname) \
appendStringInfo(str, " :" CppAsString(fldname) " %d",
node->fldname)
There is also the discussion over at [0]/messages/by-id/CACxu=vL_SD=WJiFSJyyBuZAp_2v_XBqb1x9JBiqz52a_g9z3jA@mail.gmail.com about larger redesigns of the
node serialization format. I'm also interested in that, but here I'm
mainly trying to remove more special cases to make that kind of work
easier in the future.
Any thoughts about the direction?
[0]: /messages/by-id/CACxu=vL_SD=WJiFSJyyBuZAp_2v_XBqb1x9JBiqz52a_g9z3jA@mail.gmail.com
/messages/by-id/CACxu=vL_SD=WJiFSJyyBuZAp_2v_XBqb1x9JBiqz52a_g9z3jA@mail.gmail.com
Attachments:
v1-0001-Remove-custom-_jumbleRangeTblEntry.patchtext/plain; charset=UTF-8; name=v1-0001-Remove-custom-_jumbleRangeTblEntry.patchDownload
From ef08061a6d8f01e7db350b74eaa2d403df6079b9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 1 Dec 2023 11:45:50 +0100
Subject: [PATCH v1 1/3] Remove custom _jumbleRangeTblEntry()
---
src/backend/nodes/queryjumblefuncs.c | 48 ----------------------------
src/include/nodes/parsenodes.h | 42 ++++++++++++------------
2 files changed, 21 insertions(+), 69 deletions(-)
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index 281907a4d8..9094ea02d8 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -347,51 +347,3 @@ _jumbleA_Const(JumbleState *jstate, Node *node)
}
}
}
-
-static void
-_jumbleRangeTblEntry(JumbleState *jstate, Node *node)
-{
- RangeTblEntry *expr = (RangeTblEntry *) node;
-
- JUMBLE_FIELD(rtekind);
- switch (expr->rtekind)
- {
- case RTE_RELATION:
- JUMBLE_FIELD(relid);
- JUMBLE_NODE(tablesample);
- JUMBLE_FIELD(inh);
- break;
- case RTE_SUBQUERY:
- JUMBLE_NODE(subquery);
- break;
- case RTE_JOIN:
- JUMBLE_FIELD(jointype);
- break;
- case RTE_FUNCTION:
- JUMBLE_NODE(functions);
- break;
- case RTE_TABLEFUNC:
- JUMBLE_NODE(tablefunc);
- break;
- case RTE_VALUES:
- JUMBLE_NODE(values_lists);
- break;
- case RTE_CTE:
-
- /*
- * Depending on the CTE name here isn't ideal, but it's the only
- * info we have to identify the referenced WITH item.
- */
- JUMBLE_STRING(ctename);
- JUMBLE_FIELD(ctelevelsup);
- break;
- case RTE_NAMEDTUPLESTORE:
- JUMBLE_STRING(enrname);
- break;
- case RTE_RESULT:
- break;
- default:
- elog(ERROR, "unrecognized RTE kind: %d", (int) expr->rtekind);
- break;
- }
-}
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index e494309da8..a97f3cb16f 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1018,7 +1018,7 @@ typedef enum RTEKind
typedef struct RangeTblEntry
{
- pg_node_attr(custom_read_write, custom_query_jumble)
+ pg_node_attr(custom_read_write)
NodeTag type;
@@ -1062,16 +1062,16 @@ typedef struct RangeTblEntry
* tables to be invalidated if the underlying table is altered.
*/
Oid relid; /* OID of the relation */
- char relkind; /* relation kind (see pg_class.relkind) */
- int rellockmode; /* lock level that query requires on the rel */
+ char relkind pg_node_attr(query_jumble_ignore); /* relation kind (see pg_class.relkind) */
+ int rellockmode pg_node_attr(query_jumble_ignore); /* lock level that query requires on the rel */
struct TableSampleClause *tablesample; /* sampling info, or NULL */
- Index perminfoindex;
+ Index perminfoindex pg_node_attr(query_jumble_ignore);
/*
* Fields valid for a subquery RTE (else NULL):
*/
Query *subquery; /* the sub-query */
- bool security_barrier; /* is from security_barrier view? */
+ bool security_barrier pg_node_attr(query_jumble_ignore); /* is from security_barrier view? */
/*
* Fields valid for a join RTE (else NULL/zero):
@@ -1117,17 +1117,17 @@ typedef struct RangeTblEntry
* joinleftcols/joinrighttcols.
*/
JoinType jointype; /* type of join */
- int joinmergedcols; /* number of merged (JOIN USING) columns */
- List *joinaliasvars; /* list of alias-var expansions */
- List *joinleftcols; /* left-side input column numbers */
- List *joinrightcols; /* right-side input column numbers */
+ int joinmergedcols pg_node_attr(query_jumble_ignore); /* number of merged (JOIN USING) columns */
+ List *joinaliasvars pg_node_attr(query_jumble_ignore); /* list of alias-var expansions */
+ List *joinleftcols pg_node_attr(query_jumble_ignore); /* left-side input column numbers */
+ List *joinrightcols pg_node_attr(query_jumble_ignore); /* right-side input column numbers */
/*
* join_using_alias is an alias clause attached directly to JOIN/USING. It
* is different from the alias field (below) in that it does not hide the
* range variables of the tables being joined.
*/
- Alias *join_using_alias;
+ Alias *join_using_alias pg_node_attr(query_jumble_ignore);
/*
* Fields valid for a function RTE (else NIL/zero):
@@ -1138,7 +1138,7 @@ typedef struct RangeTblEntry
* expandRTE().
*/
List *functions; /* list of RangeTblFunction nodes */
- bool funcordinality; /* is this called WITH ORDINALITY? */
+ bool funcordinality pg_node_attr(query_jumble_ignore); /* is this called WITH ORDINALITY? */
/*
* Fields valid for a TableFunc RTE (else NULL):
@@ -1155,7 +1155,7 @@ typedef struct RangeTblEntry
*/
char *ctename; /* name of the WITH list item */
Index ctelevelsup; /* number of query levels up */
- bool self_reference; /* is this a recursive self-reference? */
+ bool self_reference pg_node_attr(query_jumble_ignore); /* is this a recursive self-reference? */
/*
* Fields valid for CTE, VALUES, ENR, and TableFunc RTEs (else NIL):
@@ -1175,25 +1175,25 @@ typedef struct RangeTblEntry
* all three lists (as well as an empty-string entry in eref). Testing
* for zero coltype is the standard way to detect a dropped column.
*/
- List *coltypes; /* OID list of column type OIDs */
- List *coltypmods; /* integer list of column typmods */
- List *colcollations; /* OID list of column collation OIDs */
+ List *coltypes pg_node_attr(query_jumble_ignore); /* OID list of column type OIDs */
+ List *coltypmods pg_node_attr(query_jumble_ignore); /* integer list of column typmods */
+ List *colcollations pg_node_attr(query_jumble_ignore); /* OID list of column collation OIDs */
/*
* Fields valid for ENR RTEs (else NULL/zero):
*/
char *enrname; /* name of ephemeral named relation */
- Cardinality enrtuples; /* estimated or actual from caller */
+ Cardinality enrtuples pg_node_attr(query_jumble_ignore); /* estimated or actual from caller */
/*
* Fields valid in all RTEs:
*/
- Alias *alias; /* user-written alias clause, if any */
- Alias *eref; /* expanded reference names */
- bool lateral; /* subquery, function, or values is LATERAL? */
+ Alias *alias pg_node_attr(query_jumble_ignore); /* user-written alias clause, if any */
+ Alias *eref pg_node_attr(query_jumble_ignore); /* expanded reference names */
+ bool lateral pg_node_attr(query_jumble_ignore); /* subquery, function, or values is LATERAL? */
bool inh; /* inheritance requested? */
- bool inFromCl; /* present in FROM clause? */
- List *securityQuals; /* security barrier quals to apply, if any */
+ bool inFromCl pg_node_attr(query_jumble_ignore); /* present in FROM clause? */
+ List *securityQuals pg_node_attr(query_jumble_ignore); /* security barrier quals to apply, if any */
} RangeTblEntry;
/*
--
2.43.0
v1-0002-Simplify-range_table_mutator_impl.patchtext/plain; charset=UTF-8; name=v1-0002-Simplify-range_table_mutator_impl.patchDownload
From 4a5efa4057ba8eddad3fe62b9a789bd3a5e03db8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 1 Dec 2023 11:54:26 +0100
Subject: [PATCH v1 2/3] Simplify range_table_mutator_impl()
---
src/backend/nodes/nodeFuncs.c | 61 +++++++++++++----------------------
1 file changed, 23 insertions(+), 38 deletions(-)
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index c03f4f23e2..ac195930af 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -3688,47 +3688,32 @@ range_table_mutator_impl(List *rtable,
RangeTblEntry *newrte;
FLATCOPY(newrte, rte, RangeTblEntry);
- switch (rte->rtekind)
+
+ MUTATE(newrte->tablesample, rte->tablesample, TableSampleClause *);
+
+ if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
+ MUTATE(newrte->subquery, rte->subquery, Query *);
+ else
{
- case RTE_RELATION:
- MUTATE(newrte->tablesample, rte->tablesample,
- TableSampleClause *);
- /* we don't bother to copy eref, aliases, etc; OK? */
- break;
- case RTE_SUBQUERY:
- if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
- MUTATE(newrte->subquery, rte->subquery, Query *);
- else
- {
- /* else, copy RT subqueries as-is */
- newrte->subquery = copyObject(rte->subquery);
- }
- break;
- case RTE_JOIN:
- if (!(flags & QTW_IGNORE_JOINALIASES))
- MUTATE(newrte->joinaliasvars, rte->joinaliasvars, List *);
- else
- {
- /* else, copy join aliases as-is */
- newrte->joinaliasvars = copyObject(rte->joinaliasvars);
- }
- break;
- case RTE_FUNCTION:
- MUTATE(newrte->functions, rte->functions, List *);
- break;
- case RTE_TABLEFUNC:
- MUTATE(newrte->tablefunc, rte->tablefunc, TableFunc *);
- break;
- case RTE_VALUES:
- MUTATE(newrte->values_lists, rte->values_lists, List *);
- break;
- case RTE_CTE:
- case RTE_NAMEDTUPLESTORE:
- case RTE_RESULT:
- /* nothing to do */
- break;
+ /* else, copy RT subqueries as-is */
+ newrte->subquery = copyObject(rte->subquery);
+ }
+
+ if (!(flags & QTW_IGNORE_JOINALIASES))
+ MUTATE(newrte->joinaliasvars, rte->joinaliasvars, List *);
+ else
+ {
+ /* else, copy join aliases as-is */
+ newrte->joinaliasvars = copyObject(rte->joinaliasvars);
}
+
+ MUTATE(newrte->functions, rte->functions, List *);
+ MUTATE(newrte->tablefunc, rte->tablefunc, TableFunc *);
+ MUTATE(newrte->values_lists, rte->values_lists, List *);
MUTATE(newrte->securityQuals, rte->securityQuals, List *);
+
+ /* we don't bother to copy eref, aliases, etc; OK? */
+
newrt = lappend(newrt, newrte);
}
return newrt;
--
2.43.0
v1-0003-Remove-custom-RangeTblEntry-read-write.patchtext/plain; charset=UTF-8; name=v1-0003-Remove-custom-RangeTblEntry-read-write.patchDownload
From 0ecfd88afc56d564b1c4c60492c80ca716694ec6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 1 Dec 2023 12:12:31 +0100
Subject: [PATCH v1 3/3] Remove custom RangeTblEntry read/write
---
src/backend/nodes/outfuncs.c | 80 -----------------------------
src/backend/nodes/readfuncs.c | 92 ----------------------------------
src/include/nodes/parsenodes.h | 12 +++--
3 files changed, 8 insertions(+), 176 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index e66a99247e..ded3c59b76 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -489,86 +489,6 @@ _outExtensibleNode(StringInfo str, const ExtensibleNode *node)
methods->nodeOut(str, node);
}
-static void
-_outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
-{
- WRITE_NODE_TYPE("RANGETBLENTRY");
-
- /* put alias + eref first to make dump more legible */
- WRITE_NODE_FIELD(alias);
- WRITE_NODE_FIELD(eref);
- WRITE_ENUM_FIELD(rtekind, RTEKind);
-
- switch (node->rtekind)
- {
- case RTE_RELATION:
- WRITE_OID_FIELD(relid);
- WRITE_CHAR_FIELD(relkind);
- WRITE_INT_FIELD(rellockmode);
- WRITE_NODE_FIELD(tablesample);
- WRITE_UINT_FIELD(perminfoindex);
- break;
- case RTE_SUBQUERY:
- WRITE_NODE_FIELD(subquery);
- WRITE_BOOL_FIELD(security_barrier);
- /* we re-use these RELATION fields, too: */
- WRITE_OID_FIELD(relid);
- WRITE_CHAR_FIELD(relkind);
- WRITE_INT_FIELD(rellockmode);
- WRITE_UINT_FIELD(perminfoindex);
- break;
- case RTE_JOIN:
- WRITE_ENUM_FIELD(jointype, JoinType);
- WRITE_INT_FIELD(joinmergedcols);
- WRITE_NODE_FIELD(joinaliasvars);
- WRITE_NODE_FIELD(joinleftcols);
- WRITE_NODE_FIELD(joinrightcols);
- WRITE_NODE_FIELD(join_using_alias);
- break;
- case RTE_FUNCTION:
- WRITE_NODE_FIELD(functions);
- WRITE_BOOL_FIELD(funcordinality);
- break;
- case RTE_TABLEFUNC:
- WRITE_NODE_FIELD(tablefunc);
- break;
- case RTE_VALUES:
- WRITE_NODE_FIELD(values_lists);
- WRITE_NODE_FIELD(coltypes);
- WRITE_NODE_FIELD(coltypmods);
- WRITE_NODE_FIELD(colcollations);
- break;
- case RTE_CTE:
- WRITE_STRING_FIELD(ctename);
- WRITE_UINT_FIELD(ctelevelsup);
- WRITE_BOOL_FIELD(self_reference);
- WRITE_NODE_FIELD(coltypes);
- WRITE_NODE_FIELD(coltypmods);
- WRITE_NODE_FIELD(colcollations);
- break;
- case RTE_NAMEDTUPLESTORE:
- WRITE_STRING_FIELD(enrname);
- WRITE_FLOAT_FIELD(enrtuples);
- WRITE_NODE_FIELD(coltypes);
- WRITE_NODE_FIELD(coltypmods);
- WRITE_NODE_FIELD(colcollations);
- /* we re-use these RELATION fields, too: */
- WRITE_OID_FIELD(relid);
- break;
- case RTE_RESULT:
- /* no extra fields */
- break;
- default:
- elog(ERROR, "unrecognized RTE kind: %d", (int) node->rtekind);
- break;
- }
-
- WRITE_BOOL_FIELD(lateral);
- WRITE_BOOL_FIELD(inh);
- WRITE_BOOL_FIELD(inFromCl);
- WRITE_NODE_FIELD(securityQuals);
-}
-
static void
_outA_Expr(StringInfo str, const A_Expr *node)
{
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index cc2021c1f7..0c64e06c38 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -486,98 +486,6 @@ _readConstraint(void)
READ_DONE();
}
-static RangeTblEntry *
-_readRangeTblEntry(void)
-{
- READ_LOCALS(RangeTblEntry);
-
- /* put alias + eref first to make dump more legible */
- READ_NODE_FIELD(alias);
- READ_NODE_FIELD(eref);
- READ_ENUM_FIELD(rtekind, RTEKind);
-
- switch (local_node->rtekind)
- {
- case RTE_RELATION:
- READ_OID_FIELD(relid);
- READ_CHAR_FIELD(relkind);
- READ_INT_FIELD(rellockmode);
- READ_NODE_FIELD(tablesample);
- READ_UINT_FIELD(perminfoindex);
- break;
- case RTE_SUBQUERY:
- READ_NODE_FIELD(subquery);
- READ_BOOL_FIELD(security_barrier);
- /* we re-use these RELATION fields, too: */
- READ_OID_FIELD(relid);
- READ_CHAR_FIELD(relkind);
- READ_INT_FIELD(rellockmode);
- READ_UINT_FIELD(perminfoindex);
- break;
- case RTE_JOIN:
- READ_ENUM_FIELD(jointype, JoinType);
- READ_INT_FIELD(joinmergedcols);
- READ_NODE_FIELD(joinaliasvars);
- READ_NODE_FIELD(joinleftcols);
- READ_NODE_FIELD(joinrightcols);
- READ_NODE_FIELD(join_using_alias);
- break;
- case RTE_FUNCTION:
- READ_NODE_FIELD(functions);
- READ_BOOL_FIELD(funcordinality);
- break;
- case RTE_TABLEFUNC:
- READ_NODE_FIELD(tablefunc);
- /* The RTE must have a copy of the column type info, if any */
- if (local_node->tablefunc)
- {
- TableFunc *tf = local_node->tablefunc;
-
- local_node->coltypes = tf->coltypes;
- local_node->coltypmods = tf->coltypmods;
- local_node->colcollations = tf->colcollations;
- }
- break;
- case RTE_VALUES:
- READ_NODE_FIELD(values_lists);
- READ_NODE_FIELD(coltypes);
- READ_NODE_FIELD(coltypmods);
- READ_NODE_FIELD(colcollations);
- break;
- case RTE_CTE:
- READ_STRING_FIELD(ctename);
- READ_UINT_FIELD(ctelevelsup);
- READ_BOOL_FIELD(self_reference);
- READ_NODE_FIELD(coltypes);
- READ_NODE_FIELD(coltypmods);
- READ_NODE_FIELD(colcollations);
- break;
- case RTE_NAMEDTUPLESTORE:
- READ_STRING_FIELD(enrname);
- READ_FLOAT_FIELD(enrtuples);
- READ_NODE_FIELD(coltypes);
- READ_NODE_FIELD(coltypmods);
- READ_NODE_FIELD(colcollations);
- /* we re-use these RELATION fields, too: */
- READ_OID_FIELD(relid);
- break;
- case RTE_RESULT:
- /* no extra fields */
- break;
- default:
- elog(ERROR, "unrecognized RTE kind: %d",
- (int) local_node->rtekind);
- break;
- }
-
- READ_BOOL_FIELD(lateral);
- READ_BOOL_FIELD(inh);
- READ_BOOL_FIELD(inFromCl);
- READ_NODE_FIELD(securityQuals);
-
- READ_DONE();
-}
-
static A_Expr *
_readA_Expr(void)
{
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index a97f3cb16f..d3b1af6531 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1018,8 +1018,6 @@ typedef enum RTEKind
typedef struct RangeTblEntry
{
- pg_node_attr(custom_read_write)
-
NodeTag type;
RTEKind rtekind; /* see above */
@@ -1030,6 +1028,14 @@ typedef struct RangeTblEntry
* code that is being actively worked on. FIXME someday.
*/
+ /*
+ * Fields valid in all RTEs:
+ *
+ * put alias + eref first to make dump more legible
+ */
+ Alias *alias pg_node_attr(query_jumble_ignore); /* user-written alias clause, if any */
+ Alias *eref pg_node_attr(query_jumble_ignore); /* expanded reference names */
+
/*
* Fields valid for a plain relation RTE (else zero):
*
@@ -1188,8 +1194,6 @@ typedef struct RangeTblEntry
/*
* Fields valid in all RTEs:
*/
- Alias *alias pg_node_attr(query_jumble_ignore); /* user-written alias clause, if any */
- Alias *eref pg_node_attr(query_jumble_ignore); /* expanded reference names */
bool lateral pg_node_attr(query_jumble_ignore); /* subquery, function, or values is LATERAL? */
bool inh; /* inheritance requested? */
bool inFromCl pg_node_attr(query_jumble_ignore); /* present in FROM clause? */
--
2.43.0
On Wed, 6 Dec 2023 at 21:02, Peter Eisentraut <peter@eisentraut.org> wrote:
I have been looking into what it would take to get rid of the
custom_read_write and custom_query_jumble for the RangeTblEntry node
type. This is one of the larger and more complex exceptions left.
[...]
Now one could probably rightfully complain that having all these unused
fields dumped would make the RangeTblEntry serialization bigger. I'm
not sure who big of a problem that actually is, considering how many
often-unset fields other node types have. But it deserves some
consideration. I think the best way to work around that would be to
have a mode that omits fields that have their default value (zero).
This would be more generally useful; for example Query also has a bunch
of fields that are not often set. I think this would be pretty easy to
implement, for example like
Actually, I've worked on this last weekend, and got some good results.
It did need some fine-tuning and field annotations, but got raw
nodeToString sizes down 50%+ for the pg_rewrite table's ev_action
column, and compressed-with-pglz size of pg_rewrite total down 30%+.
#define WRITE_INT_FIELD(fldname) \
if (full_mode || node->fldname) \
appendStringInfo(str, " :" CppAsString(fldname) " %d",
node->fldname)There is also the discussion over at [0] about larger redesigns of the
node serialization format. I'm also interested in that, but here I'm
mainly trying to remove more special cases to make that kind of work
easier in the future.Any thoughts about the direction?
I've created a new thread [0] with my patch. It actually didn't need
_that_ many manual changes - most of it was just updating the
gen_node_support.pl code generation, and making the macros do a good
job.
In general I'm all for reducing special cases, so +1 on the idea. I'll
have to check the specifics of the patches at a later point in time.
Kind regards,
Matthias van de Meent
Neon (https://neon.tech)
On 06.12.23 21:02, Peter Eisentraut wrote:
I have been looking into what it would take to get rid of the
custom_read_write and custom_query_jumble for the RangeTblEntry node
type. This is one of the larger and more complex exceptions left.(Similar considerations would also apply to the Constraint node type.)
In this updated patch set, I have also added the treatment of the
Constraint type. (I also noted that the manual read/write functions for
the Constraint type are out-of-sync again, so simplifying this would be
really helpful.) I have also added commit messages to each patch.
The way I have re-ordered the patch series now, I think patches 0001
through 0003 are candidates for inclusion after review, patch 0004 still
needs a bit more analysis and testing, as described therein.
Attachments:
v2-0001-Remove-custom-Constraint-node-read-write-implemen.patchtext/plain; charset=UTF-8; name=v2-0001-Remove-custom-Constraint-node-read-write-implemen.patchDownload
From f0e896a201367a639be9d08d070066867c46d7cf Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Jan 2024 11:23:39 +0100
Subject: [PATCH v2 1/4] Remove custom Constraint node read/write
implementations
This is part of an effort to reduce the number of special cases in the
automatically generated node support functions.
Allegedly, only certain fields of the Constraint node are valid based
on contype. But this has historically not been kept up to date in the
read/write functions. The Constraint node is only used for debugging
DDL statements, so there are no strong requirements for its output,
and there is no enforcement for its correctness. (There was no read
support before a6bc3301925.) Commits e7a552f303c and abf46ad9c7b are
examples of where omissions were fixed. And the current output is
again incorrect, because the recently added "inhcount" field is not
included.
This patch just removes the custom read/write implementations for the
Constraint node type. Now we just output all the fields, which is a
bit more than before, but at least we don't have to maintain these
functions anymore. Also, we lose the string representation of the
contype field, but for this marginal use case that seems tolerable.
This patch also changes the documentation of the Constraint struct to
put less emphasis on grouping fields by constraint type but rather
document for each field how it's used.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
---
src/backend/nodes/outfuncs.c | 126 -----------------------------
src/backend/nodes/readfuncs.c | 143 ---------------------------------
src/include/nodes/parsenodes.h | 38 +++------
3 files changed, 13 insertions(+), 294 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 296ba845187..dee2b9e87b2 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -699,132 +699,6 @@ _outA_Const(StringInfo str, const A_Const *node)
WRITE_LOCATION_FIELD(location);
}
-static void
-_outConstraint(StringInfo str, const Constraint *node)
-{
- WRITE_NODE_TYPE("CONSTRAINT");
-
- WRITE_STRING_FIELD(conname);
- WRITE_BOOL_FIELD(deferrable);
- WRITE_BOOL_FIELD(initdeferred);
- WRITE_LOCATION_FIELD(location);
-
- appendStringInfoString(str, " :contype ");
- switch (node->contype)
- {
- case CONSTR_NULL:
- appendStringInfoString(str, "NULL");
- break;
-
- case CONSTR_NOTNULL:
- appendStringInfoString(str, "NOT_NULL");
- WRITE_NODE_FIELD(keys);
- WRITE_INT_FIELD(inhcount);
- WRITE_BOOL_FIELD(is_no_inherit);
- WRITE_BOOL_FIELD(skip_validation);
- WRITE_BOOL_FIELD(initially_valid);
- break;
-
- case CONSTR_DEFAULT:
- appendStringInfoString(str, "DEFAULT");
- WRITE_NODE_FIELD(raw_expr);
- WRITE_STRING_FIELD(cooked_expr);
- break;
-
- case CONSTR_IDENTITY:
- appendStringInfoString(str, "IDENTITY");
- WRITE_NODE_FIELD(options);
- WRITE_CHAR_FIELD(generated_when);
- break;
-
- case CONSTR_GENERATED:
- appendStringInfoString(str, "GENERATED");
- WRITE_NODE_FIELD(raw_expr);
- WRITE_STRING_FIELD(cooked_expr);
- WRITE_CHAR_FIELD(generated_when);
- break;
-
- case CONSTR_CHECK:
- appendStringInfoString(str, "CHECK");
- WRITE_BOOL_FIELD(is_no_inherit);
- WRITE_NODE_FIELD(raw_expr);
- WRITE_STRING_FIELD(cooked_expr);
- WRITE_BOOL_FIELD(skip_validation);
- WRITE_BOOL_FIELD(initially_valid);
- break;
-
- case CONSTR_PRIMARY:
- appendStringInfoString(str, "PRIMARY_KEY");
- WRITE_NODE_FIELD(keys);
- WRITE_NODE_FIELD(including);
- WRITE_NODE_FIELD(options);
- WRITE_STRING_FIELD(indexname);
- WRITE_STRING_FIELD(indexspace);
- WRITE_BOOL_FIELD(reset_default_tblspc);
- /* access_method and where_clause not currently used */
- break;
-
- case CONSTR_UNIQUE:
- appendStringInfoString(str, "UNIQUE");
- WRITE_BOOL_FIELD(nulls_not_distinct);
- WRITE_NODE_FIELD(keys);
- WRITE_NODE_FIELD(including);
- WRITE_NODE_FIELD(options);
- WRITE_STRING_FIELD(indexname);
- WRITE_STRING_FIELD(indexspace);
- WRITE_BOOL_FIELD(reset_default_tblspc);
- /* access_method and where_clause not currently used */
- break;
-
- case CONSTR_EXCLUSION:
- appendStringInfoString(str, "EXCLUSION");
- WRITE_NODE_FIELD(exclusions);
- WRITE_NODE_FIELD(including);
- WRITE_NODE_FIELD(options);
- WRITE_STRING_FIELD(indexname);
- WRITE_STRING_FIELD(indexspace);
- WRITE_BOOL_FIELD(reset_default_tblspc);
- WRITE_STRING_FIELD(access_method);
- WRITE_NODE_FIELD(where_clause);
- break;
-
- case CONSTR_FOREIGN:
- appendStringInfoString(str, "FOREIGN_KEY");
- WRITE_NODE_FIELD(pktable);
- WRITE_NODE_FIELD(fk_attrs);
- WRITE_NODE_FIELD(pk_attrs);
- WRITE_CHAR_FIELD(fk_matchtype);
- WRITE_CHAR_FIELD(fk_upd_action);
- WRITE_CHAR_FIELD(fk_del_action);
- WRITE_NODE_FIELD(fk_del_set_cols);
- WRITE_NODE_FIELD(old_conpfeqop);
- WRITE_OID_FIELD(old_pktable_oid);
- WRITE_BOOL_FIELD(skip_validation);
- WRITE_BOOL_FIELD(initially_valid);
- break;
-
- case CONSTR_ATTR_DEFERRABLE:
- appendStringInfoString(str, "ATTR_DEFERRABLE");
- break;
-
- case CONSTR_ATTR_NOT_DEFERRABLE:
- appendStringInfoString(str, "ATTR_NOT_DEFERRABLE");
- break;
-
- case CONSTR_ATTR_DEFERRED:
- appendStringInfoString(str, "ATTR_DEFERRED");
- break;
-
- case CONSTR_ATTR_IMMEDIATE:
- appendStringInfoString(str, "ATTR_IMMEDIATE");
- break;
-
- default:
- elog(ERROR, "unrecognized ConstrType: %d", (int) node->contype);
- break;
- }
-}
-
/*
* outNode -
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 1624b345812..b1e2f2b440a 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -343,149 +343,6 @@ _readA_Const(void)
READ_DONE();
}
-/*
- * _readConstraint
- */
-static Constraint *
-_readConstraint(void)
-{
- READ_LOCALS(Constraint);
-
- READ_STRING_FIELD(conname);
- READ_BOOL_FIELD(deferrable);
- READ_BOOL_FIELD(initdeferred);
- READ_LOCATION_FIELD(location);
-
- token = pg_strtok(&length); /* skip :contype */
- token = pg_strtok(&length); /* get field value */
- if (length == 4 && strncmp(token, "NULL", 4) == 0)
- local_node->contype = CONSTR_NULL;
- else if (length == 8 && strncmp(token, "NOT_NULL", 8) == 0)
- local_node->contype = CONSTR_NOTNULL;
- else if (length == 7 && strncmp(token, "DEFAULT", 7) == 0)
- local_node->contype = CONSTR_DEFAULT;
- else if (length == 8 && strncmp(token, "IDENTITY", 8) == 0)
- local_node->contype = CONSTR_IDENTITY;
- else if (length == 9 && strncmp(token, "GENERATED", 9) == 0)
- local_node->contype = CONSTR_GENERATED;
- else if (length == 5 && strncmp(token, "CHECK", 5) == 0)
- local_node->contype = CONSTR_CHECK;
- else if (length == 11 && strncmp(token, "PRIMARY_KEY", 11) == 0)
- local_node->contype = CONSTR_PRIMARY;
- else if (length == 6 && strncmp(token, "UNIQUE", 6) == 0)
- local_node->contype = CONSTR_UNIQUE;
- else if (length == 9 && strncmp(token, "EXCLUSION", 9) == 0)
- local_node->contype = CONSTR_EXCLUSION;
- else if (length == 11 && strncmp(token, "FOREIGN_KEY", 11) == 0)
- local_node->contype = CONSTR_FOREIGN;
- else if (length == 15 && strncmp(token, "ATTR_DEFERRABLE", 15) == 0)
- local_node->contype = CONSTR_ATTR_DEFERRABLE;
- else if (length == 19 && strncmp(token, "ATTR_NOT_DEFERRABLE", 19) == 0)
- local_node->contype = CONSTR_ATTR_NOT_DEFERRABLE;
- else if (length == 13 && strncmp(token, "ATTR_DEFERRED", 13) == 0)
- local_node->contype = CONSTR_ATTR_DEFERRED;
- else if (length == 14 && strncmp(token, "ATTR_IMMEDIATE", 14) == 0)
- local_node->contype = CONSTR_ATTR_IMMEDIATE;
-
- switch (local_node->contype)
- {
- case CONSTR_NULL:
- /* no extra fields */
- break;
-
- case CONSTR_NOTNULL:
- READ_NODE_FIELD(keys);
- READ_INT_FIELD(inhcount);
- READ_BOOL_FIELD(is_no_inherit);
- READ_BOOL_FIELD(skip_validation);
- READ_BOOL_FIELD(initially_valid);
- break;
-
- case CONSTR_DEFAULT:
- READ_NODE_FIELD(raw_expr);
- READ_STRING_FIELD(cooked_expr);
- break;
-
- case CONSTR_IDENTITY:
- READ_NODE_FIELD(options);
- READ_CHAR_FIELD(generated_when);
- break;
-
- case CONSTR_GENERATED:
- READ_NODE_FIELD(raw_expr);
- READ_STRING_FIELD(cooked_expr);
- READ_CHAR_FIELD(generated_when);
- break;
-
- case CONSTR_CHECK:
- READ_BOOL_FIELD(is_no_inherit);
- READ_NODE_FIELD(raw_expr);
- READ_STRING_FIELD(cooked_expr);
- READ_BOOL_FIELD(skip_validation);
- READ_BOOL_FIELD(initially_valid);
- break;
-
- case CONSTR_PRIMARY:
- READ_NODE_FIELD(keys);
- READ_NODE_FIELD(including);
- READ_NODE_FIELD(options);
- READ_STRING_FIELD(indexname);
- READ_STRING_FIELD(indexspace);
- READ_BOOL_FIELD(reset_default_tblspc);
- /* access_method and where_clause not currently used */
- break;
-
- case CONSTR_UNIQUE:
- READ_BOOL_FIELD(nulls_not_distinct);
- READ_NODE_FIELD(keys);
- READ_NODE_FIELD(including);
- READ_NODE_FIELD(options);
- READ_STRING_FIELD(indexname);
- READ_STRING_FIELD(indexspace);
- READ_BOOL_FIELD(reset_default_tblspc);
- /* access_method and where_clause not currently used */
- break;
-
- case CONSTR_EXCLUSION:
- READ_NODE_FIELD(exclusions);
- READ_NODE_FIELD(including);
- READ_NODE_FIELD(options);
- READ_STRING_FIELD(indexname);
- READ_STRING_FIELD(indexspace);
- READ_BOOL_FIELD(reset_default_tblspc);
- READ_STRING_FIELD(access_method);
- READ_NODE_FIELD(where_clause);
- break;
-
- case CONSTR_FOREIGN:
- READ_NODE_FIELD(pktable);
- READ_NODE_FIELD(fk_attrs);
- READ_NODE_FIELD(pk_attrs);
- READ_CHAR_FIELD(fk_matchtype);
- READ_CHAR_FIELD(fk_upd_action);
- READ_CHAR_FIELD(fk_del_action);
- READ_NODE_FIELD(fk_del_set_cols);
- READ_NODE_FIELD(old_conpfeqop);
- READ_OID_FIELD(old_pktable_oid);
- READ_BOOL_FIELD(skip_validation);
- READ_BOOL_FIELD(initially_valid);
- break;
-
- case CONSTR_ATTR_DEFERRABLE:
- case CONSTR_ATTR_NOT_DEFERRABLE:
- case CONSTR_ATTR_DEFERRED:
- case CONSTR_ATTR_IMMEDIATE:
- /* no extra fields */
- break;
-
- default:
- elog(ERROR, "unrecognized ConstrType: %d", (int) local_node->contype);
- break;
- }
-
- READ_DONE();
-}
-
static RangeTblEntry *
_readRangeTblEntry(void)
{
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index b3181f34aee..648b6197502 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2566,43 +2566,33 @@ typedef enum ConstrType /* types of constraints */
typedef struct Constraint
{
- pg_node_attr(custom_read_write)
-
NodeTag type;
ConstrType contype; /* see above */
-
- /* Fields used for most/all constraint types: */
char *conname; /* Constraint name, or NULL if unnamed */
bool deferrable; /* DEFERRABLE? */
bool initdeferred; /* INITIALLY DEFERRED? */
- int location; /* token location, or -1 if unknown */
-
- /* Fields used for constraints with expressions (CHECK and DEFAULT): */
+ bool skip_validation; /* skip validation of existing rows? */
+ bool initially_valid; /* mark the new constraint as valid? */
bool is_no_inherit; /* is constraint non-inheritable? */
- Node *raw_expr; /* expr, as untransformed parse tree */
- char *cooked_expr; /* expr, as nodeToString representation */
+ Node *raw_expr; /* CHECK or DEFAULT expression, as
+ * untransformed parse tree */
+ char *cooked_expr; /* CHECK or DEFAULT expression, as
+ * nodeToString representation */
char generated_when; /* ALWAYS or BY DEFAULT */
-
- /* Fields used for "raw" NOT NULL constraints: */
- int inhcount; /* initial inheritance count to apply */
-
- /* Fields used for unique constraints (UNIQUE and PRIMARY KEY): */
+ int inhcount; /* initial inheritance count to apply, for
+ * "raw" NOT NULL constraints */
bool nulls_not_distinct; /* null treatment for UNIQUE constraints */
List *keys; /* String nodes naming referenced key
- * column(s); also used for NOT NULL */
+ * column(s); for UNIQUE/PK/NOT NULL */
List *including; /* String nodes naming referenced nonkey
- * column(s) */
-
- /* Fields used for EXCLUSION constraints: */
- List *exclusions; /* list of (IndexElem, operator name) pairs */
-
- /* Fields used for index constraints (UNIQUE, PRIMARY KEY, EXCLUSION): */
+ * column(s); for UNIQUE/PK */
+ List *exclusions; /* list of (IndexElem, operator name) pairs;
+ * for exclusion constraints */
List *options; /* options from WITH clause */
char *indexname; /* existing index to use; otherwise NULL */
char *indexspace; /* index tablespace; NULL for default */
bool reset_default_tblspc; /* reset default_tablespace prior to
* creating the index */
- /* These could be, but currently are not, used for UNIQUE/PKEY: */
char *access_method; /* index access method; NULL for default */
Node *where_clause; /* partial index predicate */
@@ -2618,9 +2608,7 @@ typedef struct Constraint
Oid old_pktable_oid; /* pg_constraint.confrelid of my former
* self */
- /* Fields used for constraints that allow a NOT VALID specification */
- bool skip_validation; /* skip validation of existing rows? */
- bool initially_valid; /* mark the new constraint as valid? */
+ int location; /* token location, or -1 if unknown */
} Constraint;
/* ----------------------
base-commit: 31acee4b66f9f88ad5c19c1276252688bdaa95c9
--
2.43.0
v2-0002-Remove-custom-_jumbleRangeTblEntry.patchtext/plain; charset=UTF-8; name=v2-0002-Remove-custom-_jumbleRangeTblEntry.patchDownload
From 191aae34189594314042299f50309a1de3574be9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Jan 2024 11:23:39 +0100
Subject: [PATCH v2 2/4] Remove custom _jumbleRangeTblEntry()
This is part of an effort to reduce the number of special cases in the
automatically generated node support functions.
RangeTblEntry has a custom jumble function. This was probably just
carried over from the pg_stat_statements-specific code without any
detailed review. For example, the "inh" field is documented to be
valid in all RTEs, but it's only jumbled for RTE_RELATION. The
"lateral" field isn't looked at at all. I wouldn't be surprised if
there are more cases like this.
This patch removes _jumbleRangeTblEntry() and instead adds per-field
query_jumble_ignore annotations to approximately match the behavior of
the previous custom code. The pg_stat_statements test suite has some
coverage of this. It gets rid of switch on rtekind; this should be
technically correct, since we do the equal and copy functions like
this also. So for example the "inh" field is now considered in each
case. But it leaves "lateral" alone. Probably, several of these new
query_jumble_ignore should actually be dropped because the code was
wrong before.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
---
src/backend/nodes/queryjumblefuncs.c | 48 ----------------------------
src/include/nodes/parsenodes.h | 42 ++++++++++++------------
2 files changed, 21 insertions(+), 69 deletions(-)
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index e489bfceb56..7accd7b6242 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -347,51 +347,3 @@ _jumbleA_Const(JumbleState *jstate, Node *node)
}
}
}
-
-static void
-_jumbleRangeTblEntry(JumbleState *jstate, Node *node)
-{
- RangeTblEntry *expr = (RangeTblEntry *) node;
-
- JUMBLE_FIELD(rtekind);
- switch (expr->rtekind)
- {
- case RTE_RELATION:
- JUMBLE_FIELD(relid);
- JUMBLE_NODE(tablesample);
- JUMBLE_FIELD(inh);
- break;
- case RTE_SUBQUERY:
- JUMBLE_NODE(subquery);
- break;
- case RTE_JOIN:
- JUMBLE_FIELD(jointype);
- break;
- case RTE_FUNCTION:
- JUMBLE_NODE(functions);
- break;
- case RTE_TABLEFUNC:
- JUMBLE_NODE(tablefunc);
- break;
- case RTE_VALUES:
- JUMBLE_NODE(values_lists);
- break;
- case RTE_CTE:
-
- /*
- * Depending on the CTE name here isn't ideal, but it's the only
- * info we have to identify the referenced WITH item.
- */
- JUMBLE_STRING(ctename);
- JUMBLE_FIELD(ctelevelsup);
- break;
- case RTE_NAMEDTUPLESTORE:
- JUMBLE_STRING(enrname);
- break;
- case RTE_RESULT:
- break;
- default:
- elog(ERROR, "unrecognized RTE kind: %d", (int) expr->rtekind);
- break;
- }
-}
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 648b6197502..d377f16a72b 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1018,7 +1018,7 @@ typedef enum RTEKind
typedef struct RangeTblEntry
{
- pg_node_attr(custom_read_write, custom_query_jumble)
+ pg_node_attr(custom_read_write)
NodeTag type;
@@ -1062,16 +1062,16 @@ typedef struct RangeTblEntry
* tables to be invalidated if the underlying table is altered.
*/
Oid relid; /* OID of the relation */
- char relkind; /* relation kind (see pg_class.relkind) */
- int rellockmode; /* lock level that query requires on the rel */
+ char relkind pg_node_attr(query_jumble_ignore); /* relation kind (see pg_class.relkind) */
+ int rellockmode pg_node_attr(query_jumble_ignore); /* lock level that query requires on the rel */
struct TableSampleClause *tablesample; /* sampling info, or NULL */
- Index perminfoindex;
+ Index perminfoindex pg_node_attr(query_jumble_ignore);
/*
* Fields valid for a subquery RTE (else NULL):
*/
Query *subquery; /* the sub-query */
- bool security_barrier; /* is from security_barrier view? */
+ bool security_barrier pg_node_attr(query_jumble_ignore); /* is from security_barrier view? */
/*
* Fields valid for a join RTE (else NULL/zero):
@@ -1117,17 +1117,17 @@ typedef struct RangeTblEntry
* joinleftcols/joinrighttcols.
*/
JoinType jointype; /* type of join */
- int joinmergedcols; /* number of merged (JOIN USING) columns */
- List *joinaliasvars; /* list of alias-var expansions */
- List *joinleftcols; /* left-side input column numbers */
- List *joinrightcols; /* right-side input column numbers */
+ int joinmergedcols pg_node_attr(query_jumble_ignore); /* number of merged (JOIN USING) columns */
+ List *joinaliasvars pg_node_attr(query_jumble_ignore); /* list of alias-var expansions */
+ List *joinleftcols pg_node_attr(query_jumble_ignore); /* left-side input column numbers */
+ List *joinrightcols pg_node_attr(query_jumble_ignore); /* right-side input column numbers */
/*
* join_using_alias is an alias clause attached directly to JOIN/USING. It
* is different from the alias field (below) in that it does not hide the
* range variables of the tables being joined.
*/
- Alias *join_using_alias;
+ Alias *join_using_alias pg_node_attr(query_jumble_ignore);
/*
* Fields valid for a function RTE (else NIL/zero):
@@ -1138,7 +1138,7 @@ typedef struct RangeTblEntry
* expandRTE().
*/
List *functions; /* list of RangeTblFunction nodes */
- bool funcordinality; /* is this called WITH ORDINALITY? */
+ bool funcordinality pg_node_attr(query_jumble_ignore); /* is this called WITH ORDINALITY? */
/*
* Fields valid for a TableFunc RTE (else NULL):
@@ -1155,7 +1155,7 @@ typedef struct RangeTblEntry
*/
char *ctename; /* name of the WITH list item */
Index ctelevelsup; /* number of query levels up */
- bool self_reference; /* is this a recursive self-reference? */
+ bool self_reference pg_node_attr(query_jumble_ignore); /* is this a recursive self-reference? */
/*
* Fields valid for CTE, VALUES, ENR, and TableFunc RTEs (else NIL):
@@ -1175,25 +1175,25 @@ typedef struct RangeTblEntry
* all three lists (as well as an empty-string entry in eref). Testing
* for zero coltype is the standard way to detect a dropped column.
*/
- List *coltypes; /* OID list of column type OIDs */
- List *coltypmods; /* integer list of column typmods */
- List *colcollations; /* OID list of column collation OIDs */
+ List *coltypes pg_node_attr(query_jumble_ignore); /* OID list of column type OIDs */
+ List *coltypmods pg_node_attr(query_jumble_ignore); /* integer list of column typmods */
+ List *colcollations pg_node_attr(query_jumble_ignore); /* OID list of column collation OIDs */
/*
* Fields valid for ENR RTEs (else NULL/zero):
*/
char *enrname; /* name of ephemeral named relation */
- Cardinality enrtuples; /* estimated or actual from caller */
+ Cardinality enrtuples pg_node_attr(query_jumble_ignore); /* estimated or actual from caller */
/*
* Fields valid in all RTEs:
*/
- Alias *alias; /* user-written alias clause, if any */
- Alias *eref; /* expanded reference names */
- bool lateral; /* subquery, function, or values is LATERAL? */
+ Alias *alias pg_node_attr(query_jumble_ignore); /* user-written alias clause, if any */
+ Alias *eref pg_node_attr(query_jumble_ignore); /* expanded reference names */
+ bool lateral pg_node_attr(query_jumble_ignore); /* subquery, function, or values is LATERAL? */
bool inh; /* inheritance requested? */
- bool inFromCl; /* present in FROM clause? */
- List *securityQuals; /* security barrier quals to apply, if any */
+ bool inFromCl pg_node_attr(query_jumble_ignore); /* present in FROM clause? */
+ List *securityQuals pg_node_attr(query_jumble_ignore); /* security barrier quals to apply, if any */
} RangeTblEntry;
/*
--
2.43.0
v2-0003-Simplify-range_table_mutator_impl.patchtext/plain; charset=UTF-8; name=v2-0003-Simplify-range_table_mutator_impl.patchDownload
From df2898235350aafe82fb948b73e12a976e71237a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Jan 2024 11:23:39 +0100
Subject: [PATCH v2 3/4] Simplify range_table_mutator_impl()
This is part of an effort to reduce the number of special cases in the
node support functions.
Allegedly, only certain fields of RangeTblEntry are valid based on
rtekind. But exactly which ones seems to be documented and handled
inconsistently. It seems that over time, new RTE kinds have
"borrowed" fields that notionally belong to other RTE kinds, which is
technically not a problem but creates a bit of a mess when trying to
understand all this.
This patch removes the switch on rtekind from
range_table_mutator_impl(). This should be fine because all the
subroutines can handle unset/NULL fields. And it removes one more
place that needs to track knowledge about which fields are valid when.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
---
src/backend/nodes/nodeFuncs.c | 61 +++++++++++++----------------------
1 file changed, 23 insertions(+), 38 deletions(-)
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index e1a5bc7e95d..fe87238f195 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -3694,47 +3694,32 @@ range_table_mutator_impl(List *rtable,
RangeTblEntry *newrte;
FLATCOPY(newrte, rte, RangeTblEntry);
- switch (rte->rtekind)
+
+ MUTATE(newrte->tablesample, rte->tablesample, TableSampleClause *);
+
+ if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
+ MUTATE(newrte->subquery, rte->subquery, Query *);
+ else
{
- case RTE_RELATION:
- MUTATE(newrte->tablesample, rte->tablesample,
- TableSampleClause *);
- /* we don't bother to copy eref, aliases, etc; OK? */
- break;
- case RTE_SUBQUERY:
- if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
- MUTATE(newrte->subquery, rte->subquery, Query *);
- else
- {
- /* else, copy RT subqueries as-is */
- newrte->subquery = copyObject(rte->subquery);
- }
- break;
- case RTE_JOIN:
- if (!(flags & QTW_IGNORE_JOINALIASES))
- MUTATE(newrte->joinaliasvars, rte->joinaliasvars, List *);
- else
- {
- /* else, copy join aliases as-is */
- newrte->joinaliasvars = copyObject(rte->joinaliasvars);
- }
- break;
- case RTE_FUNCTION:
- MUTATE(newrte->functions, rte->functions, List *);
- break;
- case RTE_TABLEFUNC:
- MUTATE(newrte->tablefunc, rte->tablefunc, TableFunc *);
- break;
- case RTE_VALUES:
- MUTATE(newrte->values_lists, rte->values_lists, List *);
- break;
- case RTE_CTE:
- case RTE_NAMEDTUPLESTORE:
- case RTE_RESULT:
- /* nothing to do */
- break;
+ /* else, copy RT subqueries as-is */
+ newrte->subquery = copyObject(rte->subquery);
+ }
+
+ if (!(flags & QTW_IGNORE_JOINALIASES))
+ MUTATE(newrte->joinaliasvars, rte->joinaliasvars, List *);
+ else
+ {
+ /* else, copy join aliases as-is */
+ newrte->joinaliasvars = copyObject(rte->joinaliasvars);
}
+
+ MUTATE(newrte->functions, rte->functions, List *);
+ MUTATE(newrte->tablefunc, rte->tablefunc, TableFunc *);
+ MUTATE(newrte->values_lists, rte->values_lists, List *);
MUTATE(newrte->securityQuals, rte->securityQuals, List *);
+
+ /* we don't bother to copy eref, aliases, etc; OK? */
+
newrt = lappend(newrt, newrte);
}
return newrt;
--
2.43.0
v2-0004-WIP-Remove-custom-RangeTblEntry-node-read-write-i.patchtext/plain; charset=UTF-8; name=v2-0004-WIP-Remove-custom-RangeTblEntry-node-read-write-i.patchDownload
From 3f8c3217e529e2b5a07e9ea226fc71eb33e485ad Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Jan 2024 11:23:39 +0100
Subject: [PATCH v2 4/4] WIP: Remove custom RangeTblEntry node read/write
implementations
This is part of an effort to reduce the number of special cases in the
automatically generated node support functions.
Allegedly, only certain fields of RangeTblEntry are valid based on
rtekind. But exactly which ones seems to be documented and handled
inconsistently. It seems that over time, new RTE kinds have
"borrowed" fields that notionally belong to other RTE kinds, which is
technically not a problem but creates a bit of a mess when trying to
understand all this.
This patch removes the custom read/write functions for RangeTblEntry.
Those functions wanted to have a few fields at the front to make the
dump more legible; this is done now by moving the fields up in the
actual struct.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
TODO: clean up the documentation RangeTblEntry node
TODO: check how much this bloats stored rules
TODO: catversion
---
src/backend/nodes/outfuncs.c | 80 -----------------------------
src/backend/nodes/readfuncs.c | 92 ----------------------------------
src/include/nodes/parsenodes.h | 12 +++--
3 files changed, 8 insertions(+), 176 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index dee2b9e87b2..26cfd60aed9 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -489,86 +489,6 @@ _outExtensibleNode(StringInfo str, const ExtensibleNode *node)
methods->nodeOut(str, node);
}
-static void
-_outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
-{
- WRITE_NODE_TYPE("RANGETBLENTRY");
-
- /* put alias + eref first to make dump more legible */
- WRITE_NODE_FIELD(alias);
- WRITE_NODE_FIELD(eref);
- WRITE_ENUM_FIELD(rtekind, RTEKind);
-
- switch (node->rtekind)
- {
- case RTE_RELATION:
- WRITE_OID_FIELD(relid);
- WRITE_CHAR_FIELD(relkind);
- WRITE_INT_FIELD(rellockmode);
- WRITE_NODE_FIELD(tablesample);
- WRITE_UINT_FIELD(perminfoindex);
- break;
- case RTE_SUBQUERY:
- WRITE_NODE_FIELD(subquery);
- WRITE_BOOL_FIELD(security_barrier);
- /* we re-use these RELATION fields, too: */
- WRITE_OID_FIELD(relid);
- WRITE_CHAR_FIELD(relkind);
- WRITE_INT_FIELD(rellockmode);
- WRITE_UINT_FIELD(perminfoindex);
- break;
- case RTE_JOIN:
- WRITE_ENUM_FIELD(jointype, JoinType);
- WRITE_INT_FIELD(joinmergedcols);
- WRITE_NODE_FIELD(joinaliasvars);
- WRITE_NODE_FIELD(joinleftcols);
- WRITE_NODE_FIELD(joinrightcols);
- WRITE_NODE_FIELD(join_using_alias);
- break;
- case RTE_FUNCTION:
- WRITE_NODE_FIELD(functions);
- WRITE_BOOL_FIELD(funcordinality);
- break;
- case RTE_TABLEFUNC:
- WRITE_NODE_FIELD(tablefunc);
- break;
- case RTE_VALUES:
- WRITE_NODE_FIELD(values_lists);
- WRITE_NODE_FIELD(coltypes);
- WRITE_NODE_FIELD(coltypmods);
- WRITE_NODE_FIELD(colcollations);
- break;
- case RTE_CTE:
- WRITE_STRING_FIELD(ctename);
- WRITE_UINT_FIELD(ctelevelsup);
- WRITE_BOOL_FIELD(self_reference);
- WRITE_NODE_FIELD(coltypes);
- WRITE_NODE_FIELD(coltypmods);
- WRITE_NODE_FIELD(colcollations);
- break;
- case RTE_NAMEDTUPLESTORE:
- WRITE_STRING_FIELD(enrname);
- WRITE_FLOAT_FIELD(enrtuples);
- WRITE_NODE_FIELD(coltypes);
- WRITE_NODE_FIELD(coltypmods);
- WRITE_NODE_FIELD(colcollations);
- /* we re-use these RELATION fields, too: */
- WRITE_OID_FIELD(relid);
- break;
- case RTE_RESULT:
- /* no extra fields */
- break;
- default:
- elog(ERROR, "unrecognized RTE kind: %d", (int) node->rtekind);
- break;
- }
-
- WRITE_BOOL_FIELD(lateral);
- WRITE_BOOL_FIELD(inh);
- WRITE_BOOL_FIELD(inFromCl);
- WRITE_NODE_FIELD(securityQuals);
-}
-
static void
_outA_Expr(StringInfo str, const A_Expr *node)
{
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index b1e2f2b440a..54d3eecc1bc 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -343,98 +343,6 @@ _readA_Const(void)
READ_DONE();
}
-static RangeTblEntry *
-_readRangeTblEntry(void)
-{
- READ_LOCALS(RangeTblEntry);
-
- /* put alias + eref first to make dump more legible */
- READ_NODE_FIELD(alias);
- READ_NODE_FIELD(eref);
- READ_ENUM_FIELD(rtekind, RTEKind);
-
- switch (local_node->rtekind)
- {
- case RTE_RELATION:
- READ_OID_FIELD(relid);
- READ_CHAR_FIELD(relkind);
- READ_INT_FIELD(rellockmode);
- READ_NODE_FIELD(tablesample);
- READ_UINT_FIELD(perminfoindex);
- break;
- case RTE_SUBQUERY:
- READ_NODE_FIELD(subquery);
- READ_BOOL_FIELD(security_barrier);
- /* we re-use these RELATION fields, too: */
- READ_OID_FIELD(relid);
- READ_CHAR_FIELD(relkind);
- READ_INT_FIELD(rellockmode);
- READ_UINT_FIELD(perminfoindex);
- break;
- case RTE_JOIN:
- READ_ENUM_FIELD(jointype, JoinType);
- READ_INT_FIELD(joinmergedcols);
- READ_NODE_FIELD(joinaliasvars);
- READ_NODE_FIELD(joinleftcols);
- READ_NODE_FIELD(joinrightcols);
- READ_NODE_FIELD(join_using_alias);
- break;
- case RTE_FUNCTION:
- READ_NODE_FIELD(functions);
- READ_BOOL_FIELD(funcordinality);
- break;
- case RTE_TABLEFUNC:
- READ_NODE_FIELD(tablefunc);
- /* The RTE must have a copy of the column type info, if any */
- if (local_node->tablefunc)
- {
- TableFunc *tf = local_node->tablefunc;
-
- local_node->coltypes = tf->coltypes;
- local_node->coltypmods = tf->coltypmods;
- local_node->colcollations = tf->colcollations;
- }
- break;
- case RTE_VALUES:
- READ_NODE_FIELD(values_lists);
- READ_NODE_FIELD(coltypes);
- READ_NODE_FIELD(coltypmods);
- READ_NODE_FIELD(colcollations);
- break;
- case RTE_CTE:
- READ_STRING_FIELD(ctename);
- READ_UINT_FIELD(ctelevelsup);
- READ_BOOL_FIELD(self_reference);
- READ_NODE_FIELD(coltypes);
- READ_NODE_FIELD(coltypmods);
- READ_NODE_FIELD(colcollations);
- break;
- case RTE_NAMEDTUPLESTORE:
- READ_STRING_FIELD(enrname);
- READ_FLOAT_FIELD(enrtuples);
- READ_NODE_FIELD(coltypes);
- READ_NODE_FIELD(coltypmods);
- READ_NODE_FIELD(colcollations);
- /* we re-use these RELATION fields, too: */
- READ_OID_FIELD(relid);
- break;
- case RTE_RESULT:
- /* no extra fields */
- break;
- default:
- elog(ERROR, "unrecognized RTE kind: %d",
- (int) local_node->rtekind);
- break;
- }
-
- READ_BOOL_FIELD(lateral);
- READ_BOOL_FIELD(inh);
- READ_BOOL_FIELD(inFromCl);
- READ_NODE_FIELD(securityQuals);
-
- READ_DONE();
-}
-
static A_Expr *
_readA_Expr(void)
{
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index d377f16a72b..8042c5d91ed 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1018,8 +1018,6 @@ typedef enum RTEKind
typedef struct RangeTblEntry
{
- pg_node_attr(custom_read_write)
-
NodeTag type;
RTEKind rtekind; /* see above */
@@ -1030,6 +1028,14 @@ typedef struct RangeTblEntry
* code that is being actively worked on. FIXME someday.
*/
+ /*
+ * Fields valid in all RTEs:
+ *
+ * put alias + eref first to make dump more legible
+ */
+ Alias *alias pg_node_attr(query_jumble_ignore); /* user-written alias clause, if any */
+ Alias *eref pg_node_attr(query_jumble_ignore); /* expanded reference names */
+
/*
* Fields valid for a plain relation RTE (else zero):
*
@@ -1188,8 +1194,6 @@ typedef struct RangeTblEntry
/*
* Fields valid in all RTEs:
*/
- Alias *alias pg_node_attr(query_jumble_ignore); /* user-written alias clause, if any */
- Alias *eref pg_node_attr(query_jumble_ignore); /* expanded reference names */
bool lateral pg_node_attr(query_jumble_ignore); /* subquery, function, or values is LATERAL? */
bool inh; /* inheritance requested? */
bool inFromCl pg_node_attr(query_jumble_ignore); /* present in FROM clause? */
--
2.43.0
On 1/15/24 02:37, Peter Eisentraut wrote:
In this updated patch set, I have also added the treatment of the Constraint type. (I also noted
that the manual read/write functions for the Constraint type are out-of-sync again, so simplifying
this would be really helpful.) I have also added commit messages to each patch.The way I have re-ordered the patch series now, I think patches 0001 through 0003 are candidates for
inclusion after review, patch 0004 still needs a bit more analysis and testing, as described therein.
I had to apply the first patch by hand (on 9f13376396), so this looks due for a rebase. Patches 2-4
applied fine.
Compiles & passes tests after each patch.
The overall idea seems like a good improvement to me.
A few remarks about cleaning up the RangeTblEntry comments:
After the fourth patch we have a "Fields valid in all RTEs" comment twice in the struct, once at the
top and once at the bottom. It's fine IMO but maybe the second could be "More fields valid in all RTEs"?
The new order of fields in RangleTblEntry matches the intro comment, which seems like another small
benefit.
It seems like we are moving away from ever putting RTEKind-specific fields into a union as suggested
by the FIXME comment here. It was written in 2002. Is it time to remove it?
This now needs to say "above" not "below":
/*
* join_using_alias is an alias clause attached directly to JOIN/USING. It
* is different from the alias field (below) in that it does not hide the
* range variables of the tables being joined.
*/
Alias *join_using_alias pg_node_attr(query_jumble_ignore);
Re bloating the serialization output, we could leave this last patch until after the work on that
other thread is done to skip default-valued items.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
On Fri, 16 Feb 2024 at 21:36, Paul Jungwirth
<pj@illuminatedcomputing.com> wrote:
On 1/15/24 02:37, Peter Eisentraut wrote:
In this updated patch set, I have also added the treatment of the Constraint type. (I also noted
that the manual read/write functions for the Constraint type are out-of-sync again, so simplifying
this would be really helpful.) I have also added commit messages to each patch.The way I have re-ordered the patch series now, I think patches 0001 through 0003 are candidates for
inclusion after review, patch 0004 still needs a bit more analysis and testing, as described therein.Re bloating the serialization output, we could leave this last patch until after the work on that
other thread is done to skip default-valued items.
I'm not sure that the cleanup which is done when changing a RTE's
rtekind is also complete enough for this purpose.
Things like inline_cte_walker change the node->rtekind, which could
leave residual junk data in fields that are currently dropped during
serialization (as the rtekind specifically ignores those fields), but
which would add overhead when the default omission is expected to
handle these fields; as they could then contain junk. It looks like
there is some care about zeroing now unused fields, but I haven't
checked that it covers all cases and fields to the extent required so
that removing this specialized serializer would have zero impact on
size once the default omission patch is committed.
An additional patch with a single function that for this purpose
clears junk fields from RTEs that changed kind would be appreciated:
it is often hand-coded at those locations the kind changes, but that's
more sensitive to programmer error.
Kind regards,
Matthias van de Meent
On 18.02.24 00:06, Matthias van de Meent wrote:
I'm not sure that the cleanup which is done when changing a RTE's
rtekind is also complete enough for this purpose.
Things like inline_cte_walker change the node->rtekind, which could
leave residual junk data in fields that are currently dropped during
serialization (as the rtekind specifically ignores those fields), but
which would add overhead when the default omission is expected to
handle these fields; as they could then contain junk. It looks like
there is some care about zeroing now unused fields, but I haven't
checked that it covers all cases and fields to the extent required so
that removing this specialized serializer would have zero impact on
size once the default omission patch is committed.An additional patch with a single function that for this purpose
clears junk fields from RTEs that changed kind would be appreciated:
it is often hand-coded at those locations the kind changes, but that's
more sensitive to programmer error.
Yes, interesting idea. Or maybe an assert-like function that checks an
existing structure for consistency. Or maybe both. I'll try this out.
In the meantime, if there are no remaining concerns, I propose to commit
the first two patches
Remove custom Constraint node read/write implementations
Remove custom _jumbleRangeTblEntry()
On 20.02.24 08:57, Peter Eisentraut wrote:
On 18.02.24 00:06, Matthias van de Meent wrote:
I'm not sure that the cleanup which is done when changing a RTE's
rtekind is also complete enough for this purpose.
Things like inline_cte_walker change the node->rtekind, which could
leave residual junk data in fields that are currently dropped during
serialization (as the rtekind specifically ignores those fields), but
which would add overhead when the default omission is expected to
handle these fields; as they could then contain junk. It looks like
there is some care about zeroing now unused fields, but I haven't
checked that it covers all cases and fields to the extent required so
that removing this specialized serializer would have zero impact on
size once the default omission patch is committed.An additional patch with a single function that for this purpose
clears junk fields from RTEs that changed kind would be appreciated:
it is often hand-coded at those locations the kind changes, but that's
more sensitive to programmer error.Yes, interesting idea. Or maybe an assert-like function that checks an
existing structure for consistency. Or maybe both. I'll try this out.In the meantime, if there are no remaining concerns, I propose to commit
the first two patchesRemove custom Constraint node read/write implementations
Remove custom _jumbleRangeTblEntry()
After a few side quests, here is an updated patch set. (I had committed
the first of the two patches mentioned above, but not yet the second one.)
v3-0001-Remove-obsolete-comment.patch
v3-0002-Improve-comment.patch
These just update a few comments around the RangeTblEntry definition.
v3-0003-Reformat-some-node-comments.patch
v3-0004-Remove-custom-_jumbleRangeTblEntry.patch
This is pretty much the same patch as before. I have now split it up to
first reformat the comments to make room for the node annotations. This
patch is now also pgindent-proof. After some side quest discussions,
the set of fields to jumble seems correct now, so commit message
comments to the contrary have been dropped.
v3-0005-Make-RangeTblEntry-dump-order-consistent.patch
I separated that from the 0008 patch below. I think it useful even if
we don't go ahead with 0008 now, for example in dumps from the debugger,
and just in general to keep everything more consistent.
v3-0006-WIP-AssertRangeTblEntryIsValid.patch
This is in response to some of the discussions where there was some
doubt whether all fields are always filled and cleared correctly when
the RTE kind is changed. Seems correct as far as this goes. I didn't
know of a good way to hook this in, so I put it into the write/read
functions, which is obviously a bit weird if I'm proposing to remove
them later. Consider it a proof of concept.
v3-0007-Simplify-range_table_mutator_impl-and-range_table.patch
v3-0008-WIP-Remove-custom-RangeTblEntry-node-read-write-i.patch
At this point, I'm not too stressed about pressing forward with these
last two patches. We can look at them again perhaps if we make progress
on a more compact node output format. When I started this thread, I had
a lot of questions about various details about the RangeTblEntry struct,
and we have achieved many answers during the discussions, so I'm happy
with the progress. So for PG17, I'd like to just do patches 0001..0005.
Attachments:
v3-0001-Remove-obsolete-comment.patchtext/plain; charset=UTF-8; name=v3-0001-Remove-obsolete-comment.patchDownload
From dc53b7ddfc5aa004a0d222b4084a1c580f05a296 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 11 Mar 2024 09:15:33 +0100
Subject: [PATCH v3 1/8] Remove obsolete comment
The idea to use a union in the definition of RangeTblEntry is clearly
not being pursued.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
---
src/include/nodes/parsenodes.h | 6 ------
1 file changed, 6 deletions(-)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 2380821600..5113f97363 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1028,12 +1028,6 @@ typedef struct RangeTblEntry
RTEKind rtekind; /* see above */
- /*
- * XXX the fields applicable to only some rte kinds should be merged into
- * a union. I didn't do this yet because the diffs would impact a lot of
- * code that is being actively worked on. FIXME someday.
- */
-
/*
* Fields valid for a plain relation RTE (else zero):
*
base-commit: af0e7deb4a1c369bb8154ac55f085d6a93fe5c35
--
2.44.0
v3-0002-Improve-comment.patchtext/plain; charset=UTF-8; name=v3-0002-Improve-comment.patchDownload
From 9fd2ae6a561ea72f627057d922e48d92eaafe099 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 11 Mar 2024 09:15:33 +0100
Subject: [PATCH v3 2/8] Improve comment
Clarify that RangeTblEntry.lateral reflects whether LATERAL was
specified in the statement (as opposed to whether lateralness is
implicit). Also, the list of applicable entry types was incomplete.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
---
src/include/nodes/parsenodes.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 5113f97363..346dd5e0e2 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1196,7 +1196,7 @@ typedef struct RangeTblEntry
*/
Alias *alias; /* user-written alias clause, if any */
Alias *eref; /* expanded reference names */
- bool lateral; /* subquery, function, or values is LATERAL? */
+ bool lateral; /* was LATERAL specified? */
bool inFromCl; /* present in FROM clause? */
List *securityQuals; /* security barrier quals to apply, if any */
} RangeTblEntry;
--
2.44.0
v3-0003-Reformat-some-node-comments.patchtext/plain; charset=UTF-8; name=v3-0003-Reformat-some-node-comments.patchDownload
From 77fd2b4cdfdaf90f27a37c966d6aa490c4f6a419 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 11 Mar 2024 09:15:33 +0100
Subject: [PATCH v3 3/8] Reformat some node comments
Reformat some comments in node field definitions to avoid long lines.
This makes room for per-field annotations. Similar to 835d476fd2.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
---
src/include/nodes/parsenodes.h | 86 ++++++++++++++++++++++------------
1 file changed, 57 insertions(+), 29 deletions(-)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 346dd5e0e2..a3c4e86f4c 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1066,18 +1066,26 @@ typedef struct RangeTblEntry
* relation. This allows plans referencing AFTER trigger transition
* tables to be invalidated if the underlying table is altered.
*/
- Oid relid; /* OID of the relation */
- bool inh; /* inheritance requested? */
- char relkind; /* relation kind (see pg_class.relkind) */
- int rellockmode; /* lock level that query requires on the rel */
- Index perminfoindex; /* index of RTEPermissionInfo entry, or 0 */
- struct TableSampleClause *tablesample; /* sampling info, or NULL */
+ /* OID of the relation */
+ Oid relid;
+ /* inheritance requested? */
+ bool inh;
+ /* relation kind (see pg_class.relkind) */
+ char relkind;
+ /* lock level that query requires on the rel */
+ int rellockmode;
+ /* index of RTEPermissionInfo entry, or 0 */
+ Index perminfoindex;
+ /* sampling info, or NULL */
+ struct TableSampleClause *tablesample;
/*
* Fields valid for a subquery RTE (else NULL):
*/
- Query *subquery; /* the sub-query */
- bool security_barrier; /* is from security_barrier view? */
+ /* the sub-query */
+ Query *subquery;
+ /* is from security_barrier view? */
+ bool security_barrier;
/*
* Fields valid for a join RTE (else NULL/zero):
@@ -1122,11 +1130,15 @@ typedef struct RangeTblEntry
* merged columns could not be dropped); this is not accounted for in
* joinleftcols/joinrighttcols.
*/
- JoinType jointype; /* type of join */
- int joinmergedcols; /* number of merged (JOIN USING) columns */
- List *joinaliasvars; /* list of alias-var expansions */
- List *joinleftcols; /* left-side input column numbers */
- List *joinrightcols; /* right-side input column numbers */
+ JoinType jointype;
+ /* number of merged (JOIN USING) columns */
+ int joinmergedcols;
+ /* list of alias-var expansions */
+ List *joinaliasvars;
+ /* left-side input column numbers */
+ List *joinleftcols;
+ /* right-side input column numbers */
+ List *joinrightcols;
/*
* join_using_alias is an alias clause attached directly to JOIN/USING. It
@@ -1143,8 +1155,10 @@ typedef struct RangeTblEntry
* implicit, and must be accounted for "by hand" in places such as
* expandRTE().
*/
- List *functions; /* list of RangeTblFunction nodes */
- bool funcordinality; /* is this called WITH ORDINALITY? */
+ /* list of RangeTblFunction nodes */
+ List *functions;
+ /* is this called WITH ORDINALITY? */
+ bool funcordinality;
/*
* Fields valid for a TableFunc RTE (else NULL):
@@ -1154,14 +1168,18 @@ typedef struct RangeTblEntry
/*
* Fields valid for a values RTE (else NIL):
*/
- List *values_lists; /* list of expression lists */
+ /* list of expression lists */
+ List *values_lists;
/*
* Fields valid for a CTE RTE (else NULL/zero):
*/
- char *ctename; /* name of the WITH list item */
- Index ctelevelsup; /* number of query levels up */
- bool self_reference; /* is this a recursive self-reference? */
+ /* name of the WITH list item */
+ char *ctename;
+ /* number of query levels up */
+ Index ctelevelsup;
+ /* is this a recursive self-reference? */
+ bool self_reference;
/*
* Fields valid for CTE, VALUES, ENR, and TableFunc RTEs (else NIL):
@@ -1181,24 +1199,34 @@ typedef struct RangeTblEntry
* all three lists (as well as an empty-string entry in eref). Testing
* for zero coltype is the standard way to detect a dropped column.
*/
- List *coltypes; /* OID list of column type OIDs */
- List *coltypmods; /* integer list of column typmods */
- List *colcollations; /* OID list of column collation OIDs */
+ /* OID list of column type OIDs */
+ List *coltypes;
+ /* integer list of column typmods */
+ List *coltypmods;
+ /* OID list of column collation OIDs */
+ List *colcollations;
/*
* Fields valid for ENR RTEs (else NULL/zero):
*/
- char *enrname; /* name of ephemeral named relation */
- Cardinality enrtuples; /* estimated or actual from caller */
+ /* name of ephemeral named relation */
+ char *enrname;
+ /* estimated or actual from caller */
+ Cardinality enrtuples;
/*
* Fields valid in all RTEs:
*/
- Alias *alias; /* user-written alias clause, if any */
- Alias *eref; /* expanded reference names */
- bool lateral; /* was LATERAL specified? */
- bool inFromCl; /* present in FROM clause? */
- List *securityQuals; /* security barrier quals to apply, if any */
+ /* user-written alias clause, if any */
+ Alias *alias;
+ /* expanded reference names */
+ Alias *eref;
+ /* was LATERAL specified? */
+ bool lateral;
+ /* present in FROM clause? */
+ bool inFromCl;
+ /* security barrier quals to apply, if any */
+ List *securityQuals;
} RangeTblEntry;
/*
--
2.44.0
v3-0004-Remove-custom-_jumbleRangeTblEntry.patchtext/plain; charset=UTF-8; name=v3-0004-Remove-custom-_jumbleRangeTblEntry.patchDownload
From 70f2efa0d15d6533594814041dac2651efe78e8e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 11 Mar 2024 09:15:33 +0100
Subject: [PATCH v3 4/8] Remove custom _jumbleRangeTblEntry()
This is part of an effort to reduce the number of special cases in the
automatically generated node support functions.
This patch removes _jumbleRangeTblEntry() and instead adds per-field
query_jumble_ignore annotations to match the behavior of the previous
custom code. The pg_stat_statements test suite has some coverage of
this. It gets rid of switch on rtekind; this should be technically
correct, since we do the equal and copy functions like this also.
The list of fields to jumble has been checked and corrected as of
8b29a119fd.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
---
src/backend/nodes/queryjumblefuncs.c | 49 ----------------------------
src/include/nodes/parsenodes.h | 40 +++++++++++------------
2 files changed, 20 insertions(+), 69 deletions(-)
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index 2c116c8728..be823a7f8f 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -353,52 +353,3 @@ _jumbleA_Const(JumbleState *jstate, Node *node)
}
}
}
-
-static void
-_jumbleRangeTblEntry(JumbleState *jstate, Node *node)
-{
- RangeTblEntry *expr = (RangeTblEntry *) node;
-
- JUMBLE_FIELD(rtekind);
- switch (expr->rtekind)
- {
- case RTE_RELATION:
- JUMBLE_FIELD(relid);
- JUMBLE_FIELD(inh);
- JUMBLE_NODE(tablesample);
- break;
- case RTE_SUBQUERY:
- JUMBLE_NODE(subquery);
- break;
- case RTE_JOIN:
- JUMBLE_FIELD(jointype);
- break;
- case RTE_FUNCTION:
- JUMBLE_NODE(functions);
- JUMBLE_FIELD(funcordinality);
- break;
- case RTE_TABLEFUNC:
- JUMBLE_NODE(tablefunc);
- break;
- case RTE_VALUES:
- JUMBLE_NODE(values_lists);
- break;
- case RTE_CTE:
-
- /*
- * Depending on the CTE name here isn't ideal, but it's the only
- * info we have to identify the referenced WITH item.
- */
- JUMBLE_STRING(ctename);
- JUMBLE_FIELD(ctelevelsup);
- break;
- case RTE_NAMEDTUPLESTORE:
- JUMBLE_STRING(enrname);
- break;
- case RTE_RESULT:
- break;
- default:
- elog(ERROR, "unrecognized RTE kind: %d", (int) expr->rtekind);
- break;
- }
-}
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index a3c4e86f4c..aae50abdb0 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1022,7 +1022,7 @@ typedef enum RTEKind
typedef struct RangeTblEntry
{
- pg_node_attr(custom_read_write, custom_query_jumble)
+ pg_node_attr(custom_read_write)
NodeTag type;
@@ -1071,11 +1071,11 @@ typedef struct RangeTblEntry
/* inheritance requested? */
bool inh;
/* relation kind (see pg_class.relkind) */
- char relkind;
+ char relkind pg_node_attr(query_jumble_ignore);
/* lock level that query requires on the rel */
- int rellockmode;
+ int rellockmode pg_node_attr(query_jumble_ignore);
/* index of RTEPermissionInfo entry, or 0 */
- Index perminfoindex;
+ Index perminfoindex pg_node_attr(query_jumble_ignore);
/* sampling info, or NULL */
struct TableSampleClause *tablesample;
@@ -1085,7 +1085,7 @@ typedef struct RangeTblEntry
/* the sub-query */
Query *subquery;
/* is from security_barrier view? */
- bool security_barrier;
+ bool security_barrier pg_node_attr(query_jumble_ignore);
/*
* Fields valid for a join RTE (else NULL/zero):
@@ -1132,20 +1132,20 @@ typedef struct RangeTblEntry
*/
JoinType jointype;
/* number of merged (JOIN USING) columns */
- int joinmergedcols;
+ int joinmergedcols pg_node_attr(query_jumble_ignore);
/* list of alias-var expansions */
- List *joinaliasvars;
+ List *joinaliasvars pg_node_attr(query_jumble_ignore);
/* left-side input column numbers */
- List *joinleftcols;
+ List *joinleftcols pg_node_attr(query_jumble_ignore);
/* right-side input column numbers */
- List *joinrightcols;
+ List *joinrightcols pg_node_attr(query_jumble_ignore);
/*
* join_using_alias is an alias clause attached directly to JOIN/USING. It
* is different from the alias field (below) in that it does not hide the
* range variables of the tables being joined.
*/
- Alias *join_using_alias;
+ Alias *join_using_alias pg_node_attr(query_jumble_ignore);
/*
* Fields valid for a function RTE (else NIL/zero):
@@ -1179,7 +1179,7 @@ typedef struct RangeTblEntry
/* number of query levels up */
Index ctelevelsup;
/* is this a recursive self-reference? */
- bool self_reference;
+ bool self_reference pg_node_attr(query_jumble_ignore);
/*
* Fields valid for CTE, VALUES, ENR, and TableFunc RTEs (else NIL):
@@ -1200,11 +1200,11 @@ typedef struct RangeTblEntry
* for zero coltype is the standard way to detect a dropped column.
*/
/* OID list of column type OIDs */
- List *coltypes;
+ List *coltypes pg_node_attr(query_jumble_ignore);
/* integer list of column typmods */
- List *coltypmods;
+ List *coltypmods pg_node_attr(query_jumble_ignore);
/* OID list of column collation OIDs */
- List *colcollations;
+ List *colcollations pg_node_attr(query_jumble_ignore);
/*
* Fields valid for ENR RTEs (else NULL/zero):
@@ -1212,21 +1212,21 @@ typedef struct RangeTblEntry
/* name of ephemeral named relation */
char *enrname;
/* estimated or actual from caller */
- Cardinality enrtuples;
+ Cardinality enrtuples pg_node_attr(query_jumble_ignore);
/*
* Fields valid in all RTEs:
*/
/* user-written alias clause, if any */
- Alias *alias;
+ Alias *alias pg_node_attr(query_jumble_ignore);
/* expanded reference names */
- Alias *eref;
+ Alias *eref pg_node_attr(query_jumble_ignore);
/* was LATERAL specified? */
- bool lateral;
+ bool lateral pg_node_attr(query_jumble_ignore);
/* present in FROM clause? */
- bool inFromCl;
+ bool inFromCl pg_node_attr(query_jumble_ignore);
/* security barrier quals to apply, if any */
- List *securityQuals;
+ List *securityQuals pg_node_attr(query_jumble_ignore);
} RangeTblEntry;
/*
--
2.44.0
v3-0005-Make-RangeTblEntry-dump-order-consistent.patchtext/plain; charset=UTF-8; name=v3-0005-Make-RangeTblEntry-dump-order-consistent.patchDownload
From e0412cdbd0d84b39a40355042b3c99fd88273b8d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 11 Mar 2024 09:15:33 +0100
Subject: [PATCH v3 5/8] Make RangeTblEntry dump order consistent
Put the fields alias and eref earlier in the struct, so that it
matches the order in _outRangeTblEntry()/_readRangeTblEntry(). This
helps if we ever want to fully automate out/read of RangeTblEntry.
Also, it makes dumps in the debugger easier to read in the same way.
Internally, this makes no difference.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
TODO: catversion
---
src/backend/nodes/outfuncs.c | 1 -
src/backend/nodes/readfuncs.c | 1 -
src/include/nodes/parsenodes.h | 14 ++++++++++----
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 29cbc83bd9..c55375e7f9 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -494,7 +494,6 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
{
WRITE_NODE_TYPE("RANGETBLENTRY");
- /* put alias + eref first to make dump more legible */
WRITE_NODE_FIELD(alias);
WRITE_NODE_FIELD(eref);
WRITE_ENUM_FIELD(rtekind, RTEKind);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index a122407c88..c4d01a441a 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -348,7 +348,6 @@ _readRangeTblEntry(void)
{
READ_LOCALS(RangeTblEntry);
- /* put alias + eref first to make dump more legible */
READ_NODE_FIELD(alias);
READ_NODE_FIELD(eref);
READ_ENUM_FIELD(rtekind, RTEKind);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index aae50abdb0..6192492df8 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1026,6 +1026,16 @@ typedef struct RangeTblEntry
NodeTag type;
+ /*
+ * Fields valid in all RTEs:
+ *
+ * put alias + eref first to make dump more legible
+ */
+ /* user-written alias clause, if any */
+ Alias *alias pg_node_attr(query_jumble_ignore);
+ /* expanded reference names */
+ Alias *eref pg_node_attr(query_jumble_ignore);
+
RTEKind rtekind; /* see above */
/*
@@ -1217,10 +1227,6 @@ typedef struct RangeTblEntry
/*
* Fields valid in all RTEs:
*/
- /* user-written alias clause, if any */
- Alias *alias pg_node_attr(query_jumble_ignore);
- /* expanded reference names */
- Alias *eref pg_node_attr(query_jumble_ignore);
/* was LATERAL specified? */
bool lateral pg_node_attr(query_jumble_ignore);
/* present in FROM clause? */
--
2.44.0
v3-0006-WIP-AssertRangeTblEntryIsValid.patchtext/plain; charset=UTF-8; name=v3-0006-WIP-AssertRangeTblEntryIsValid.patchDownload
From 357cfbc2622480abf6a041dc3715f439b74c789f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 11 Mar 2024 09:15:33 +0100
Subject: [PATCH v3 6/8] WIP: AssertRangeTblEntryIsValid()
This provides a function AssertRangeTblEntryIsValid() that checks that
a RangeTblEntry node is filled with a valid combination of fields.
This is hooked into the write/read functions so that it is executed by
WRITE_READ_PARSE_PLAN_TREES.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
---
src/backend/nodes/nodeFuncs.c | 59 +++++++++++++++++++++++++++++++++++
src/backend/nodes/outfuncs.c | 2 ++
src/backend/nodes/readfuncs.c | 2 ++
src/include/nodes/nodeFuncs.h | 6 ++++
4 files changed, 69 insertions(+)
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 6ba8e73256..db1b9bec13 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -4571,3 +4571,62 @@ planstate_walk_members(PlanState **planstates, int nplans,
return false;
}
+
+#ifdef USE_ASSERT_CHECKING
+
+/*
+ * Assertion check that a RangeTblEntry node is filled with a valid
+ * combination of fields.
+ *
+ * Best used together with WRITE_READ_PARSE_PLAN_TREES.
+ */
+void
+AssertRangeTblEntryIsValid(const RangeTblEntry *rte)
+{
+ Assert(rte->rtekind == RTE_RELATION ||
+ rte->rtekind == RTE_SUBQUERY ||
+ rte->rtekind == RTE_JOIN ||
+ rte->rtekind == RTE_FUNCTION ||
+ rte->rtekind == RTE_TABLEFUNC ||
+ rte->rtekind == RTE_VALUES ||
+ rte->rtekind == RTE_CTE ||
+ rte->rtekind == RTE_NAMEDTUPLESTORE ||
+ rte->rtekind == RTE_RESULT);
+
+ Assert(rte->eref);
+
+ if (rte->relid)
+ Assert(rte->rtekind == RTE_RELATION || rte->rtekind == RTE_SUBQUERY || rte->rtekind == RTE_NAMEDTUPLESTORE);
+ if (rte->inh)
+ Assert(rte->rtekind == RTE_RELATION || rte->rtekind == RTE_SUBQUERY);
+ if (rte->relkind)
+ Assert(rte->rtekind == RTE_RELATION || rte->rtekind == RTE_SUBQUERY);
+ if (rte->rellockmode)
+ Assert(rte->rtekind == RTE_RELATION || rte->rtekind == RTE_SUBQUERY);
+ if (rte->tablesample)
+ Assert(rte->rtekind == RTE_RELATION);
+ if (rte->perminfoindex)
+ Assert(rte->rtekind == RTE_RELATION || rte->rtekind == RTE_SUBQUERY);
+ if (rte->subquery)
+ Assert(rte->rtekind == RTE_SUBQUERY);
+ if (rte->security_barrier)
+ Assert(rte->rtekind == RTE_SUBQUERY);
+ if (rte->joinmergedcols || rte->joinaliasvars || rte->joinleftcols || rte->joinrightcols || rte->join_using_alias)
+ Assert(rte->rtekind == RTE_JOIN);
+ if (rte->functions || rte->funcordinality)
+ Assert(rte->rtekind == RTE_FUNCTION);
+ if (rte->tablefunc)
+ Assert(rte->rtekind == RTE_TABLEFUNC);
+ if (rte->values_lists)
+ Assert(rte->rtekind == RTE_VALUES);
+ if (rte->ctename || rte->ctelevelsup || rte->self_reference)
+ Assert(rte->rtekind == RTE_CTE);
+ if (rte->coltypes || rte->coltypmods || rte->colcollations)
+ Assert(rte->rtekind == RTE_TABLEFUNC || rte->rtekind == RTE_VALUES || rte->rtekind == RTE_CTE || rte->rtekind == RTE_NAMEDTUPLESTORE);
+ if (rte->enrname || rte->enrtuples)
+ Assert(rte->rtekind == RTE_NAMEDTUPLESTORE);
+ if (rte->lateral)
+ Assert(rte->rtekind == RTE_RELATION || rte->rtekind == RTE_SUBQUERY || rte->rtekind == RTE_FUNCTION || rte->rtekind == RTE_TABLEFUNC || rte->rtekind == RTE_VALUES);
+}
+
+#endif
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index c55375e7f9..5e34584c55 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -21,6 +21,7 @@
#include "lib/stringinfo.h"
#include "miscadmin.h"
#include "nodes/bitmapset.h"
+#include "nodes/nodeFuncs.h"
#include "nodes/nodes.h"
#include "nodes/pg_list.h"
#include "utils/datum.h"
@@ -492,6 +493,7 @@ _outExtensibleNode(StringInfo str, const ExtensibleNode *node)
static void
_outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
{
+ AssertRangeTblEntryIsValid(node);
WRITE_NODE_TYPE("RANGETBLENTRY");
WRITE_NODE_FIELD(alias);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index c4d01a441a..217d81041a 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -30,6 +30,7 @@
#include "miscadmin.h"
#include "nodes/bitmapset.h"
+#include "nodes/nodeFuncs.h"
#include "nodes/readfuncs.h"
@@ -432,6 +433,7 @@ _readRangeTblEntry(void)
READ_BOOL_FIELD(inFromCl);
READ_NODE_FIELD(securityQuals);
+ AssertRangeTblEntryIsValid(local_node);
READ_DONE();
}
diff --git a/src/include/nodes/nodeFuncs.h b/src/include/nodes/nodeFuncs.h
index eaba59bed8..873ed9e9b7 100644
--- a/src/include/nodes/nodeFuncs.h
+++ b/src/include/nodes/nodeFuncs.h
@@ -219,4 +219,10 @@ extern bool planstate_tree_walker_impl(struct PlanState *planstate,
planstate_tree_walker_callback walker,
void *context);
+#ifdef USE_ASSERT_CHECKING
+extern void AssertRangeTblEntryIsValid(const RangeTblEntry *rte);
+#else
+#define AssertRangeTblEntryIsValid(rte) ((void)true)
+#endif
+
#endif /* NODEFUNCS_H */
--
2.44.0
v3-0007-Simplify-range_table_mutator_impl-and-range_table.patchtext/plain; charset=UTF-8; name=v3-0007-Simplify-range_table_mutator_impl-and-range_table.patchDownload
From b2332f2ea03307ee6bc0e2e62fa5f364f7340039 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 11 Mar 2024 09:15:33 +0100
Subject: [PATCH v3 7/8] Simplify range_table_mutator_impl() and
range_table_entry_walker_impl()
This is part of an effort to reduce the number of special cases in the
node support functions.
This patch removes the switch on rtekind from
range_table_mutator_impl() and range_table_entry_walker_impl(). This
should be fine because all the subroutines can handle unset/NULL
fields. And it removes one more place that needs to track knowledge
about which fields are valid when.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
---
src/backend/nodes/nodeFuncs.c | 110 ++++++++++++----------------------
1 file changed, 37 insertions(+), 73 deletions(-)
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index db1b9bec13..410f700ee5 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2699,41 +2699,20 @@ range_table_entry_walker_impl(RangeTblEntry *rte,
if (WALK(rte))
return true;
- switch (rte->rtekind)
- {
- case RTE_RELATION:
- if (WALK(rte->tablesample))
- return true;
- break;
- case RTE_SUBQUERY:
- if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
- if (WALK(rte->subquery))
- return true;
- break;
- case RTE_JOIN:
- if (!(flags & QTW_IGNORE_JOINALIASES))
- if (WALK(rte->joinaliasvars))
- return true;
- break;
- case RTE_FUNCTION:
- if (WALK(rte->functions))
- return true;
- break;
- case RTE_TABLEFUNC:
- if (WALK(rte->tablefunc))
- return true;
- break;
- case RTE_VALUES:
- if (WALK(rte->values_lists))
- return true;
- break;
- case RTE_CTE:
- case RTE_NAMEDTUPLESTORE:
- case RTE_RESULT:
- /* nothing to do */
- break;
- }
-
+ if (WALK(rte->tablesample))
+ return true;
+ if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
+ if (WALK(rte->subquery))
+ return true;
+ if (!(flags & QTW_IGNORE_JOINALIASES))
+ if (WALK(rte->joinaliasvars))
+ return true;
+ if (WALK(rte->functions))
+ return true;
+ if (WALK(rte->tablefunc))
+ return true;
+ if (WALK(rte->values_lists))
+ return true;
if (WALK(rte->securityQuals))
return true;
@@ -3693,47 +3672,32 @@ range_table_mutator_impl(List *rtable,
RangeTblEntry *newrte;
FLATCOPY(newrte, rte, RangeTblEntry);
- switch (rte->rtekind)
+
+ MUTATE(newrte->tablesample, rte->tablesample, TableSampleClause *);
+
+ if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
+ MUTATE(newrte->subquery, rte->subquery, Query *);
+ else
{
- case RTE_RELATION:
- MUTATE(newrte->tablesample, rte->tablesample,
- TableSampleClause *);
- /* we don't bother to copy eref, aliases, etc; OK? */
- break;
- case RTE_SUBQUERY:
- if (!(flags & QTW_IGNORE_RT_SUBQUERIES))
- MUTATE(newrte->subquery, rte->subquery, Query *);
- else
- {
- /* else, copy RT subqueries as-is */
- newrte->subquery = copyObject(rte->subquery);
- }
- break;
- case RTE_JOIN:
- if (!(flags & QTW_IGNORE_JOINALIASES))
- MUTATE(newrte->joinaliasvars, rte->joinaliasvars, List *);
- else
- {
- /* else, copy join aliases as-is */
- newrte->joinaliasvars = copyObject(rte->joinaliasvars);
- }
- break;
- case RTE_FUNCTION:
- MUTATE(newrte->functions, rte->functions, List *);
- break;
- case RTE_TABLEFUNC:
- MUTATE(newrte->tablefunc, rte->tablefunc, TableFunc *);
- break;
- case RTE_VALUES:
- MUTATE(newrte->values_lists, rte->values_lists, List *);
- break;
- case RTE_CTE:
- case RTE_NAMEDTUPLESTORE:
- case RTE_RESULT:
- /* nothing to do */
- break;
+ /* else, copy RT subqueries as-is */
+ newrte->subquery = copyObject(rte->subquery);
}
+
+ if (!(flags & QTW_IGNORE_JOINALIASES))
+ MUTATE(newrte->joinaliasvars, rte->joinaliasvars, List *);
+ else
+ {
+ /* else, copy join aliases as-is */
+ newrte->joinaliasvars = copyObject(rte->joinaliasvars);
+ }
+
+ MUTATE(newrte->functions, rte->functions, List *);
+ MUTATE(newrte->tablefunc, rte->tablefunc, TableFunc *);
+ MUTATE(newrte->values_lists, rte->values_lists, List *);
MUTATE(newrte->securityQuals, rte->securityQuals, List *);
+
+ /* we don't bother to copy eref, aliases, etc; OK? */
+
newrt = lappend(newrt, newrte);
}
return newrt;
--
2.44.0
v3-0008-WIP-Remove-custom-RangeTblEntry-node-read-write-i.patchtext/plain; charset=UTF-8; name=v3-0008-WIP-Remove-custom-RangeTblEntry-node-read-write-i.patchDownload
From ef4c35a8aa6e6e855814629545cacc97fbcb9066 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 11 Mar 2024 09:15:33 +0100
Subject: [PATCH v3 8/8] WIP: Remove custom RangeTblEntry node read/write
implementations
This is part of an effort to reduce the number of special cases in the
automatically generated node support functions.
This patch removes the custom read/write functions for RangeTblEntry.
Discussion: https://www.postgresql.org/message-id/flat/4b27fc50-8cd6-46f5-ab20-88dbaadca645@eisentraut.org
TODO: check how much this bloats stored rules
TODO: catversion
---
src/backend/nodes/outfuncs.c | 81 -----------------------------
src/backend/nodes/readfuncs.c | 93 ----------------------------------
src/include/nodes/parsenodes.h | 2 -
3 files changed, 176 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 5e34584c55..dbc316ed32 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -490,87 +490,6 @@ _outExtensibleNode(StringInfo str, const ExtensibleNode *node)
methods->nodeOut(str, node);
}
-static void
-_outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
-{
- AssertRangeTblEntryIsValid(node);
- WRITE_NODE_TYPE("RANGETBLENTRY");
-
- WRITE_NODE_FIELD(alias);
- WRITE_NODE_FIELD(eref);
- WRITE_ENUM_FIELD(rtekind, RTEKind);
-
- switch (node->rtekind)
- {
- case RTE_RELATION:
- WRITE_OID_FIELD(relid);
- WRITE_BOOL_FIELD(inh);
- WRITE_CHAR_FIELD(relkind);
- WRITE_INT_FIELD(rellockmode);
- WRITE_UINT_FIELD(perminfoindex);
- WRITE_NODE_FIELD(tablesample);
- break;
- case RTE_SUBQUERY:
- WRITE_NODE_FIELD(subquery);
- WRITE_BOOL_FIELD(security_barrier);
- /* we re-use these RELATION fields, too: */
- WRITE_OID_FIELD(relid);
- WRITE_BOOL_FIELD(inh);
- WRITE_CHAR_FIELD(relkind);
- WRITE_INT_FIELD(rellockmode);
- WRITE_UINT_FIELD(perminfoindex);
- break;
- case RTE_JOIN:
- WRITE_ENUM_FIELD(jointype, JoinType);
- WRITE_INT_FIELD(joinmergedcols);
- WRITE_NODE_FIELD(joinaliasvars);
- WRITE_NODE_FIELD(joinleftcols);
- WRITE_NODE_FIELD(joinrightcols);
- WRITE_NODE_FIELD(join_using_alias);
- break;
- case RTE_FUNCTION:
- WRITE_NODE_FIELD(functions);
- WRITE_BOOL_FIELD(funcordinality);
- break;
- case RTE_TABLEFUNC:
- WRITE_NODE_FIELD(tablefunc);
- break;
- case RTE_VALUES:
- WRITE_NODE_FIELD(values_lists);
- WRITE_NODE_FIELD(coltypes);
- WRITE_NODE_FIELD(coltypmods);
- WRITE_NODE_FIELD(colcollations);
- break;
- case RTE_CTE:
- WRITE_STRING_FIELD(ctename);
- WRITE_UINT_FIELD(ctelevelsup);
- WRITE_BOOL_FIELD(self_reference);
- WRITE_NODE_FIELD(coltypes);
- WRITE_NODE_FIELD(coltypmods);
- WRITE_NODE_FIELD(colcollations);
- break;
- case RTE_NAMEDTUPLESTORE:
- WRITE_STRING_FIELD(enrname);
- WRITE_FLOAT_FIELD(enrtuples);
- WRITE_NODE_FIELD(coltypes);
- WRITE_NODE_FIELD(coltypmods);
- WRITE_NODE_FIELD(colcollations);
- /* we re-use these RELATION fields, too: */
- WRITE_OID_FIELD(relid);
- break;
- case RTE_RESULT:
- /* no extra fields */
- break;
- default:
- elog(ERROR, "unrecognized RTE kind: %d", (int) node->rtekind);
- break;
- }
-
- WRITE_BOOL_FIELD(lateral);
- WRITE_BOOL_FIELD(inFromCl);
- WRITE_NODE_FIELD(securityQuals);
-}
-
static void
_outA_Expr(StringInfo str, const A_Expr *node)
{
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 217d81041a..1a322659be 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -344,99 +344,6 @@ _readA_Const(void)
READ_DONE();
}
-static RangeTblEntry *
-_readRangeTblEntry(void)
-{
- READ_LOCALS(RangeTblEntry);
-
- READ_NODE_FIELD(alias);
- READ_NODE_FIELD(eref);
- READ_ENUM_FIELD(rtekind, RTEKind);
-
- switch (local_node->rtekind)
- {
- case RTE_RELATION:
- READ_OID_FIELD(relid);
- READ_BOOL_FIELD(inh);
- READ_CHAR_FIELD(relkind);
- READ_INT_FIELD(rellockmode);
- READ_UINT_FIELD(perminfoindex);
- READ_NODE_FIELD(tablesample);
- break;
- case RTE_SUBQUERY:
- READ_NODE_FIELD(subquery);
- READ_BOOL_FIELD(security_barrier);
- /* we re-use these RELATION fields, too: */
- READ_OID_FIELD(relid);
- READ_BOOL_FIELD(inh);
- READ_CHAR_FIELD(relkind);
- READ_INT_FIELD(rellockmode);
- READ_UINT_FIELD(perminfoindex);
- break;
- case RTE_JOIN:
- READ_ENUM_FIELD(jointype, JoinType);
- READ_INT_FIELD(joinmergedcols);
- READ_NODE_FIELD(joinaliasvars);
- READ_NODE_FIELD(joinleftcols);
- READ_NODE_FIELD(joinrightcols);
- READ_NODE_FIELD(join_using_alias);
- break;
- case RTE_FUNCTION:
- READ_NODE_FIELD(functions);
- READ_BOOL_FIELD(funcordinality);
- break;
- case RTE_TABLEFUNC:
- READ_NODE_FIELD(tablefunc);
- /* The RTE must have a copy of the column type info, if any */
- if (local_node->tablefunc)
- {
- TableFunc *tf = local_node->tablefunc;
-
- local_node->coltypes = tf->coltypes;
- local_node->coltypmods = tf->coltypmods;
- local_node->colcollations = tf->colcollations;
- }
- break;
- case RTE_VALUES:
- READ_NODE_FIELD(values_lists);
- READ_NODE_FIELD(coltypes);
- READ_NODE_FIELD(coltypmods);
- READ_NODE_FIELD(colcollations);
- break;
- case RTE_CTE:
- READ_STRING_FIELD(ctename);
- READ_UINT_FIELD(ctelevelsup);
- READ_BOOL_FIELD(self_reference);
- READ_NODE_FIELD(coltypes);
- READ_NODE_FIELD(coltypmods);
- READ_NODE_FIELD(colcollations);
- break;
- case RTE_NAMEDTUPLESTORE:
- READ_STRING_FIELD(enrname);
- READ_FLOAT_FIELD(enrtuples);
- READ_NODE_FIELD(coltypes);
- READ_NODE_FIELD(coltypmods);
- READ_NODE_FIELD(colcollations);
- /* we re-use these RELATION fields, too: */
- READ_OID_FIELD(relid);
- break;
- case RTE_RESULT:
- /* no extra fields */
- break;
- default:
- elog(ERROR, "unrecognized RTE kind: %d",
- (int) local_node->rtekind);
- break;
- }
-
- READ_BOOL_FIELD(lateral);
- READ_BOOL_FIELD(inFromCl);
- READ_NODE_FIELD(securityQuals);
-
- AssertRangeTblEntryIsValid(local_node);
- READ_DONE();
-}
-
static A_Expr *
_readA_Expr(void)
{
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 6192492df8..b3b5a6cdf1 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1022,8 +1022,6 @@ typedef enum RTEKind
typedef struct RangeTblEntry
{
- pg_node_attr(custom_read_write)
-
NodeTag type;
/*
--
2.44.0
On Mon, Mar 11, 2024 at 5:29 AM Peter Eisentraut <peter@eisentraut.org>
wrote:
On 20.02.24 08:57, Peter Eisentraut wrote:
On 18.02.24 00:06, Matthias van de Meent wrote:
I'm not sure that the cleanup which is done when changing a RTE's
rtekind is also complete enough for this purpose.
Things like inline_cte_walker change the node->rtekind, which could
leave residual junk data in fields that are currently dropped during
serialization (as the rtekind specifically ignores those fields), but
which would add overhead when the default omission is expected to
handle these fields; as they could then contain junk. It looks like
there is some care about zeroing now unused fields, but I haven't
checked that it covers all cases and fields to the extent required so
that removing this specialized serializer would have zero impact on
size once the default omission patch is committed.An additional patch with a single function that for this purpose
clears junk fields from RTEs that changed kind would be appreciated:
it is often hand-coded at those locations the kind changes, but that's
more sensitive to programmer error.Yes, interesting idea. Or maybe an assert-like function that checks an
existing structure for consistency. Or maybe both. I'll try this out.In the meantime, if there are no remaining concerns, I propose to commit
the first two patchesRemove custom Constraint node read/write implementations
Remove custom _jumbleRangeTblEntry()After a few side quests, here is an updated patch set. (I had committed
the first of the two patches mentioned above, but not yet the second one.)v3-0001-Remove-obsolete-comment.patch
v3-0002-Improve-comment.patchThese just update a few comments around the RangeTblEntry definition.
v3-0003-Reformat-some-node-comments.patch
v3-0004-Remove-custom-_jumbleRangeTblEntry.patchThis is pretty much the same patch as before. I have now split it up to
first reformat the comments to make room for the node annotations. This
patch is now also pgindent-proof. After some side quest discussions,
the set of fields to jumble seems correct now, so commit message
comments to the contrary have been dropped.v3-0005-Make-RangeTblEntry-dump-order-consistent.patch
I separated that from the 0008 patch below. I think it useful even if
we don't go ahead with 0008 now, for example in dumps from the debugger,
and just in general to keep everything more consistent.v3-0006-WIP-AssertRangeTblEntryIsValid.patch
This is in response to some of the discussions where there was some
doubt whether all fields are always filled and cleared correctly when
the RTE kind is changed. Seems correct as far as this goes. I didn't
know of a good way to hook this in, so I put it into the write/read
functions, which is obviously a bit weird if I'm proposing to remove
them later. Consider it a proof of concept.v3-0007-Simplify-range_table_mutator_impl-and-range_table.patch
v3-0008-WIP-Remove-custom-RangeTblEntry-node-read-write-i.patchAt this point, I'm not too stressed about pressing forward with these
last two patches. We can look at them again perhaps if we make progress
on a more compact node output format. When I started this thread, I had
a lot of questions about various details about the RangeTblEntry struct,
and we have achieved many answers during the discussions, so I'm happy
with the progress. So for PG17, I'd like to just do patches 0001..0005.
Patches 1 thru 5 look good to me
cheers
andrew
On 21.03.24 10:51, Andrew Dunstan wrote:
At this point, I'm not too stressed about pressing forward with these
last two patches. We can look at them again perhaps if we make
progress
on a more compact node output format. When I started this thread, I
had
a lot of questions about various details about the RangeTblEntry
struct,
and we have achieved many answers during the discussions, so I'm happy
with the progress. So for PG17, I'd like to just do patches 0001..0005.Patches 1 thru 5 look good to me
Thanks for checking. I have committed these (1 through 5) and will
close the commit fest entry.