Subject: Re: Refactor query normalization into core query jumbling

From: Sami Imseih <[email-masked]>
Date: Tue, 6 Jan 2026 14:20:47 -0600

> >> backwards compatible. Basically, we keep JumbleState a non-constant,
> >> but provide core APIs for any operation, such as
> >> generate_normalized_query,
> >> that needs to modify the state. So, my approach was not about enforcing a
> >> read-only JumbleState, but about providing the API to dissuade an author
> >> from modifying a JumbleState.
>
> > Given the lack of public APIs to modify JumbleState today, I don't see how
> > an extension would do
> > modifications in a meaningful way, short of copying the code. I think we
> > should be a bit bolder here in
> > enforcing a convention, either clearly making it read-only or dropping the
> > argument again.
>
> Based on the discussion so far I am leaning towards making JumbleState
> read-only as described here [0]. I don't see how we can drop JumbleState
> completely from hooks, since normalization needs to occur on-demand
> by the extension.

Here is v4.

0001 - addresses the comments made by Bertrand.
0002 - makes JumbleState a constant when passed to post_parse_analyze
and updates all downstream code that consume the JumbleState. This
means we now need to copy the locations from JState into a local/temporary
array when generating the normalized string.

--
Sami Imseih
Amazon Web Services (AWS)

---

From: Sami Imseih <[email-masked]>
Date: Mon, 16 Mar 2026 21:12:17 -0500

> array when generating the normalized string.

PFA a mandatory rebase.

---

From: Lukas Fittl <[email-masked]>
Date: Wed, 25 Mar 2026 18:34:02 -0700

Hi Sami,

In 0001:

> diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
> > index 87db8dc1a32..d4b26202c47 100644
> --- a/src/backend/nodes/queryjumblefuncs.c
> +++ b/src/backend/nodes/queryjumblefuncs.c
> ...
> @@ -773,3 +775,249 @@ _jumbleRangeTblEntry_eref(JumbleState *jstate,
> +
> +    /* we don't want to re-emit any escape string warnings */
> +    yyextra.escape_string_warning = false;
> +

I don't think this is needed anymore, as of
45762084545ec14dbbe66ace1d69d7e89f8978ac.

> +/*
> + * Callback to generate a normalized version of the query string that will be used to
> + * represent all similar queries.
> + *

I don't think the term "Callback" makes sense here - I think you could
just keep the original wording.

In 0002:

> diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
> index d4b26202c47..fe8f0ccd278 100644
> --- a/src/backend/nodes/queryjumblefuncs.c
> +++ b/src/backend/nodes/queryjumblefuncs.c
> ...
> @@ -813,14 +815,20 @@ SetConstantLengths(JumbleState *jstate, const char *query,
>     core_YYSTYPE yylval;
>     YYLTYPE        yylloc;
>
> +    if (jstate->clocations_count == 0)
> +        return NULL;
> +
> +    /* Copy constant locations to avoid modifying jstate */
> +    locs = palloc(jstate->clocations_count * sizeof(LocationLen));
> +    memcpy(locs, jstate->clocations, jstate->clocations_count * sizeof(LocationLen));
> +

You could use palloc_array for locs here.

> @@ -938,12 +948,13 @@ GenerateNormalizedQuery(JumbleState *jstate, const char *query,
>                 last_off = 0,    /* Offset from start for previous tok */
>                 last_tok_len = 0;    /* Length (in bytes) of that tok */
>     int            num_constants_replaced = 0;
> +    LocationLen *locs = NULL;
>
>     /*
>      * Set constants' lengths in JumbleState, as only locations are set during
>      * DoJumble(). Note this also ensures the items are sorted by location.
>      */
> -    SetConstantLengths(jstate, query, query_loc);
> +    locs = SetConstantLengths(jstate, query, query_loc);

I think we should update the comment here to reflect the fact that
we're no longer modifying JumbleState.

Otherwise these patches look good - it'd be nice to still get this
into 19 so we have less code duplication across the different
extensions working with normalized query text.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Sami Imseih <[email-masked]>
Date: Wed, 25 Mar 2026 22:31:28 -0500

Thanks for looking!

> 45762084545ec14dbbe66ace1d69d7e89f8978ac.

Correct. That showed up after my last rebase.

> just keep the original wording.

This was a remnant of my earlier experimentation. I removed.

A few notes on the comments:

- * Note that the normalized representation may well vary depending on
- * just which "equivalent" query is used to create the hashtable entry.
- * We assume this is OK.

This was in the original generate_normalize_query and since it mentions
hashtable entry, I moved the comment (in-spirit) to where pg_stat_statements
calls GenerateNormailzeQuery

* If query_loc > 0, then "query" has been advanced by that much compared to
* the original string start, as is the case with multi-statement strings, so
* we need to translate the provided locations to compensate. (This lets us
* avoid re-scanning statements before the one of interest, so it's worth doing.)
*
This comment was originally duplicated in both SetConstantLengths, so I
just kept it as-is in SetConstantLengths and added a shorter reference in
GenerateNormalizeQuery

Also, this comment "It is the caller's job to ensure that the string
is a valid SQL statement..."
made more sense in GenerateNormalizeQuery rather than SetConstantLengths, since
GenerateNormalizeQuery is the public function.

> You could use palloc_array for locs here.

done.

> we're no longer modifying JumbleState.

done.

> extensions working with normalized query text.

I agree!

v6 addresses the comments.

---

From: Lukas Fittl <[email-masked]>
Date: Thu, 26 Mar 2026 09:35:15 -0700

Hi Sami,

> v6 addresses the comments.

Great - I've tested that again and changes look good.

Two more thoughts:

1) I think "SetConstantLengths" should be renamed to
"GetConstantLengths", or "CalculateConstantLengths" in 0002, since
we're no longer modifying JumbleState

2) I wonder if we should export this function in 0002 - that would
specifically help pg_tracing, which also wants to extract inline
parameter values in addition to replacing them with a $n parameter
reference marker - I could also see that being useful for any other
extension that's interested in pulling out parameter values

I've also done some testing with two previously mentioned extensions,
pg_stat_monitor [0] and pg_tracing [1]. Both look like they can be
adapted to the new method with their regression tests succeeding.

Thanks,
Lukas

[0]: https://github.com/lfittl/pg_stat_monitor/commit/d3521de9f6736c1bb745bc31dae736540efe1781
[1]: https://github.com/lfittl/pg_tracing/commit/ecff95a87a1c60e8a5fe47662cc6a2148bd3632f

-- 
Lukas Fittl

---

From: Sami Imseih <[email-masked]>
Date: Thu, 26 Mar 2026 12:18:44 -0500

> we're no longer modifying JumbleState

Makes sense. I renamed it to ComputeConstantLengths; as this will reflect
that it's more than a getter function, and it's doing actual work. Compute
also seems more consistent with other function names we have out there.

> adapted to the new method with their regression tests succeeding.

That did cross my mind. I agree with this.

Both of those changes belong in 0002. See the attached v7.

---

From: Lukas Fittl <[email-masked]>
Date: Thu, 26 Mar 2026 18:50:20 -0700

> Both of those changes belong in 0002. See the attached v7.

Looks good!

I've also done a quick follow-up test with pg_tracing and that
simplifies its logic as expected [0] to be able to extract inline
parameter values.

Thanks,
Lukas

[0]: https://github.com/lfittl/pg_tracing/commit/ae937fdee4aa57206b31b5746f36dd8e55cf43d1#diff-41cf816684c420b8808fb4899e38a38aaf4f875613c4785e291010aa2b0ea267

-- 
Lukas Fittl

---

From: Michael Paquier <[email-masked]>
Date: Fri, 27 Mar 2026 14:17:57 +0900

> simplifies its logic as expected [0] to be able to extract inline

I have been looking at what you have here.  As mentioned upthread, I
am on board with aiming for JumbleState to be a const so as we enforce
as policy that no extension is allowed to touch what the core code has
computed.

However, I am not convinced by most of the patch.  The logic to
recompute the lengths of the constants is a concept proper to PGSS.
Perhaps we could reconsider moving that into core at some point, but
I am honestly not sure to see the range of benefits we'd gain from
that.

This line of arguments is stronger for the normalization of the query
string.  Why should the core code decide what a normalized string
should look like when it comes to the detection of the constants, if
any?  Instead of a dollar-quoted number, we could enforce a bunch of
things, like a '?' or a '$woozah$' at these locations.

Saying that, the key point of the patch is to take a copy of the
JumbleState locations, and recompute it for the needs of PGSS for the
sake of the normalized query representation.  Hence, why don't we just
do that at the end?  That should be enough to enforce the const marker
for the JumbleState across all the loaded extensions that want to look
at it.  This leads me to the simpler patch attached.

Comments or tomatoes?  That's simpler, and I'd be OK with just that
for v19.  That would be much better than the current state of affairs,
where PGSS decides to enforce its own ideas in the JumbleState, ideas 
fed to anything looping into the post-parse-analyze hook after PGSS.
--
Michael
From 336e6aa38d6dc3901c3fa1f746796a0b8fc87cd2 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Fri, 27 Mar 2026 14:13:00 +0900
Subject: [PATCH v8] Make JumbleState const in post_parse_analyze hook

Change the post_parse_analyze_hook_type signature to take a const
JumbleState parameter, preventing hooks from modifying the jumble
state during query analysis. This improves API safety by making it
clear that hooks should only read from the jumble state, not modify
it.

Update pg_stat_statements and related functions to match the new const
signature.  Refactor fill_in_constant_lengths() to return a newly
allocated LocationLen array instead of modifying the JumbleState, so as
PGSS does not touch the now-const JumbleState.  The routine is renamed
to compute_constant_lengths().

Discussion: https://postgr.es/m/[email-masked]
---
 src/include/parser/analyze.h                  |  2 +-
 .../pg_stat_statements/pg_stat_statements.c   | 75 ++++++++++++-------
 2 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.h
index e10270ff0ffd..b2db6fa4e8c4 100644
--- a/src/include/parser/analyze.h
+++ b/src/include/parser/analyze.h
@@ -21,7 +21,7 @@
 /* Hook for plugins to get control at end of parse analysis */
 typedef void (*post_parse_analyze_hook_type) (ParseState *pstate,
 											  Query *query,
-											  JumbleState *jstate);
+											  const JumbleState *jstate);
 extern PGDLLIMPORT post_parse_analyze_hook_type post_parse_analyze_hook;

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 6cb14824ec3b..0b6be5e255a4 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -335,7 +335,7 @@ static void pgss_shmem_request(void);
 static void pgss_shmem_startup(void);
 static void pgss_shmem_shutdown(int code, Datum arg);
 static void pgss_post_parse_analyze(ParseState *pstate, Query *query,
-									JumbleState *jstate);
+									const JumbleState *jstate);
 static PlannedStmt *pgss_planner(Query *parse,
 								 const char *query_string,
 								 int cursorOptions,
@@ -359,7 +359,7 @@ static void pgss_store(const char *query, int64 queryId,
 					   const BufferUsage *bufusage,
 					   const WalUsage *walusage,
 					   const struct JitInstrumentation *jitusage,
-					   JumbleState *jstate,
+					   const JumbleState *jstate,
 					   int parallel_workers_to_launch,
 					   int parallel_workers_launched,
 					   PlannedStmtOrigin planOrigin);
@@ -378,13 +378,14 @@ static char *qtext_fetch(Size query_offset, int query_len,
 static bool need_gc_qtexts(void);
 static void gc_qtexts(void);
 static TimestampTz entry_reset(Oid userid, Oid dbid, int64 queryid, bool minmax_only);
-static char *generate_normalized_query(JumbleState *jstate, const char *query,
+static char *generate_normalized_query(const JumbleState *jstate,
+									   const char *query,
 									   int query_loc, int *query_len_p);
-static void fill_in_constant_lengths(JumbleState *jstate, const char *query,
-									 int query_loc);
+static LocationLen *compute_constant_lengths(const JumbleState *jstate,
+											 const char *query,
+											 int query_loc);
 static int	comp_location(const void *a, const void *b);
 
-
 /*
  * Module load callback
  */
@@ -840,7 +841,7 @@ error:
  * Post-parse-analysis hook: mark query with a queryId
  */
 static void
-pgss_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate)
+pgss_post_parse_analyze(ParseState *pstate, Query *query, const JumbleState *jstate)
 {
 	if (prev_post_parse_analyze_hook)
 		prev_post_parse_analyze_hook(pstate, query, jstate);
@@ -1297,7 +1298,7 @@ pgss_store(const char *query, int64 queryId,
 		   const BufferUsage *bufusage,
 		   const WalUsage *walusage,
 		   const struct JitInstrumentation *jitusage,
-		   JumbleState *jstate,
+		   const JumbleState *jstate,
 		   int parallel_workers_to_launch,
 		   int parallel_workers_launched,
 		   PlannedStmtOrigin planOrigin)
@@ -2845,7 +2846,7 @@ release_lock:
  * Returns a palloc'd string.
  */
 static char *
-generate_normalized_query(JumbleState *jstate, const char *query,
+generate_normalized_query(const JumbleState *jstate, const char *query,
 						  int query_loc, int *query_len_p)
 {
 	char	   *norm_query;
@@ -2857,12 +2858,14 @@ generate_normalized_query(JumbleState *jstate, const char *query,
 				last_off = 0,	/* Offset from start for previous tok */
 				last_tok_len = 0;	/* Length (in bytes) of that tok */
 	int			num_constants_replaced = 0;
+	LocationLen *locs = NULL;
 
 	/*
-	 * Get constants' lengths (core system only gives us locations).  Note
-	 * this also ensures the items are sorted by location.
+	 * Determine constants' lengths (core system only gives us locations),
+	 * and return a sorted copy of jstate's LocationLen data with lengths
+	 * filled in.
 	 */
-	fill_in_constant_lengths(jstate, query, query_loc);
+	locs = compute_constant_lengths(jstate, query, query_loc);
 
 	/*
 	 * Allow for $n symbols to be longer than the constants they replace.
@@ -2888,15 +2891,15 @@ generate_normalized_query(JumbleState *jstate, const char *query,
 		 * the parameter in the next iteration (or after the loop is done),
 		 * which is a bit odd but seems to work okay in most cases.
 		 */
-		if (jstate->clocations[i].extern_param && !jstate->has_squashed_lists)
+		if (locs[i].extern_param && !jstate->has_squashed_lists)
 			continue;
 
-		off = jstate->clocations[i].location;
+		off = locs[i].location;
 
 		/* Adjust recorded location if we're dealing with partial string */
 		off -= query_loc;
 
-		tok_len = jstate->clocations[i].length;
+		tok_len = locs[i].length;
 
 		if (tok_len < 0)
 			continue;			/* ignore any duplicates */
@@ -2915,7 +2918,7 @@ generate_normalized_query(JumbleState *jstate, const char *query,
 		 */
 		n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d%s",
 							  num_constants_replaced + 1 + jstate->highest_extern_param_id,
-							  jstate->clocations[i].squashed ? " /*, ... */" : "");
+							  locs[i].squashed ? " /*, ... */" : "");
 		num_constants_replaced++;
 
 		/* move forward */
@@ -2924,6 +2927,10 @@ generate_normalized_query(JumbleState *jstate, const char *query,
 		last_tok_len = tok_len;
 	}
 
+	/* Clean up, if needed */
+	if (locs)
+		pfree(locs);
+
 	/*
 	 * We've copied up until the last ignorable constant.  Copy over the
 	 * remaining bytes of the original query string.
@@ -2942,8 +2949,9 @@ generate_normalized_query(JumbleState *jstate, const char *query,
 }
 
 /*
- * Given a valid SQL string and an array of constant-location records,
- * fill in the textual lengths of those constants.
+ * Given a valid SQL string and an array of constant-location records, return
+ * the textual lengths of those constants in a newly allocated LocationLen
+ * array, or NULL if there are no constants.
  *
  * The constants may use any allowed constant syntax, such as float literals,
  * bit-strings, single-quoted strings and dollar-quoted strings.  This is
@@ -2959,16 +2967,19 @@ generate_normalized_query(JumbleState *jstate, const char *query,
  * past the first to -1 so that they can later be ignored.
  *
  * If query_loc > 0, then "query" has been advanced by that much compared to
- * the original string start, so we need to translate the provided locations
- * to compensate.  (This lets us avoid re-scanning statements before the one
- * of interest, so it's worth doing.)
+ * the original string start, as is the case with multi-statement strings, so
+ * we need to translate the provided locations to compensate.  (This lets us
+ * avoid re-scanning statements before the one of interest, so it's worth
+ * doing.)
  *
  * N.B. There is an assumption that a '-' character at a Const location begins
  * a negative numeric constant.  This precludes there ever being another
  * reason for a constant to start with a '-'.
+ *
+ * It is the caller's responsibility to free the result, if necessary.
  */
-static void
-fill_in_constant_lengths(JumbleState *jstate, const char *query,
+static LocationLen *
+compute_constant_lengths(const JumbleState *jstate, const char *query,
 						 int query_loc)
 {
 	LocationLen *locs;
@@ -2977,14 +2988,20 @@ fill_in_constant_lengths(JumbleState *jstate, const char *query,
 	core_YYSTYPE yylval;
 	YYLTYPE		yylloc;
 
+	if (jstate->clocations_count == 0)
+		return NULL;
+
+	/* Copy constant locations to avoid modifying jstate */
+	locs = palloc_array(LocationLen, jstate->clocations_count);
+	memcpy(locs, jstate->clocations, jstate->clocations_count * sizeof(LocationLen));
+
 	/*
 	 * Sort the records by location so that we can process them in order while
 	 * scanning the query text.
 	 */
 	if (jstate->clocations_count > 1)
-		qsort(jstate->clocations, jstate->clocations_count,
+		qsort(locs, jstate->clocations_count,
 			  sizeof(LocationLen), comp_location);
-	locs = jstate->clocations;
 
 	/* initialize the flex scanner --- should match raw_parser() */
 	yyscanner = scanner_init(query,
@@ -3008,7 +3025,11 @@ fill_in_constant_lengths(JumbleState *jstate, const char *query,
 		if (locs[i].squashed)
 			continue;			/* squashable list, ignore */
 
-		/* Adjust recorded location if we're dealing with partial string */
+		/*
+		 * Adjust the constant's location using the provided starting location
+		 * of the current statement.  This allows us to avoid scanning a
+		 * multi-statement string from the beginning.
+		 */
 		loc = locs[i].location - query_loc;
 		Assert(loc >= 0);
 
@@ -3064,6 +3085,8 @@ fill_in_constant_lengths(JumbleState *jstate, const char *query,
 	}
 
 	scanner_finish(yyscanner);
+
+	return locs;
 }
 
 /*
-- 
2.53.0

---

From: Lukas Fittl <[email-masked]>
Date: Fri, 27 Mar 2026 00:19:01 -0700

> things, like a '?' or a '$woozah$' at these locations.

Fair enough, though I haven't seen any extensions that do that in
practice - its reasonable to have normalization result in a query
string that's parsable again and can be passed to EXPLAIN
(GENERIC_PLAN).

> fed to anything looping into the post-parse-analyze hook after PGSS.

I'm not convinced that making the const change alone is a good idea,
without also providing some helpers to reduce the repeated code in
extensions.

What if we only put the ComputeConstantLengths (as Sami had it in v7)
in core, together with making JumbleState const?

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Sami Imseih <[email-masked]>
Date: Fri, 27 Mar 2026 12:09:04 -0500

> string that's parsable again and can be passed to EXPLAIN

with regards to generate_normalized_query, AFAICT, the most common
case is extensions are using it for is dollar quoted number, but I agree
this one is a gray area.

> in core, together with making JumbleState const?

I agree that ComputeConstantLengths should be in core. This one is
not a gray area IMO. The query jumble already records constant locations,
but leaves the lengths unset. ComputeConstantLengths is just the
completion of that  work. There could be no other interpretation,
unlike generate_normalized_query, of what the lengths should be.

---

From: Michael Paquier <[email-masked]>
Date: Sat, 28 Mar 2026 10:42:09 +0900

> On Mar 28, 2026, at 2:09, Sami Imseih <[email-masked]> wrote:

This is an interesting remark. One problem with any patches presented yet is that we don’t attach the lengths as part of the in-core query jumbling procedure: we plug the lengths later using the same structure as the query jumbling. It seems to me that this is half-baked overall. I think that we don’t want to pay the extra length computation in the core query jumbling at the end, then why does it make sense to include the lengths in the JumbleState structure at all? Shouldn’t we use a different structure filled inside PGSS for this purpose rather than reuse the same thing for PGSS and the in-core part (don’t have the code in front of me now).
--
Michael

---

From: Lukas Fittl <[email-masked]>
Date: Sun, 29 Mar 2026 22:21:46 -0700

> This is an interesting remark. One problem with any patches presented yet is that we don’t attach the lengths as part of the in-core query jumbling procedure: we plug the lengths later using the sa…

I see where you're coming from on that, but I don't think we can
remove anything here in practice:

typedef struct LocationLen
{
    int            location; /* Required */
    int            length; /* Set by _jumbleElements */
    bool        squashed; /* Set by RecordConstLocation called from
_jumbleElements */
    bool        extern_param; /* Set by RecordConstLocation called
from  _jumbleParam */
} LocationLen;

typedef struct JumbleState
{
    unsigned char *jumble; /* Required */
    Size        jumble_len; /* Required */
    LocationLen *clocations; /* Required to track constant locations */
    int            clocations_buf_size; /* Required to track constant
locations */
    int            clocations_count; /* Required to track constant locations */
    int            highest_extern_param_id; /* Set by _jumbleParam */
    bool        has_squashed_lists; /* Set by _jumbleElements */
    unsigned int pending_nulls; /* Required */
    Size        total_jumble_len; /* Required */
} JumbleState;

The only refactoring I could think of is to write out the
_jumbleElements information separately, then we could actually drop
length, and maybe squashed, from LocationLen. But I'm not sure that
really buys us much? It would be more clear I guess, because the mixed
locations of where length gets set is indeed a bit odd.

I still think it'd be reasonable for us to include
ComputeConstantLengths in core to complete the picture of what we're
doing with _jumbleElements and the length field already anyway. Its
basically a way to fully hydrate the partially filled out JumbleState
from the initial jumble.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Sami Imseih <[email-masked]>
Date: Mon, 30 Mar 2026 14:20:47 -0500

> remove anything here in practice:

Yes. Not unless we want to rely on the parser to track the lengths in
Const, which could be invasive, but I have not looked into it.

Paying the length computation at the end of every jumble is also
going to impact performance, so that will not be a good idea.

> basically a way to fully hydrate the partially filled out JumbleState

I fully agree, ComputeConstantLengths is an optional post-jumble-query step
for a consumer that wishes to calculate the lengths. The length calculation
is not unique to a plug-in, so in my mind the work it's doing is core
jumbling functionality.

---

From: Michael Paquier <[email-masked]>
Date: Tue, 31 Mar 2026 08:52:46 +0900

> Const, which could be invasive, but I have not looked into it.

Hmm.  We may not want to get down to that, still that would be
cheaper than reprocessing the parsing of the query string twice. 

> is not unique to a plug-in, so in my mind the work it's doing is core

Okay.  I could fall into that for this release.  Marking the
JumbleState as a const is the most important piece here.  I'm +-0
regarding this routine, but I can also see your point about how it's
useful to give at least the option to extensions to have a
recomputation of the Const lengths, the same way as PGSS.  What are
the extensions that would use that?
--
Michael