David Rowley <[email protected]> writes:
> On Wed, 10 Jun 2026 at 04:54, Tom Lane <[email protected]> wrote:
>> +1. There are also other comments about query jumbling in nodes/*.h that
>> seem pretty information-free now. They might have been helpful before
>> we invented query_jumble_ignore and related annotations, but now they
>> seem just duplicative.
> Here's a patch for that. I did leave a few comments which mention a
> reason why a particular field is ignored. That seems like it could be
> useful. I think I've got all the ones that just talk about what's
> included or ignored.
All these changes look good, but I have a few more suggestions,
attached as a delta on top of yours. Notably
* - query_jumble_location: Mark the field as a location to track. This is
- * only allowed for integer fields that include "location" in their name.
+ * only used for fields of type ParseLoc, which otherwise are not jumbled.
If you look at how gen_node_support.pl implements that annotation,
my revised statement is correct about the field type, and I don't
see anything that actually constrains the field name to be "location".
Maybe some earlier implementation behaved that way?
regards, tom lane
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 91377a6cde3..a1644322908 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1427,8 +1427,6 @@ typedef struct RTEPermissionInfo
* time. We do however remember how many columns we thought the type had
* (including dropped columns!), so that we can successfully ignore any
* columns added after the query was parsed.
- *
- * The query jumbling only needs to track the function expression.
*/
typedef struct RangeTblFunction
{
@@ -1647,9 +1645,6 @@ typedef struct GroupingSet
* When refname isn't null, the partitionClause is always copied from there;
* the orderClause might or might not be copied (see copiedOrder); the framing
* options are never copied, per spec.
- *
- * The information relevant for the query jumbling is the partition clause
- * type and its bounds.
*/
typedef struct WindowClause
{
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 7977ee24783..bb05aeebee4 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -319,7 +319,10 @@ typedef struct Var
* references). This ensures that the Const node is self-contained and makes
* it more likely that equal() will see logically identical values as equal.
*
- * Only the constant type OID is relevant for the query jumbling.
+ * For query jumble, we don't want different const values changing the jumble
+ * result. We only jumble consttype as different const types could result in
+ * very different plans and execution times, which is useful to distinguish in
+ * extensions such as pg_stat_statements.
*/
typedef struct Const
{
@@ -346,10 +349,7 @@ typedef struct Const
*/
bool constbyval pg_node_attr(query_jumble_ignore);
- /*
- * token location, or -1 if unknown. All constants are tracked as
- * locations in query jumbling, to be marked as parameters.
- */
+ /* token location, or -1 if unknown. */
ParseLoc location pg_node_attr(query_jumble_location);
} Const;
@@ -452,9 +452,6 @@ typedef struct Param
* and can share the result. Aggregates with same 'transno' but different
* 'aggno' can share the same transition state, only the final function needs
* to be called separately.
- *
- * Information related to collations, transition types and internal states
- * are irrelevant for the query jumbling.
*/
typedef struct Aggref
{
@@ -550,9 +547,6 @@ typedef struct Aggref
*
* In raw parse output we have only the args list; parse analysis fills in the
* refs list, and the planner fills in the cols list.
- *
- * All the fields used as information for an internal state are irrelevant
- * for the query jumbling.
*/
typedef struct GroupingFunc
{
@@ -574,13 +568,6 @@ typedef struct GroupingFunc
ParseLoc location;
} GroupingFunc;
-/*
- * WindowFunc
- *
- * Collation information is irrelevant for the query jumbling, as is the
- * internal state information of the node like "winstar" and "winagg".
- */
-
/*
* Null Treatment options. If specified, initially set to PARSER_IGNORE_NULLS
* which is then converted to IGNORE_NULLS if the window function allows the
@@ -591,6 +578,11 @@ typedef struct GroupingFunc
#define PARSER_RESPECT_NULLS 2
#define IGNORE_NULLS 3
+ /*
+ * WindowFunc
+ *
+ * Node type to represent a call to a window function.
+ */
typedef struct WindowFunc
{
Expr xpr;
@@ -703,8 +695,6 @@ typedef struct MergeSupportFunc
* subscripting logic. Likewise, reftypmod and refcollid will match the
* container's properties in a store, but could be different in a fetch.
*
- * Any internal state data is ignored for the query jumbling.
- *
* Note: for the cases where a container is returned, if refexpr yields a R/W
* expanded container, then the implementation is allowed to modify that
* object in-place and return the same object.
@@ -772,9 +762,6 @@ typedef enum CoercionForm
/*
* FuncExpr - expression node for a function call
- *
- * Collation information is irrelevant for the query jumbling, only the
- * arguments and the function OID matter.
*/
typedef struct FuncExpr
{
@@ -839,9 +826,6 @@ typedef struct NamedArgExpr
* of the node. The planner makes sure it is valid before passing the node
* tree to the executor, but during parsing/planning opfuncid can be 0.
* Therefore, equal() will accept a zero value as being equal to other values.
- *
- * Internal state information and collation data is irrelevant for the query
- * jumbling.
*/
typedef struct OpExpr
{
@@ -919,9 +903,6 @@ typedef OpExpr NullIfExpr;
* Similar to OpExpr, opfuncid, hashfuncid, and negfuncid are not necessarily
* filled in right away, so will be ignored for equality if they are not set
* yet.
- *
- * OID entries of the internal function types are irrelevant for the query
- * jumbling, but the operator OID and the arguments are.
*/
typedef struct ScalarArrayOpExpr
{
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index a2925ae4946..372eee20680 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -105,14 +105,12 @@ typedef enum NodeTag
* - custom_query_jumble: Has custom implementation in queryjumblefuncs.c
* for the field of a node. Also available as a node attribute.
*
- * - query_jumble_ignore: Ignore the field for the query jumbling. Note
- * that typmod and collation information are usually irrelevant for the
- * query jumbling.
+ * - query_jumble_ignore: Ignore the field for query jumbling.
*
* - query_jumble_squash: Squash multiple values during query jumbling.
*
* - query_jumble_location: Mark the field as a location to track. This is
- * only allowed for integer fields that include "location" in their name.
+ * only used for fields of type ParseLoc, which otherwise are not jumbled.
*
* - read_as(VALUE): In nodeRead(), replace the field's value with VALUE.
*
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index a1644322908..4133c404a6b 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -109,10 +109,13 @@ typedef uint64 AclMode; /* a bitmask of privilege bits */
* Planning converts a Query tree into a Plan tree headed by a PlannedStmt
* node --- the Query structure is not used by the executor.
*
- * All the fields ignored for the query jumbling are not semantically
- * significant (such as alias names), as is ignored anything that can
- * be deduced from child nodes (else we'd just be double-hashing that
- * piece of information).
+ * We ignore fields for query jumbling if they are not semantically
+ * significant (such as alias names). We also ignore anything that can
+ * be deduced from other fields or child nodes, else we'd just be
+ * double-hashing that piece of information. In some places query jumbling
+ * deliberately ignores fields that are semantically significant, such as
+ * Const values, because we have made a policy decision to combine queries
+ * that differ only in those respects.
*/
typedef struct Query
{
@@ -125,8 +128,8 @@ typedef struct Query
/*
* query identifier (can be set by plugins); ignored for equal, as it
- * might not be set; also not stored. This is the result of the query
- * jumble, hence ignored.
+ * might not be set; also not stored. This is the output of query
+ * jumbling, hence it must be ignored as an input.
*
* We store this as a signed value as this is the form it's displayed to
* users in places such as EXPLAIN and pg_stat_statements. Primarily this
@@ -142,8 +145,7 @@ typedef struct Query
/*
* rtable index of target relation for INSERT/UPDATE/DELETE/MERGE; 0 for
- * SELECT. This is ignored in the query jumble as unrelated to the
- * compilation of the query ID.
+ * SELECT.
*/
int resultRelation pg_node_attr(query_jumble_ignore);
@@ -1818,7 +1820,7 @@ typedef struct CommonTableExpr
/*
* Number of RTEs referencing this CTE (excluding internal
- * self-references), irrelevant for query jumbling.
+ * self-references).
*/
int cterefcount pg_node_attr(query_jumble_ignore);
/* list of output column names */
@@ -2359,7 +2361,7 @@ typedef struct SetOperationStmt
Node *rarg; /* right child */
/* Eventually add fields for CORRESPONDING spec here */
- /* Fields derived during parse analysis (irrelevant for query jumbling): */
+ /* Fields derived during parse analysis: */
/* OID list of output column type OIDs */
List *colTypes pg_node_attr(query_jumble_ignore);
/* integer list of output column typmods */
@@ -3735,8 +3737,6 @@ typedef struct InlineCodeBlock
* list contains copies of the expressions for all output arguments, in the
* order of the procedure's declared arguments. (outargs is never evaluated,
* but is useful to the caller as a reference for what to assign to.)
- * The transformed call state is not relevant in the query jumbling, only the
- * function call is.
* ----------------------
*/
typedef struct CallStmt
