From: | Greg Sabino Mullane <htamfids(at)gmail(dot)com> |
---|---|
To: | Sami Imseih <samimseih(at)gmail(dot)com> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: track generic and custom plans in pg_stat_statements |
Date: | 2025-03-05 20:29:45 |
Message-ID: | CAKAnmmLhwV+dpstH88X7fbm2X77X9COEhMWsY-NQChVV0=0TTQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Overall I like the idea; adds some nice visibility to something that has
been very ephemeral in the past.
Not included in this patch and maybe for follow-up work, is this
> information a good idea also?
can be added to EXPLAIN output and perhaps pg_stat_database.
>
I could see EXPLAIN being somewhat useful (especially for non-interactive
things like auto_explain), so a weak +1 on that.
Definitely not useful for pg_stat_database IMHO.
Some quick comments on the patch:
FILE: contrib/pg_stat_statements/expected/plan_cache.out
These tests seem very verbose. Why not just prepare a simple query:
prepare foo as select $1 > 0;
execute foo(1);
...then tweak things via plan_cache_mode to ensure we test the right things?
Also would be nice to constrain the pg_stat_statement SELECTs with some
careful WHERE clauses. Would also allow you to remove the ORDER BY and the
COLLATE.
FILE: contrib/pg_stat_statements/meson.build
oldextversions should still have a trailing comma
FILE: contrib/pg_stat_statements/pg_stat_statements.c
+ if (cplan && cplan->status > PLAN_CACHE_STATUS_CUSTOM_PLAN)
Not really comfortable with using the enum like this. Better would be
explicitly listing the two states that lead to the increment. Not as good
but still better:
cplan->status >= PLAN_CACHE_STATUS_GENERIC_PLAN_BUILD
+ PlanCacheStatus status; /* is it a reused generic plan? */
The comment should be updated
FILE: contrib/pg_stat_statements/sql/plan_cache.sql
Missing a newline at the end
FILE: doc/src/sgml/pgstatstatements.sgml
+ Total number of non-utility statements executed using a generic plan
I'm not sure we need to specify non-utility here.
Cheers,
Greg
--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2025-03-05 20:33:14 | Re: Remove curl installation from CI images |
Previous Message | Daniel Gustafsson | 2025-03-05 20:27:45 | Remove curl installation from CI images |