On Thu, Feb 09, 2023 at 06:12:50PM -0500, Tom Lane wrote: > I'm okay with the pathnodes.h changes --- although surely you don't need > changes like this: > > - pg_node_attr(abstract) > + pg_node_attr(abstract, no_query_jumble) > > "abstract" should already imply "no_query_jumble".
Okay, understood. Following this string of thoughts, I am a bit
surprised for two cases, though:
- PartitionPruneStep.
- Plan.
Both are abstract and both are marked with no_equal. I guess that
applying no_query_jumble to both of them is fine, and that's what you
mean?
> I wonder too if you could shorten the changes by making no_query_jumble
> an inheritable attribute, and then just applying it to Path and Plan.
Ah. I did not catch what you meant here at first, but I think that I
do now. Are you referring to the part of gen_node_support.pl where we
propagate properties when a node is a supertype? This part would be
taken into account when a node is parsed but we find that its first
member is already tracked as a node:
# Propagate some node attributes from supertypes
if ($supertype)
{
push @no_copy, $in_struct
if elem $supertype, @no_copy;
push @no_equal, $in_struct
if elem $supertype, @no_equal;
push @no_read, $in_struct
if elem $supertype, @no_read;
+ push @no_query_jumble, $in_struct
+ if elem $supertype, @no_query_jumble;
}
A benefit of doing that would also discard all the Scan and Sort
nodes. So like the other no_* attributes, it makes sense to force the
inheritance here.
> The changes in parsenodes.h seem wrong, except for RawStmt. Those node
> types are used in parsed queries, aren't they?
RTEPermissionInfo is a recent addition, as of a61b1f7. This commit
documents it as a plan node, still it is part of a Query while being
ignored in the query jumbling since its introduction, so I am a bit
confused by this one.
Anyway, none of these need to be included in the query jumbling
currently because they are ignored, but I'd be fine to generate their
code by default as they could become relevant if other nodes begin to
rely on them more heavily, as being part of queries. Peter E. has
mentioned upthread that a few nodes should include more jumbling while
some other parts should be ignored. This should be analyzed
separately because ~15 does not seem to be strictly right, either.
Attached is a patch refreshed with all that. Feel free to ignore 0002
as that's just useful to enforce the tests to go through the jumbling
code. The attached reaches 95.0% of line coverage after a check-world
in funcs.c.
Thoughts?
--
Michael
From 316df43f7574f54ff93606549167102e319f5473 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 10 Feb 2023 11:15:24 +0900
Subject: [PATCH v2 1/2] Mark more nodes with node attribute no_query_jumble
This mostly covers Plan and Path nodes, which should never be included
in the jumbling because we ignore these in Query nodes. no_query_jumble
is now an inherited attribute to be able to achieve that, like its read
and write counterparts.
---
src/include/nodes/nodes.h | 8 ++--
src/include/nodes/parsenodes.h | 5 +++
src/include/nodes/pathnodes.h | 54 ++++++++++++++-------------
src/include/nodes/plannodes.h | 16 ++++----
src/include/nodes/primnodes.h | 4 ++
src/backend/nodes/gen_node_support.pl | 2 +
6 files changed, 52 insertions(+), 37 deletions(-)
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 75dfe1919d..e6cf520547 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -77,10 +77,10 @@ typedef enum NodeTag
*
* Node types can be supertypes of other types whether or not they are marked
* abstract: if a node struct appears as the first field of another struct
- * type, then it is the supertype of that type. The no_copy, no_equal, and
- * no_read node attributes are automatically inherited from the supertype.
- * (Notice that nodetag_only does not inherit, so it's not quite equivalent
- * to a combination of other attributes.)
+ * type, then it is the supertype of that type. The no_copy, no_equal,
+ * no_query_jumble and no_read node attributes are automatically inherited
+ * from the supertype. (Notice that nodetag_only does not inherit, so it's
+ * not quite equivalent to a combination of other attributes.)
*
* Valid node field attributes:
*
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 855da99ec0..9c97148b69 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1728,9 +1728,14 @@ typedef struct TriggerTransition
*
* stmt_location/stmt_len identify the portion of the source text string
* containing this raw statement (useful for multi-statement strings).
+ *
+ * This is irrelevant for query jumbling, as this is not used in parsed
+ * queries.
*/
typedef struct RawStmt
{
+ pg_node_attr(no_query_jumble)
+
NodeTag type;
Node *stmt; /* raw parse tree */
int stmt_location; /* start location, or -1 if unknown */
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 0d4b1ec4e4..be4d791212 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -94,7 +94,7 @@ typedef enum UpperRelationKind
*/
typedef struct PlannerGlobal
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -194,7 +194,7 @@ typedef struct PlannerInfo PlannerInfo;
struct PlannerInfo
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -853,7 +853,7 @@ typedef enum RelOptKind
typedef struct RelOptInfo
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -1098,7 +1098,7 @@ typedef struct IndexOptInfo IndexOptInfo;
struct IndexOptInfo
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -1208,7 +1208,7 @@ struct IndexOptInfo
*/
typedef struct ForeignKeyOptInfo
{
- pg_node_attr(custom_read_write, no_copy_equal, no_read)
+ pg_node_attr(custom_read_write, no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -1258,7 +1258,7 @@ typedef struct ForeignKeyOptInfo
*/
typedef struct StatisticExtInfo
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -1309,7 +1309,7 @@ typedef struct StatisticExtInfo
*/
typedef struct JoinDomain
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -1371,7 +1371,7 @@ typedef struct JoinDomain
*/
typedef struct EquivalenceClass
{
- pg_node_attr(custom_read_write, no_copy_equal, no_read)
+ pg_node_attr(custom_read_write, no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -1422,7 +1422,7 @@ typedef struct EquivalenceClass
*/
typedef struct EquivalenceMember
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -1455,7 +1455,7 @@ typedef struct EquivalenceMember
*/
typedef struct PathKey
{
- pg_node_attr(no_read)
+ pg_node_attr(no_read, no_query_jumble)
NodeTag type;
@@ -1503,7 +1503,7 @@ typedef enum VolatileFunctionStatus
*/
typedef struct PathTarget
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -1550,7 +1550,7 @@ typedef struct PathTarget
*/
typedef struct ParamPathInfo
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -1596,7 +1596,7 @@ typedef struct ParamPathInfo
*/
typedef struct Path
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -1730,7 +1730,7 @@ typedef struct IndexPath
*/
typedef struct IndexClause
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
struct RestrictInfo *rinfo; /* original restriction or join clause */
@@ -2231,7 +2231,7 @@ typedef struct AggPath
typedef struct GroupingSetData
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
List *set; /* grouping set as list of sortgrouprefs */
@@ -2240,7 +2240,7 @@ typedef struct GroupingSetData
typedef struct RollupData
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
List *groupClause; /* applicable subset of parse->groupClause */
@@ -2509,7 +2509,7 @@ typedef struct LimitPath
typedef struct RestrictInfo
{
- pg_node_attr(no_read)
+ pg_node_attr(no_read, no_query_jumble)
NodeTag type;
@@ -2724,6 +2724,8 @@ typedef struct MergeScanSelCache
typedef struct PlaceHolderVar
{
+ pg_node_attr(no_query_jumble)
+
Expr xpr;
/* the represented expression */
@@ -2825,7 +2827,7 @@ typedef struct SpecialJoinInfo SpecialJoinInfo;
struct SpecialJoinInfo
{
- pg_node_attr(no_read)
+ pg_node_attr(no_read, no_query_jumble)
NodeTag type;
Relids min_lefthand; /* base+OJ relids in minimum LHS for join */
@@ -2853,7 +2855,7 @@ struct SpecialJoinInfo
*/
typedef struct OuterJoinClauseInfo
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
RestrictInfo *rinfo; /* a mergejoinable outer-join clause */
@@ -2892,6 +2894,8 @@ typedef struct OuterJoinClauseInfo
typedef struct AppendRelInfo
{
+ pg_node_attr(no_query_jumble)
+
NodeTag type;
/*
@@ -2967,7 +2971,7 @@ typedef struct AppendRelInfo
*/
typedef struct RowIdentityVarInfo
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -3005,7 +3009,7 @@ typedef struct RowIdentityVarInfo
typedef struct PlaceHolderInfo
{
- pg_node_attr(no_read)
+ pg_node_attr(no_read, no_query_jumble)
NodeTag type;
@@ -3038,7 +3042,7 @@ typedef struct PlaceHolderInfo
*/
typedef struct MinMaxAggInfo
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -3116,7 +3120,7 @@ typedef struct MinMaxAggInfo
*/
typedef struct PlannerParamItem
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -3296,7 +3300,7 @@ typedef struct JoinCostWorkspace
*/
typedef struct AggInfo
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
@@ -3330,7 +3334,7 @@ typedef struct AggInfo
*/
typedef struct AggTransInfo
{
- pg_node_attr(no_copy_equal, no_read)
+ pg_node_attr(no_copy_equal, no_read, no_query_jumble)
NodeTag type;
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 4781a9c632..659bd05c0c 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -46,7 +46,7 @@
*/
typedef struct PlannedStmt
{
- pg_node_attr(no_equal)
+ pg_node_attr(no_equal, no_query_jumble)
NodeTag type;
@@ -122,7 +122,7 @@ typedef struct PlannedStmt
*/
typedef struct Plan
{
- pg_node_attr(abstract, no_equal)
+ pg_node_attr(abstract, no_equal, no_query_jumble)
NodeTag type;
@@ -813,7 +813,7 @@ typedef struct NestLoop
typedef struct NestLoopParam
{
- pg_node_attr(no_equal)
+ pg_node_attr(no_equal, no_query_jumble)
NodeTag type;
int paramno; /* number of the PARAM_EXEC Param to set */
@@ -1377,7 +1377,7 @@ typedef enum RowMarkType
*/
typedef struct PlanRowMark
{
- pg_node_attr(no_equal)
+ pg_node_attr(no_equal, no_query_jumble)
NodeTag type;
Index rti; /* range table index of markable relation */
@@ -1425,7 +1425,7 @@ typedef struct PlanRowMark
*/
typedef struct PartitionPruneInfo
{
- pg_node_attr(no_equal)
+ pg_node_attr(no_equal, no_query_jumble)
NodeTag type;
Bitmapset *root_parent_relids;
@@ -1452,7 +1452,7 @@ typedef struct PartitionPruneInfo
*/
typedef struct PartitionedRelPruneInfo
{
- pg_node_attr(no_equal)
+ pg_node_attr(no_equal, no_query_jumble)
NodeTag type;
@@ -1495,7 +1495,7 @@ typedef struct PartitionedRelPruneInfo
*/
typedef struct PartitionPruneStep
{
- pg_node_attr(abstract, no_equal)
+ pg_node_attr(abstract, no_equal, no_query_jumble)
NodeTag type;
int step_id;
@@ -1570,7 +1570,7 @@ typedef struct PartitionPruneStepCombine
*/
typedef struct PlanInvalItem
{
- pg_node_attr(no_equal)
+ pg_node_attr(no_equal, no_query_jumble)
NodeTag type;
int cacheId; /* a syscache ID, see utils/syscache.h */
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 6d740be5c0..1be1642d92 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -982,6 +982,8 @@ typedef struct SubLink
*/
typedef struct SubPlan
{
+ pg_node_attr(no_query_jumble)
+
Expr xpr;
/* Fields copied from original SubLink: */
SubLinkType subLinkType; /* see above */
@@ -1029,6 +1031,8 @@ typedef struct SubPlan
*/
typedef struct AlternativeSubPlan
{
+ pg_node_attr(no_query_jumble)
+
Expr xpr;
List *subplans; /* SubPlan(s) with equivalent results */
} AlternativeSubPlan;
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 19ed29657c..2e57db915b 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -422,6 +422,8 @@ foreach my $infile (@ARGV)
if elem $supertype, @no_equal;
push @no_read, $in_struct
if elem $supertype, @no_read;
+ push @no_query_jumble, $in_struct
+ if elem $supertype, @no_query_jumble;
}
}
--
2.39.1
From 51bc070c098721fd91ae729408365a222c3d1959 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 10 Feb 2023 11:21:57 +0900
Subject: [PATCH v2 2/2] Switch compute_query_id = regress to mean "on" and
force it in pg_regress
This is just a tweak to force the tests to go through the query
jumbling.
---
src/include/nodes/queryjumble.h | 2 ++
src/backend/commands/explain.c | 2 +-
src/backend/nodes/queryjumblefuncs.c | 2 +-
src/test/regress/pg_regress.c | 3 ++-
doc/src/sgml/config.sgml | 2 +-
5 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/include/nodes/queryjumble.h b/src/include/nodes/queryjumble.h
index 204b8f74fd..3aa7d93255 100644
--- a/src/include/nodes/queryjumble.h
+++ b/src/include/nodes/queryjumble.h
@@ -80,6 +80,8 @@ IsQueryIdEnabled(void)
return false;
if (compute_query_id == COMPUTE_QUERY_ID_ON)
return true;
+ if (compute_query_id == COMPUTE_QUERY_ID_REGRESS)
+ return true;
return query_id_enabled;
}
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index fbbf28cf06..5aba713348 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -777,7 +777,7 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc)
ExplainPrintSettings(es);
/*
- * COMPUTE_QUERY_ID_REGRESS means COMPUTE_QUERY_ID_AUTO, but we don't show
+ * COMPUTE_QUERY_ID_REGRESS means COMPUTE_QUERY_ID_ON, but we don't show
* the queryid in any of the EXPLAIN plans to keep stable the results
* generated by regression test suites.
*/
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index d7fd72d70f..4d594106ec 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -257,7 +257,7 @@ _jumbleNode(JumbleState *jstate, Node *node)
default:
/* Only a warning, since we can stumble along anyway */
- elog(WARNING, "unrecognized node type: %d",
+ elog(ERROR, "unrecognized node type: %d",
(int) nodeTag(expr));
break;
}
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 6cd5998b9d..d3aafa156c 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1876,8 +1876,9 @@ create_database(const char *dbname)
"ALTER DATABASE \"%s\" SET lc_numeric TO 'C';"
"ALTER DATABASE \"%s\" SET lc_time TO 'C';"
"ALTER DATABASE \"%s\" SET bytea_output TO 'hex';"
+ "ALTER DATABASE \"%s\" SET compute_query_id TO 'regress';"
"ALTER DATABASE \"%s\" SET timezone_abbreviations TO 'Default';",
- dbname, dbname, dbname, dbname, dbname, dbname);
+ dbname, dbname, dbname, dbname, dbname, dbname, dbname);
psql_end_command(buf, "postgres");
/*
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8c56b134a8..56f8dac286 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8226,7 +8226,7 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
<literal>on</literal> (always enabled), <literal>auto</literal>,
which lets modules such as <xref linkend="pgstatstatements"/>
automatically enable it, and <literal>regress</literal> which
- has the same effect as <literal>auto</literal>, except that the
+ has the same effect as <literal>on</literal>, except that the
query identifier is not shown in the <literal>EXPLAIN</literal> output
in order to facilitate automated regression testing.
The default is <literal>auto</literal>.
--
2.39.1
signature.asc
Description: PGP signature
