Subject: Re: Cleaning up PREPARE query strings?

From: Julien Rouhaud <[email-masked]>
Date: Sun, 11 Jan 2026 15:50:19 +0800

Hi,

On Wed, Dec 24, 2025 at 11:21:00AM -0500, Tom Lane wrote:
> Julien Rouhaud <[email-masked]> writes:
> > I'm attaching a POC patch to fix that behavior by teaching PREPARE to clean the
> > passed query text the same way as pg_stat_statements.
>
> This patch invalidates all the location fields in the parsed query:
> they could not be used to generate sane error cursors referencing the
> truncated string.  I'm not sure how many places try to generate such
> errors post-parsing, but it's more than zero, and I've long had
> ambitions of trying to extend that substantially (e.g, allowing
> execution-time errors from functions to point at the relevant function
> call).

Ah I wasn't aware of that.  After a quick look I'm assuming it's all the
callers of executor_errposition()?

There aren't a lot of them, and I've unfortunately been unable to hit any of
them.  Every scenario I tried was always caught during planning time.  Do you
have any example on how to exercise them?

> Certainly the patch could be extended to update all those fields,
> but that increases its complexity very significantly.  I doubt
> that it's worth it.  My reaction to your example is more like
> "if that bothers you, don't do it that way".

I agree that updating the location fields in the whole parsetree is a non
starter, butt I don't think that it's the only option.

If that's indeed the cases going through executor_errposition(), they rely on
having the executor state at hand to retrieve the query string.   We could
simply have an extra "query string offset" field stored there that would always
be 0 except when PREPARE changes the input query string and then do the
location to character number calculation after applying that offset. And that
offset should already be returned by CleanQuerytext() so it wouldn't have extra
complication.

---

From: Sami Imseih <[email-masked]>
Date: Tue, 13 Jan 2026 14:24:51 -0600

I was looking a bit more here, and I found that this patch breaks if
pg_stat_statements is enabled.

```
postgres=# SELECT 'bingo'\;  PREPARE q1 AS SELECT 1 AS a \; SELECT 42;
server closed the connection unexpectedly
    This probably means the server terminated abnormally
    before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
!?>
```

This is because the raw statement location is set to 0 with the patch which
breaks generate_nromalized_query when trying to deal with a multi command
statement. So, I don't think the way v1 is doing things will actually work.

I am thinking that storing the statement length and location in the entry
of prepared_queries may be the better option. Having that info, the
cleaned up query text can be generated on the fly whenever
pg_prepared_statement is called.

We may not need to call CleanQuerytext whenever, however it does
have the benefit of removing leading and trailing whitespace and the
terminating semicolon.

What do you think?

--
Sami Imseih
Amazon Web Services (AWS)

---

From: Julien Rouhaud <[email-masked]>
Date: Wed, 14 Jan 2026 06:50:59 +0800

Hi,

> statement. So, I don't think the way v1 is doing things will actually work.

That's very strange.  I'm pretty sure that I tried since the earlier approach I
mentioned using doing it in CreateCachedPlan() had that exact problem.

Also, src/test/recovery/t/027_stream_regress.pl does run the full regression
tests with pg_stat_statements enabled, and it doesn't fail.

I'm not sure what is different in your case.

---

From: Sami Imseih <[email-masked]>
Date: Wed, 14 Jan 2026 09:56:21 -0600

> I'm not sure what is different in your case.

the regression tests succeed because by the time the multi-statement command
is executed, the queries involved have already been tracked by
pg_stat_statements,
so they don't need to get stored again, and no reason to go through
generate_normalize_query. In this case, SELECT $1 is already tracked.

To repro the crash, just reset pg_stat_statements prior.

```
diff --git a/src/test/regress/sql/prepare.sql b/src/test/regress/sql/prepare.sql
index 0e7fe44725e..51421357f26 100644
--- a/src/test/regress/sql/prepare.sql
+++ b/src/test/regress/sql/prepare.sql
@@ -4,6 +4,9 @@

 SELECT name, statement, parameter_types, result_types FROM
pg_prepared_statements;

+CREATE EXTENSION pg_stat_statements;
+SELECT pg_stat_statements_reset();
+
 SELECT 'bingo'\;  PREPARE q1 AS SELECT 1 AS a \; SELECT 42;
 EXECUTE q1;
```

For this test, we should either reset statements before or construct a
statement for the test that has not been tracked. We can maybe
do that with a query against a newly created table in prepare.sql.
I like the latter more.

---

From: Julien Rouhaud <[email-masked]>
Date: Thu, 15 Jan 2026 12:41:29 +0800

Hi,

> generate_normalize_query. In this case, SELECT $1 is already tracked.

I see.  I'm not sure why my first prototype used to crash with that exact same
problem in 027_stream_regress but not this one, but anyway.

I only had a quick look at the code but unless I'm missing something it's
mostly an oversight as I pass "new_query" to CreateCachedPlan() but still pass
the original query string to pg_analyze_and_rewrite_varparams().  Using
"new_query" there too should fix the problem?

In an earlier message you wrote

> pg_prepared_statement is called.

I don't like this approach as it has more execution time overhead, but also
doesn't fix at all the memory waste in the prepared statements cache.

> +++ b/src/test/regress/sql/prepare.sql

FWIW I think that this should belong to pg_stat_statements testing, no the main
regression tests.  This would also ensure that we see consistent results in
some other scenarios.

---

From: Sami Imseih <[email-masked]>
Date: Thu, 15 Jan 2026 13:13:36 -0600

> "new_query" there too should fix the problem?

That will be a fix, but that seems way too invasive for me. Not sure if it may
break other things.

> doesn't fix at all the memory waste in the prepared statements cache.

ISTM that your proposal will actually use more memory because
pstate->p_sourcetext does not get free'd, but we must now allocate
space for a new "cleaned" query.

As far as execution overhead, this will only be paid when you query
pg_prepared_statement, and we can even optimize this a bit by only
cleaning once and reusing the results.

> regression tests.  This would also ensure that we see consistent results in

I agree.