| From: | Julien Rouhaud <rjuju123(at)gmail(dot)com> |
|---|---|
| To: | Sami Imseih <samimseih(at)gmail(dot)com> |
| Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: Cleaning up PREPARE query strings? |
| Date: | 2026-01-15 04:41:29 |
| Message-ID: | aWhv-ZXEJ3kd-dWa@jrouhaud |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
On Wed, Jan 14, 2026 at 09:56:21AM -0600, Sami Imseih wrote:
> > 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.
>
> 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.
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
> 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.
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.
> 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.
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 | Date | Subject | |
|---|---|---|---|
| Next Message | Tatsuro Yamada | 2026-01-15 04:43:50 | Re: [PATCH] psql: add \dcs to list all constraints |
| Previous Message | jian he | 2026-01-15 04:31:07 | Re: support ALTER COLUMN SET EXPRESSION over virtual generated column with check constraint |