Horton

Thread

Commits

  1. doc: Update header file mention for CompareType

  2. Allow plugins to set a 64-bit plan identifier in PlannedStmt

  3. Fix indentation of comment in plannodes.h

  4. Reformat node comments in plannodes.h

  5. injection_points: Add routine able to drop all stats

  6. Add pgstat_drop_matching_entries() to pgstats

  7. Move pg_stat_statements query jumbling to core.

  1. [PATCH] Optionally record Plan IDs to track plan changes for a query

    Lukas Fittl <lukas@fittl.com> — 2025-01-02T20:46:04Z

    Hi all,

Inspired by a prior proposal by Sami Imseih for tracking Plan IDs [0], as
well as extensions like pg_stat_plans [1] (unmaintained), pg_store_plans
[2] (not usable on production, see notes later) and aurora_stat_plans [3]
(enabled by default on AWS), this proposed patch set adds:

1. An updated in-core facility to optionally track Plan IDs based on
hashing the plan nodes during the existing treewalk in setrefs.c -
controlled by the new "compute_plan_id" GUC
2. An example user of plan IDs with a new pg_stat_plans extension in
contrib, that also records the first plan text with EXPLAIN (COSTS OFF)

My overall perspective is that (1) is best done in-core to keep overhead
low, whilst (2) could be done outside of core (or merged with a future
pg_stat_statements) and is included here mainly for illustration purposes.

Notes including what constitutes a plan ID follow, after a quick example:

## Example

Having the planid + an extension that records it, plus the first plan text,
lets you track different plans for the same query:

bench=# SELECT * FROM pgbench_accounts WHERE aid = 123;
bench=# SET enable_indexscan = off;
bench=# SELECT * FROM pgbench_accounts WHERE aid = 123;
bench=# SELECT queryid, planid, plan FROM pg_stat_plans WHERE plan LIKE
'%pgbench%';
       queryid        |        planid        |
 plan
----------------------+----------------------+------------------------------------------------------------
 -5986989572677096226 | -2057350818695327558 | Index Scan using
pgbench_accounts_pkey on pgbench_accounts+
                      |                      |   Index Cond: (aid = 123)
 -5986989572677096226 |  2815444815385882663 | Bitmap Heap Scan on
pgbench_accounts                      +
                      |                      |   Recheck Cond: (aid = 123)
                              +
                      |                      |   ->  Bitmap Index Scan on
pgbench_accounts_pkey          +
                      |                      |         Index Cond: (aid =
123)

And this also supports showing the plan for a currently running query (call
count is zero in such cases):

session 1:
bench# SELECT pg_sleep(100), COUNT(*) FROM pgbench_accounts;

session 2:
bench=# SELECT query, plan FROM pg_stat_activity
  JOIN pg_stat_plans ON (usesysid = userid AND datid = dbid AND query_id =
queryid AND plan_id = planid)
  WHERE query LIKE 'SELECT pg_sleep%';
                         query                         |
 plan
-------------------------------------------------------+------------------------------------
 SELECT pg_sleep(100), COUNT(*) FROM pgbench_accounts; | Aggregate
                +
                                                       |   ->  Seq Scan on
pgbench_accounts

## What is a plan ID?

My overall hypothesis here is that identifying different plan shapes for
the same normalized query (i.e. queryid) is useful, because it lets you
detect use of different plan choices such as which join order or index was
used based on different input parameters (or different column statistics
due to a recent ANALYZE) for the same normalized query.

You can get this individually for a given query with EXPLAIN of course, but
if you want to track this over time the only workable mechanism in my
experience is auto_explain, which is good for sampling outliers, but bad
for getting a comprehensive view of which plans where used and how often.

To me the closest to what I consider a "plan shape" is the output of
EXPLAIN (COSTS OFF), that is, the plan nodes and their filters/conditions,
but discarding the exact costs as well as ignoring any execution
statistics. The idea behind the proposed plan ID implementation is trying
to match that by hashing plan nodes, similar to how query IDs hash
post-parse analysis query nodes.

One notable edge case are plans that involve partitions - those could of
course lead to a lot of different planids for a given queryid, based on how
many partitions were pruned. We could consider special casing this, e.g. by
trying to be smart about declarative partitioning, and considering plans to
be identical if they scan the same number of partitions with the same scan
methods. However this could also be done by an out-of-core extension,
either by defining a better planid mechanism, or maintaining a grouped
planid of sorts based on the internal planid.

The partitions problem reminds me a bit of the IN list problem with
pg_stat_statements (which we still haven't resolved) - despite the problem
the extension has been successfully used for many years by many Postgres
users, even for those workloads where you have thousands of entries for the
same query with different IN list lengths.

## Why does this need to be in core?

Unfortunately both existing open-source extensions I'm familiar with are
not suitable for production use. Out of the two, only pg_store_plans [2] is
being maintained, however it carries significant overhead because it
calculates the plan ID by hashing the EXPLAIN text output every time a
query is executed.

My colleague Marko (CCed) and I evaluated whether pg_store_plans could be
modified to instead calculate the planid by hashing the plan tree, and ran
into three issues:

1. The existing node jumbling in core is not usable by extensions, and it
is necessary to have something like it for hashing Filters/Conds
(ultimately requiring us to duplicate all of it in the extension, and keep
maintaining that for every major release)
2. Whilst its cheap enough, it seems unnecessary to do an additional tree
walk when setrefs.c already walks the plan tree in a near-final state
3. It seems useful to enable showing the plan shape of a currently running
query (e.g. to identify whether a plan regression causes the query to run
forever), and this is much easier to do by adding planid to
pg_stat_activity, like the queryid

I also suspect that Aurora's implementation in [3] had some in-core
modifications to enable it work efficiently, but I'm not familiar with any
implementation details beyond what's in the public documentation.

## Implementation notes

The attached patch set includes two preparatory patches that could be
committed independently if deemed useful:

The first patch allows use of node jumbling by other unit files /
extensions, which would help an out-of-core extension avoid duplicating all
the node jumbling code.

The second patch adds a function for the extensible cumulative statistics
system to drop all entries for a given statistics kind. This already exists
for resetting, but in case of a dynamic list of entries its more useful to
be able to drop all of them when "reset" is called.

The third patch adds plan ID tracking in core. This is turned off by
default, and can be enabled by setting "compute_plan_id" to "on". Plan IDs
are shown in pg_stat_activity, as well as EXPLAIN and auto_explain output,
to allow matching a given plan ID to a plan text, without requiring the use
of an extension. There are some minor TODOs in the plan jumbling logic that
I haven't finalized yet. There is also an open question whether we should
use the node attribute mechanism instead of custom jumbling logic?

The fourth patch adds the pg_stat_plans contrib extension, for illustrative
purposes. This is inspired by pg_stat_statements, but intentionally kept
separate for easier review and since it does not use an external file and
could technically be used independently. We may want to develop this into a
unified pg_stat_statements+plans in-core mechanism in the future, but I
think that is best kept for a separate discussion.

The pg_stat_plans extension utilizes the cumulative statistics system for
tracking statistics (extensible thanks to recent changes!), as well as
dynamic shared memory to track plan texts up to a given limit (2kB by
default). As a side note, managing extra allocations with the new
extensible stats is a bit cumbersome - it would be helpful to have a hook
for cleaning up data associated to entries (like a DSA allocation).

Thanks,
Lukas

[0]:
https://www.postgresql.org/message-id/flat/604E3199-2DD2-47DD-AC47-774A6F97DCA9%40amazon.com
[1]: https://github.com/2ndQuadrant/pg_stat_plans
[2]: https://ossc-db.github.io/pg_store_plans/
[3]:
https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/aurora_stat_plans.html

-- 
Lukas Fittl

  2. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Jeremy Schneider <schneider@ardentperf.com> — 2025-01-07T06:35:25Z

    On Thu, 2 Jan 2025 12:46:04 -0800
Lukas Fittl <lukas@fittl.com> wrote:

> this proposed patch set adds:
> 
> 1. An updated in-core facility to optionally track Plan IDs based on
> hashing the plan nodes during the existing treewalk in setrefs.c -
> controlled by the new "compute_plan_id" GUC
> 2. An example user of plan IDs with a new pg_stat_plans extension in
> contrib, that also records the first plan text with EXPLAIN (COSTS
> OFF)
> 
> My overall perspective is that (1) is best done in-core to keep
> overhead low, whilst (2) could be done outside of core (or merged
> with a future pg_stat_statements) and is included here mainly for
> illustration purposes.

And 2025 is starting with a bang! Nice to see this email! Being able to
collect telemetry that indicates when plan changes happened would be
very useful.

The specifics of how a plan ID is generated are going to have some edge
cases (as you noted)

I concur that the ideal place for this to eventually land would be
alongside queryid in pg_stat_activity

-Jeremy

  3. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Artem Gavrilov <artem.gavrilov@percona.com> — 2025-01-21T18:47:37Z

    On Thu, Jan 2, 2025 at 10:47 PM Lukas Fittl <lukas@fittl.com> wrote:

>
> The first patch allows use of node jumbling by other unit files /
> extensions, which would help an out-of-core extension avoid duplicating all
> the node jumbling code.
>
> The second patch adds a function for the extensible cumulative statistics
> system to drop all entries for a given statistics kind. This already exists
> for resetting, but in case of a dynamic list of entries its more useful to
> be able to drop all of them when "reset" is called.
>
> The third patch adds plan ID tracking in core. This is turned off by
> default, and can be enabled by setting "compute_plan_id" to "on". Plan IDs
> are shown in pg_stat_activity, as well as EXPLAIN and auto_explain output,
> to allow matching a given plan ID to a plan text, without requiring the use
> of an extension. There are some minor TODOs in the plan jumbling logic that
> I haven't finalized yet. There is also an open question whether we should
> use the node attribute mechanism instead of custom jumbling logic?
>
> The fourth patch adds the pg_stat_plans contrib extension, for
> illustrative purposes. This is inspired by pg_stat_statements, but
> intentionally kept separate for easier review and since it does not use an
> external file and could technically be used independently. We may want to
> develop this into a unified pg_stat_statements+plans in-core mechanism in
> the future, but I think that is best kept for a separate discussion.
>
> The pg_stat_plans extension utilizes the cumulative statistics system for
> tracking statistics (extensible thanks to recent changes!), as well as
> dynamic shared memory to track plan texts up to a given limit (2kB by
> default). As a side note, managing extra allocations with the new
> extensible stats is a bit cumbersome - it would be helpful to have a hook
> for cleaning up data associated to entries (like a DSA allocation).
>
> Thanks,
> Lukas
>
> [0]:
> https://www.postgresql.org/message-id/flat/604E3199-2DD2-47DD-AC47-774A6F97DCA9%40amazon.com
> <https://url.avanan.click/v2/r01/___https://www.postgresql.org/message-id/flat/604E3199-2DD2-47DD-AC47-774A6F97DCA9*40amazon.com___.YXAzOnBlcmNvbmE6YTpnOjk5NDUyOGU1MTIwZDhhNDQxNzNiMDM0NjEwZjY1NTIxOjc6YWM0YjpjN2VmMzI5ZmVjMmM2N2RlNDg0MGVlNjJmMGFlOTQ3OGQ1NTM1ODZmZGMxNzI2NGQ4NmEwMDcxYmI1ODVjY2RjOmg6VDpO>
> [1]: https://github.com/2ndQuadrant/pg_stat_plans
> <https://url.avanan.click/v2/r01/___https://github.com/2ndQuadrant/pg_stat_plans___.YXAzOnBlcmNvbmE6YTpnOjk5NDUyOGU1MTIwZDhhNDQxNzNiMDM0NjEwZjY1NTIxOjc6NjM3NTowODVhZWY2OGY1MjdhYWEzY2NiMDY1NTVlNzcwYjM5YTlmOTI5ODU3ZWI5ZWY2NjY1YTljMDBmMWEyNDU0ZmMwOmg6VDpO>
> [2]: https://ossc-db.github.io/pg_store_plans/
> <https://url.avanan.click/v2/r01/___https://ossc-db.github.io/pg_store_plans/___.YXAzOnBlcmNvbmE6YTpnOjk5NDUyOGU1MTIwZDhhNDQxNzNiMDM0NjEwZjY1NTIxOjc6NjU3ZDo0NjA1YzQ1ZTk2ZGEzZmZiNmM5NTEyYjZiMTRmYjk3Y2RjMTE5M2ZkMTMwYTg4ZWM1NjdmMWY1N2RhZjI5YTliOmg6VDpO>
> [3]:
> https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/aurora_stat_plans.html
> <https://url.avanan.click/v2/r01/___https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/aurora_stat_plans.html___.YXAzOnBlcmNvbmE6YTpnOjk5NDUyOGU1MTIwZDhhNDQxNzNiMDM0NjEwZjY1NTIxOjc6ZGNjNTozOGIyNDM2MWVhYzg1MTcyNjc5NzJlZTdkM2JkNzliMjE3NjYzODk5MGQwMTdkNDM1YzliMGU5MDA1ZmEwNzFlOmg6VDpO>
>
> --
> Lukas Fittl
>

Hello Lukas,

We have another extension that does plan ID tracking: pg_stat_monitor. So I
think it would be great to have this functionality in core.

I tested your patch set on top of *86749ea3b76* PG revision on MacOS. All
tests successfully passed. However, pgident shows that some files are not
properly formatted.



-- 

<https://www.percona.com/>

Artem Gavrilov
Senior Software Engineer, Percona

artem.gavrilov@percona.com

  4. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-01-22T00:46:53Z

    On Thu, Jan 02, 2025 at 12:46:04PM -0800, Lukas Fittl wrote:
> Inspired by a prior proposal by Sami Imseih for tracking Plan IDs [0], as
> well as extensions like pg_stat_plans [1] (unmaintained), pg_store_plans
> [2] (not usable on production, see notes later) and aurora_stat_plans [3]
> (enabled by default on AWS), this proposed patch set adds:

0002 introduces this new routine to delete all the entries of the new
stats kind you are adding:
+void
+pgstat_drop_entries_of_kind(PgStat_Kind kind)
+{
+	dshash_seq_status hstat;
+	PgStatShared_HashEntry *ps;
+	uint64		not_freed_count = 0;
+
+	dshash_seq_init(&hstat, pgStatLocal.shared_hash, true);

This is the same as pgstat_drop_all_entries(), except for the filter
based on the stats kind and the fact that you need to take care of the
local reference for an entry of this kind, if there are any, like
pgstat_drop_entry().  Why not, that can be useful on its own depending
on the stats you are working on.  May I suggest the addition of a code
path outside of your main proposal to test this API?  For example
injection_stats.c with a new SQL function to reset everything.

+static void
+pgstat_gc_plan_memory()
+{
+	dshash_seq_status hstat;
+	PgStatShared_HashEntry *p;
+
+	/* dshash entry is not modified, take shared lock */
+	dshash_seq_init(&hstat, pgStatLocal.shared_hash, false);
+	while ((p = dshash_seq_next(&hstat)) != NULL)
+	{
+		PgStatShared_Common *header;
+		PgStat_StatPlanEntry *statent;

Question time: pgstat_drop_entries_of_kind() is called once in 0004,
which does a second sequential scan of pgStatLocal.shared_hash.
That's not efficient, making me question what's the reason to think
why pgstat_drop_entries_of_kind() is the best approach to use.  I like 
the idea of pgstat_drop_entries_of_kind(), less how it's applied in
the context of the main patch.

Mixed feelings about the choices of JumblePlanNode() in 0003 based on
its complexity as implemented.  When it comes to such things, we
should keep the custom node functions short, applying node_attr
instead to the elements of the nodes so as the assumptions behind the
jumbling are documented within the structure definitions in the
headers, not the jumbling code itself.
--
Michael

  5. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Sami Imseih <samimseih@gmail.com> — 2025-01-24T01:25:35Z

    Thanks for starting this thread. This is an important feature.

I am still reviewing, but wanted to share some initial comments.

== pg_stat_plans extension (0004)

1. pg_stat_plans_1_0 should not call pgstat_fetch_entry.l
This is not needed since we already have the entry with a shared lock
and it could lead to an assertion error when pgstat_fetch_entry
may conditionally call dshash_find. dshash_find asserts that the lock
is not already held. Calling pgstat_get_entry_data should be
enough here.

2. a "toplevel" bool field is missing in pg_stat_plans to indicate the
plan is for a nested query.

3. I think we should add cumulative planning_time. This
Probably should be controlled with a separate GUC as well.

4. For deallocation, I wonder if it makes more sense to zap the
plans with the lowest total execution time rather than calls; or make
this configurable. In fact, I think choosing the eviction strategy
should be done in pg_stat_statements as well ( but that belongs
in a separate discussion ). The idea is to give more priority to
plans that have the most overall database time.

5. What are your thoughts about controlling the memory by
size rather than .max and .max_size ? if a typical plan
is 2KB, a user can fit 10k plans with 20MB. A typical
user can probably allocate much more memory for this
purpose.

Also, pgstat_gc_plans is doing a loop over the
hash to get the # of entries. I don't think this
is a good idea for performance and it may not be possible to
actually enforce the .max on a dshash since
the lock is taken on a partition level.

6. I do like the idea of showing an in-flight plan.
This is so useful for a rare plan, especially on the
first capture of the plan ( calls = 0), and the planId
can be joined with pg_stat_activity to get the query
text.

 /* Record initial entry now, so plan text is available for currently
running queries */
        pgstat_report_plan_stats(queryDesc,
                                 0, /* executions are counted in
pgsp_ExecutorEnd */
                                    0.0);

We will need to be clear in the documentation
that calls being 0 is a valid scenario.


== core plan id computation (0003)

1. compute_plan_id should do exactly what compute_query_id
does. It should have an "auto" as the default which automatically
computes a plan id when pg_stat_plans is enabled.

2.
> Mixed feelings about the choices of JumblePlanNode() in 0003 based on
> its complexity as implemented. When it comes to such things, we
> should keep the custom node functions short, applying node_attr
> instead to the elements of the nodes so as the assumptions behind the
> jumbling are documented within the structure definitions in the
> headers, not the jumbling code itself.

+1

we should be able to control which node is considered for plan_id
computation using a node attribute such as plan_jumble_ignore.
I played around with this idea by building on top of your proposal
and attached my experiment code for this. The tricky part will be finalizing
which nodes and node fields to use for plan computation.

3. We may want to combine all the jumbling code into
a single jumble.c since the query and plan jumble will
share a lot of the same code, i.e. JumbleState.
_JumbleNode, etc.


Regards,

Sami Imseih
Amazon Web Services (AWS)

  6. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Lukas Fittl <lukas@fittl.com> — 2025-01-24T07:44:01Z

    On Tue, Jan 21, 2025 at 10:47 AM Artem Gavrilov <artem.gavrilov@percona.com>
wrote:

> We have another extension that does plan ID tracking: pg_stat_monitor. So
> I think it would be great to have this functionality in core.
>

Thanks! I had forgotten that pg_stat_monitor can optionally track plan
statistics. Its actually another data point for why the plan ID calculation
should be in core:

Like pg_store_plans, pg_stat_monitor is hashing the plan text to calculate
the plan ID [0], which can have measurable overhead (judging from our
benchmarks of pg_store_plans). It also utilizes EXPLAIN (COSTS OFF) for
getting the plan text [1], which tracks with my thinking as to what should
be considered significant for the plan ID jumbling.

I tested your patch set on top of *86749ea3b76* PG revision on MacOS. All
> tests successfully passed. However, pgident shows that some files are not
> properly formatted.
>

Thanks, appreciate the test and note re: pgident, taking care of that in
the next patch refresh.

Thanks,
Lukas

[0]:
https://github.com/percona/pg_stat_monitor/blob/main/pg_stat_monitor.c#L730
[1]:
https://github.com/percona/pg_stat_monitor/blob/main/pg_stat_monitor.c#L678

-- 
Lukas Fittl

  7. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Andrei Lepikhov <lepihov@gmail.com> — 2025-01-24T09:23:08Z

    On 1/3/25 03:46, Lukas Fittl wrote:
> My overall perspective is that (1) is best done in-core to keep overhead 
> low, whilst (2) could be done outside of core (or merged with a future 
> pg_stat_statements) and is included here mainly for illustration purposes.
Thank you for the patch and your attention to this issue!

I am pleased with the export of the jumbling functions and their 
generalisation.

I may not be close to the task monitoring area, but I utilise queryId 
and other tools to differ plan nodes inside extensions. Initially, like 
queryId serves as a class identifier for queries, plan_id identifies a 
class of nodes, not a single node. In the implementation provided here, 
nodes with the same hash can represent different subtrees. For example, 
JOIN(A, JOIN(B,C)) and JOIN(JOIN(B,C),A) may have the same ID.

Moreover, I wonder if this version of plan_id reacts to the join level 
change. It appears that only a change of the join clause alters the 
plan_id hash value, which means you would end up with a single hash for 
very different plan nodes. Is that acceptable? To address this, we 
should consider the hashes of the left and right subtrees and the hashes 
of each subplan (especially in the case of Append).

Overall, similar to discussions on queryId, various extensions may want 
different logic for generating plan_id (more or less unique guarantees, 
for example). Hence, it would be beneficial to separate this logic and 
allow extensions to provide different plan_ids. IMO, What we need is a 
'List *ext' field in each of the Plan, Path, PlanStmt, and Query 
structures. Such 'ext' field may contain different stuff that extensions 
want to push without interference between them - specific plan_id as an 
example.

Additionally, we could bridge the gap between the cloud of paths and the 
plan by adding a hook at the end of the create_plan_recurse routine. 
This may facilitate the transfer of information regarding optimiser 
decisions that could be influenced by an extension into the plan.

-- 
regards, Andrei Lepikhov

  8. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Lukas Fittl <lukas@fittl.com> — 2025-01-24T09:59:00Z

    Thanks for the reviews! Attached an updated v2 patch set, notes inline
below.

On Tue, Jan 21, 2025 at 4:47 PM Michael Paquier <michael@paquier.xyz> wrote:

> May I suggest the addition of a code
> path outside of your main proposal to test this API?  For example
> injection_stats.c with a new SQL function to reset everything.
>

Good idea - added an example use of this to injection_stats.c in the
attached 0002.

Question time: pgstat_drop_entries_of_kind() is called once in 0004,
> which does a second sequential scan of pgStatLocal.shared_hash.
> That's not efficient, making me question what's the reason to think
> why pgstat_drop_entries_of_kind() is the best approach to use.  I like
> the idea of pgstat_drop_entries_of_kind(), less how it's applied in
> the context of the main patch.
>

My motivation for doing two scans here, one in pgstat_drop_entries_of_kind
and one in pgstat_gc_plan_memory (both called from the reset function) was
that the first time through we hold an exclusive lock on
pgStatLocal.shared_hash, vs the second time (when we free plan texts) we
hold a share lock.

Maybe that doesn't matter, since "dsa_free" is fast anyway, and we can just
do this all in one go whilst holding an exclusive lock?

Overall, I also do wonder if it wouldn't be better to have a callback
mechanism in the shared memory stats, so stats plugins can do extra work
when an entry gets dropped (like freeing the DSA memory for the plan text),
vs having to add all this extra logic to do it.

On Thu, Jan 23, 2025 at 5:25 PM Sami Imseih <samimseih@gmail.com> wrote:

> Thanks for starting this thread. This is an important feature.
>
> I am still reviewing, but wanted to share some initial comments.
>

Thanks for taking the time! I had started on a v2 patch based on Michael's
note before I saw your email and experiment, so apologies for sending this
in the middle of your review :)

== pg_stat_plans extension (0004)
>
> 1. pg_stat_plans_1_0 should not call pgstat_fetch_entry.l
> This is not needed since we already have the entry with a shared lock
> and it could lead to an assertion error when pgstat_fetch_entry
> may conditionally call dshash_find. dshash_find asserts that the lock
> is not already held. Calling pgstat_get_entry_data should be
> enough here.
>

Fixed.


> 2. a "toplevel" bool field is missing in pg_stat_plans to indicate the
> plan is for a nested query.
>

Good point - added.

3. I think we should add cumulative planning_time. This
> Probably should be controlled with a separate GUC as well.
>

Hmm. Don't we already have that in pg_stat_statements?

Though, in practice I see that turned off most of the time (due to its
overhead?), not sure if we could do better if it this was done here instead?

4. For deallocation, I wonder if it makes more sense to zap the
> plans with the lowest total execution time rather than calls; or make
> this configurable. In fact, I think choosing the eviction strategy
> should be done in pg_stat_statements as well ( but that belongs
> in a separate discussion ). The idea is to give more priority to
> plans that have the most overall database time.
>

Yeah, that's a good point, its likely people would be most interested in
slow plans, vs those that were called a lot.

Happy to adjust it that way - I don't think we need to make it configurable
to be honest.

5. What are your thoughts about controlling the memory by
> size rather than .max and .max_size ? if a typical plan
> is 2KB, a user can fit 10k plans with 20MB. A typical
> user can probably allocate much more memory for this
> purpose.
>

Interesting idea! I'd be curious to get more feedback on the overall
approach here before digging into this, but I like this as it'd be more
intuitive from an end-user perspective.

6. I do like the idea of showing an in-flight plan.
> This is so useful for a rare plan, especially on the
> first capture of the plan ( calls = 0), and the planId
> can be joined with pg_stat_activity to get the query
> text.
>

Yes indeed, I think it would be a miss if we didn't allow looking at
in-flight plan IDs.

For the record, I can't take credit for the idea, I think I got this either
from your earlier plan ID patch, or from talking with you at PGConf NYC
last year.

== core plan id computation (0003)
>
> 1. compute_plan_id should do exactly what compute_query_id
> does. It should have an "auto" as the default which automatically
> computes a plan id when pg_stat_plans is enabled.
>

Yep, it does seem better to be consistent here. I added "auto" in v2 and
made it the default.

> Mixed feelings about the choices of JumblePlanNode() in 0003 based on
> > its complexity as implemented. When it comes to such things, we
> > should keep the custom node functions short, applying node_attr
> > instead to the elements of the nodes so as the assumptions behind the
> > jumbling are documented within the structure definitions in the
> > headers, not the jumbling code itself.
>
> +1
>
> we should be able to control which node is considered for plan_id
> computation using a node attribute such as plan_jumble_ignore.
> I played around with this idea by building on top of your proposal
> and attached my experiment code for this. The tricky part will be
> finalizing
> which nodes and node fields to use for plan computation.
>

Agreed, its better to do this via the node_attr infrastructure. I've done
this in the attached before I saw your experiment code, so it may be worth
comparing the approaches.

Generally, I tried to stay closer to the idea of "only jumble what EXPLAIN
(COSTS OFF) would show", vs jumbling most plan fields by default.

That does mean we have a lot of extra "node_attr(query_jumble_ignore)" tags
in the plan node structs. We could potentially invent a new way of only
jumbling what's marked vs the current jumbling all by default + ignoring
some fields, but not sure if that's worth it.

3. We may want to combine all the jumbling code into
> a single jumble.c since the query and plan jumble will
> share a lot of the same code, i.e. JumbleState.
> _JumbleNode, etc.
>

Agreed, that's what I ended up doing in v2. I think we can state that plan
jumbling is a super set of query jumbling, so it seems best to not have two
copies of very similar jumbling conds/funcs.

I retained the "query" prefix for now to not generate a big diff, but we
should maybe consider dropping that in both the source file names and the
node attributes?

Thanks,
Lukas

-- 
Lukas Fittl

  9. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-01-27T03:53:36Z

    On Fri, Jan 24, 2025 at 01:59:00AM -0800, Lukas Fittl wrote:
> Overall, I also do wonder if it wouldn't be better to have a callback
> mechanism in the shared memory stats, so stats plugins can do extra work
> when an entry gets dropped (like freeing the DSA memory for the plan text),
> vs having to add all this extra logic to do it.

Not sure about this part yet.  I have looked at 0002 to begin with
something and it is really useful on its own.  Stats kinds calling
this routine don't need to worry about the internals of dropping local
references or doing a seqscan on the shared hash table.  However, what
you have sent lacks in flexibility to me, and the duplication with
pgstat_drop_all_entries is annoying.  This had better be merged in a
single routine.

Attached is an updated version that adds an optional "do_drop"
callback in the function that does the seqscan on the dshash, to
decide if an entry should be gone or not.  This follows the same model
as the "reset" part, where stats kind can push the matching function
they want to work on the individual entries.  We could add a
pgstat_drop_entries_of_kind(), but I'm not feeling that this is
strongly necessary with the basic interface in place.

The changes in the module injection_points were not good.  The SQL
function was named "reset" but that's a drop operation.

What do you think?
--
Michael

  10. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-01-31T04:37:03Z

    On Mon, Jan 27, 2025 at 12:53:36PM +0900, Michael Paquier wrote:
> Not sure about this part yet.  I have looked at 0002 to begin with
> something and it is really useful on its own.  Stats kinds calling
> this routine don't need to worry about the internals of dropping local
> references or doing a seqscan on the shared hash table.  However, what
> you have sent lacks in flexibility to me, and the duplication with
> pgstat_drop_all_entries is annoying.  This had better be merged in a
> single routine.

After thinking more about this one, I still want this toy and hearing
nothing I have applied it, with a second commit for the addition in
injection_points to avoid multiple bullet points in a single commit.

I have noticed post-commit that I have made a mistake in the credits
of a632cd354d35 and ce5c620fb625 for your family name.  Really sorry
about that!  This mistake is on me..

> What do you think?

Attached is a rebased version of the three remaining patches.  While
looking at this stuff, I have noticed an extra cleanup that would be
good to have, as a separate change: we could reformat a bit the plan
header comments so as these do not require a rewrite when adding
node_attr to them, like d575051b9af9.

Sami's patch set posted at [1] has the same problem, making the
proposals harder to parse and review, and the devil is in the details
with these pg_node_attr() properties attached to the structures.  That
would be something to do on top of the proposed patch sets.  Would any
of you be interested in that?

[1]: https://www.postgresql.org/message-id/CAA5RZ0sUPPOpkRZD=Za83op2ngcPC7dp249vcHA-X5YS7p3n8Q@mail.gmail.com
--
Michael

  11. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Lukas Fittl <lukas@fittl.com> — 2025-01-31T05:19:49Z

    On Thu, Jan 30, 2025 at 8:37 PM Michael Paquier <michael@paquier.xyz> wrote:

> After thinking more about this one, I still want this toy and hearing
> nothing I have applied it, with a second commit for the addition in
> injection_points to avoid multiple bullet points in a single commit.
>

Thanks for committing! I had intended to review/test your patch, but the
earlier parts of the week got way too busy.

I think the API with do_drop makes sense, and whilst I'd think there is
some extra overhead to calling the function vs having an inline check for
kind, it seems unlikely this would be used in a performance critical
context, and the flexibility seems useful.

I have noticed post-commit that I have made a mistake in the credits
> of a632cd354d35 and ce5c620fb625 for your family name.  Really sorry
> about that!  This mistake is on me..
>

No worries regarding the name, happens to me all the time :)

> What do you think?
>
> Attached is a rebased version of the three remaining patches.  While
> looking at this stuff, I have noticed an extra cleanup that would be
> good to have, as a separate change: we could reformat a bit the plan
> header comments so as these do not require a rewrite when adding
> node_attr to them, like d575051b9af9.
>

Yeah, I think that'd be helpful to move the comments before the fields - it
definitely gets hard to read.

Sami's patch set posted at [1] has the same problem, making the
> proposals harder to parse and review, and the devil is in the details
> with these pg_node_attr() properties attached to the structures.  That
> would be something to do on top of the proposed patch sets.  Would any
> of you be interested in that?
>

I'd be happy to tackle that - were you thinking to simply move any comments
before the field, in each case where we're adding an annotation?

Separately I've been thinking how we could best have a discussion/review on
whether the jumbling of specific plan struct fields is correct. I was
thinking maybe a quick wiki page could be helpful, noting why to jumble/not
jumble certain fields?

Thanks,
Lukas

-- 
Lukas Fittl

  12. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-01-31T05:33:14Z

    On Thu, Jan 30, 2025 at 09:19:49PM -0800, Lukas Fittl wrote:
> I'd be happy to tackle that - were you thinking to simply move any comments
> before the field, in each case where we're adding an annotation?

Yes.

> Separately I've been thinking how we could best have a discussion/review on
> whether the jumbling of specific plan struct fields is correct. I was
> thinking maybe a quick wiki page could be helpful, noting why to jumble/not
> jumble certain fields?

Makes sense.  This is a complicated topic.
--
Michael

  13. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Sami Imseih <samimseih@gmail.com> — 2025-02-04T23:14:48Z

    >> Separately I've been thinking how we could best have a discussion/review on
>> whether the jumbling of specific plan struct fields is correct. I was
>> thinking maybe a quick wiki page could be helpful, noting why to jumble/not
>> jumble certain fields?

> Makes sense.  This is a complicated topic.

+1 for the Wiki page

I started looking at the set of patches and started with v3-0001.
For that one, I think we need to refactor a bit more for
maintainability/readability.

queryjumblefuncs.c now has dual purposes which is the generic node jumbling
code and now it also has the specific query jumbling code. That seems wrong
from a readability/maintainability perspective.

Here are my high-level thoughts on this:
1. rename queryjumblefuncs.c to jumblefuncs.c
2. move the query jumbling related code to parser/analyze.c,
since query jumbling occurs there during parsing.
3. Rewrite the comments in the new jumblefuncs.c to
make it clear the intention of this infrastructure; that
it is used to jumble nodes for query or plan trees.

I can work on this if you agree.

Regards,

Sami

  14. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-02-05T00:12:14Z

    On Tue, Feb 04, 2025 at 05:14:48PM -0600, Sami Imseih wrote:
> Here are my high-level thoughts on this:
> 1. rename queryjumblefuncs.c to jumblefuncs.c

If these APIs are used for somethings else than Query structure, yes,
the renaming makes sense.

> 2. move the query jumbling related code to parser/analyze.c,
> since query jumbling occurs there during parsing.

Not sure about this one.  It depends on how much is changed.  As long
as everything related to the nodes stays in src/backend/nodes/,
perhaps that's OK.

> 3. Rewrite the comments in the new jumblefuncs.c to
> make it clear the intention of this infrastructure; that
> it is used to jumble nodes for query or plan trees.

Seems to me that this could be done before 2, as well.

> I can work on this if you agree.

I'd welcome an extra patch to rework a bit the format of the comments
for the Plan nodes, to ease the addition of pg_node_attr(), making any
proposed patches more readable.
--
Michael

  15. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Sami Imseih <samimseih@gmail.com> — 2025-02-05T01:31:46Z

    >> I can work on this if you agree.

> I'd welcome an extra patch to rework a bit the format of the comments
> for the Plan nodes, to ease the addition of pg_node_attr(), making any
> proposed patches more readable.

I'll take care of this also.

Regards,

Sami

  16. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Lukas Fittl <lukas@fittl.com> — 2025-02-05T02:09:50Z

    Looks like some emails were sent before I could send my draft email, but
hopefully this should make the follow up work easier :)

Attached a v4 patch set with a few minor changes to plan ID jumbling:

* Range table jumbling is now done in a separate JumbleRangeTable function
after setrefs.c walked the tree - this way we avoid having custom logic for
RT Indexes in the node jumbling, and keeping a reference to PlannerGlobal
in the jumble struct
* Moved the JumbleNode call to the bottom of the set_plan_references
function for clarity - previously it was before descending into inner/outer
plan, but after some other recursive calls to set_plan_references, which
didn't really make sense
* Fixed a bug with JUMBLE_ARRAY incorrectly taking the reference of the
array (which caused planid to change incorrectly between runs)
* Added JUMBLE_BITMAPSET

Further, I've significantly reduced the number of fields ignored for plan
jumbling:

Basically the approach taken in this version is that only things that would
negatively affect the planid (i.e. make it unique when it shouldn't be) are
ignored, vs ignoring duplicate fields and fields that are only used by the
executor (which is what v1-v3 did). I'm not 100% sure that's the right
approach (but it does keep the diff a good amount smaller), I think the
tradeoff here is basically jumbling performance vs maintenance overhead
when fields are added/changed.

This does not yet move field-specific comments to their own line in nodes
where we're adding node attributes, I'll leave that for Sami to work on.

On Tue, Feb 4, 2025 at 3:15 PM Sami Imseih <samimseih@gmail.com> wrote:

> >> Separately I've been thinking how we could best have a
> discussion/review on
> >> whether the jumbling of specific plan struct fields is correct. I was
> >> thinking maybe a quick wiki page could be helpful, noting why to
> jumble/not
> >> jumble certain fields?
>
> > Makes sense.  This is a complicated topic.
>
> +1 for the Wiki page
>

Started here: https://wiki.postgresql.org/wiki/Plan_ID_Jumbling

Thanks,
Lukas

-- 
Lukas Fittl

  17. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Lukas Fittl <lukas@fittl.com> — 2025-02-05T02:16:07Z

    Hi Andrei,

On Fri, Jan 24, 2025 at 1:23 AM Andrei Lepikhov <lepihov@gmail.com> wrote:

> I may not be close to the task monitoring area, but I utilise queryId
> and other tools to differ plan nodes inside extensions. Initially, like
> queryId serves as a class identifier for queries, plan_id identifies a
> class of nodes, not a single node. In the implementation provided here,
> nodes with the same hash can represent different subtrees. For example,
> JOIN(A, JOIN(B,C)) and JOIN(JOIN(B,C),A) may have the same ID.
>

> Moreover, I wonder if this version of plan_id reacts to the join level
> change. It appears that only a change of the join clause alters the
> plan_id hash value, which means you would end up with a single hash for
> very different plan nodes. Is that acceptable? To address this, we
> should consider the hashes of the left and right subtrees and the hashes
> of each subplan (especially in the case of Append).
>

I looked back at this again just to confirm we're not missing anything:

I don't think any of the posted patch versions (including the just shared
v4) have a problem with distinguishing two plans that are very similar but
only differ in JOIN order. Since we descend into the inner/outer plans via
the setrefs.c treewalk, the placement of JOIN nodes vs other nodes should
cause a different plan jumble (and we include both the node tag for the
join/scan nodes, as well as the RT index the scans point to in the jumble).

Do you have a reproducer that shows these two generate the same plan ID?

Thanks,
Lukas

-- 
Lukas Fittl

  18. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Andrei Lepikhov <lepihov@gmail.com> — 2025-02-05T04:03:52Z

    On 2/5/25 09:16, Lukas Fittl wrote:
> Hi Andrei,
> 
> On Fri, Jan 24, 2025 at 1:23 AM Andrei Lepikhov <lepihov@gmail.com 
> <mailto:lepihov@gmail.com>> wrote:
> 
>     I may not be close to the task monitoring area, but I utilise queryId
>     and other tools to differ plan nodes inside extensions. Initially, like
>     queryId serves as a class identifier for queries, plan_id identifies a
>     class of nodes, not a single node. In the implementation provided here,
>     nodes with the same hash can represent different subtrees. For example,
>     JOIN(A, JOIN(B,C)) and JOIN(JOIN(B,C),A) may have the same ID.
> 
> 
>     Moreover, I wonder if this version of plan_id reacts to the join level
>     change. It appears that only a change of the join clause alters the
>     plan_id hash value, which means you would end up with a single hash for
>     very different plan nodes. Is that acceptable? To address this, we
>     should consider the hashes of the left and right subtrees and the
>     hashes
>     of each subplan (especially in the case of Append).
> 
> 
> I looked back at this again just to confirm we're not missing anything:
> 
> I don't think any of the posted patch versions (including the just 
> shared v4) have a problem with distinguishing two plans that are very 
> similar but only differ in JOIN order. Since we descend into the inner/ 
> outer plans via the setrefs.c treewalk, the placement of JOIN nodes vs 
> other nodes should cause a different plan jumble (and we include both 
> the node tag for the join/scan nodes, as well as the RT index the scans 
> point to in the jumble).
Maybe. I haven't dive into that stuff deeply yet. It is not difficult to 
check.
The main point was that different extensions want different plan_ids. 
For example, planner extensions want to guarantee the distinctness and 
sort of stability of this field inside a query plan. Does the hash value 
guarantee that?
We have discussed how queryId should be generated more than once. That's 
why I think the plan_id generation logic should be implemented inside an 
extension, not in the core.


-- 
regards, Andrei Lepikhov

  19. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Sami Imseih <samimseih@gmail.com> — 2025-02-07T01:52:53Z

    > This does not yet move field-specific comments to their own line in nodes where we're adding node attributes, I'll leave that for Sami to work on.
>

Hi,

Attached is a new set of patches for fixing the long comments
in plannodes.h and to refactor queryjumblefuncs.c

v5-0001
-----------

This fixes the long comments in plannodes.h to make it easier to add the
attribute annotation. It made the most sense to make this the first patch
in the set.


v5-0002
-----------
>> Here are my high-level thoughts on this:
>> 1. rename queryjumblefuncs.c to jumblefuncs.c

> If these APIs are used for somethings else than Query structure, yes,
> the renaming makes sense.

Done. Also rewrote the header comment in jumblefuncs.c to describe
a more generic node jumbling mechanism that this file now offers.

>> 2. move the query jumbling related code to parser/analyze.c,
>> since query jumbling occurs there during parsing.

> Not sure about this one.  It depends on how much is changed.  As long
> as everything related to the nodes stays in src/backend/nodes/,
> perhaps that's OK.

Yes, after getting my hands on this, I agree with you. It made more sense
to keep all the jumbling work in jumblefuncs.c

v5-0003 and v5-0004 introduce the planId in core and pg_stat_plans. These
needed rebasing only; but I have not yet looked at this thoroughly.

We should aim to get 0001 and 0002 committed next.

Regards,

Sami

  20. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-02-10T01:25:05Z

    On Thu, Feb 06, 2025 at 07:52:53PM -0600, Sami Imseih wrote:
> This fixes the long comments in plannodes.h to make it easier to add the
> attribute annotation. It made the most sense to make this the first patch
> in the set.

A commit that happened last Friday made also this to have conflict.

> Done. Also rewrote the header comment in jumblefuncs.c to describe
> a more generic node jumbling mechanism that this file now offers.
>
> Yes, after getting my hands on this, I agree with you. It made more sense
> to keep all the jumbling work in jumblefuncs.c

-static void AppendJumble(JumbleState *jstate,
-                         const unsigned char *item, Size size

I don't understand why there is a need for publishing AppendJumble()
while it remains statis in jumblefuncs.c.  This is not needed in 0003
and 0004, either.

Should we use more generic names for the existing custom_query_jumble,
no_query_jumble, query_jumble_ignore and query_jumble_location?  Last
time I've talked about that with Peter E, "jumble" felt too generic,
so perhaps we're looking for a completely new term?  This impacts as
well the naming of the existing queryjumblefuncs.c.  The simplest term
that may be possible here is "hash", actually, because that's what we
are doing with all these node structures?  That's also a very generic
term. The concept of location does not apply to plans, based on the
current proposal, so perhaps we should talk about "query normalization
location"?

Point is that query_jumble_ignore is used in the planner nodes, which
feels inconsistent, so perhaps we could rename query_jumble_ignore and
no_query_jumble to "hash_ignore" and/or "no_hash", or something like
that?  This may point towards the need of a split, not sure, still the
picture is incomplete.

> v5-0003 and v5-0004 introduce the planId in core and pg_stat_plans. These
> needed rebasing only; but I have not yet looked at this thoroughly.
> 
> We should aim to get 0001 and 0002 committed next.

Yeah.  I didn't see any reasons why 0001 should not happen now, as it
makes the whole easier while making the header styles a bit more
consistent.  Perhaps also if somebody forks the code and adds some
pg_node_attr() properties?

v5-0003 and v5-0004, not sure yet.  The intrisincs of the planner make
putting a strict definition of what a hash means hard to set down,
we should work towards studying that more first.  I don't see this
happen until the next release freeze.
--
Michael

  21. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Sami Imseih <samimseih@gmail.com> — 2025-02-10T20:02:10Z

    > This fixes the long comments in plannodes.h to make it easier to add the
> attribute annotation. It made the most sense to make this the first patch
> in the set.

> A commit that happened last Friday made also this to have conflict.

Thanks for committing v5-0001. I am not sure why there is comment
that is not correctly indented. Attached is a fix for that


> I don't understand why there is a need for publishing AppendJumble()
> while it remains statis in jumblefuncs.c.  This is not needed in 0003
> and 0004, either.

In v5-0002, AppendJumble is no longer static.

-static void
+void
 AppendJumble(JumbleState *jstate, const unsigned char *item, Size size)
 {

Maybe I am missing something?

> Should we use more generic names for the existing custom_query_jumble,
> no_query_jumble, query_jumble_ignore and query_jumble_location?  Last
> time I've talked about that with Peter E, "jumble" felt too generic,
> so perhaps we're looking for a completely new term?  This impacts as
> well the naming of the existing queryjumblefuncs.c.  The simplest term
> that may be possible here is "hash", actually, because that's what we

> Point is that query_jumble_ignore is used in the planner nodes, which
> feels inconsistent, so perhaps we could rename query_jumble_ignore and
> no_query_jumble to "hash_ignore" and/or "no_hash", or something like
> that?  This may point towards the need of a split, not sure, still the
> picture is incomplete.

I was thinking about this as I was reworking the comments in jumblefuncs.c
for v5-0002.

I am OK with moving away from "jumble" in-lieu of something else, but
my thoughts are we should actually call this process "fingerprint"
( a term we already use in the queryjumblefuncs.c comment ).
A fingerprint consists of all the interesting parts of a node tree that are
appended and the final product is a hash of this fingerprint ( i.e. queryId )
For node attributes we can specify "fingerprint_ignore"
or "no_fingerprint". What do you think?

> The concept of location does not apply to plans, based on the
> current proposal, so perhaps we should talk about "query normalization
> location"?

Are you referring to JUMBLE_LOCATION? and whether to keep it in
queryjumblefuncs.c ( or jumblefuncs.c as is being proposed )?


Regards,

Sami

  22. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Sami Imseih <samimseih@gmail.com> — 2025-02-10T20:14:09Z

    Another thought that I have is that If we mention that extensions can use
these jumbling ( or whatever the final name is ) functions outside of
core, it makes
sense to actually show an example of this. What do you think?

-- 
Sami

  23. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-02-10T22:43:09Z

    On Mon, Feb 10, 2025 at 02:02:10PM -0600, Sami Imseih wrote:
> Thanks for committing v5-0001. I am not sure why there is comment
> that is not correctly indented. Attached is a fix for that

Thanks, fixed.  The reason behind that is likely that I have fat
fingers.
--
Michael

  24. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-02-11T23:57:23Z

    On Mon, Feb 10, 2025 at 02:14:09PM -0600, Sami Imseih wrote:
> Another thought that I have is that If we mention that extensions can use
> these jumbling ( or whatever the final name is ) functions outside of
> core, it makes
> sense to actually show an example of this. What do you think?

Not sure.  Do you have anything specific in mind that pgss is not able
to achieve with its jumbling based on the query strings?
--
Michael

  25. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-02-12T00:08:00Z

    On Mon, Feb 10, 2025 at 02:02:10PM -0600, Sami Imseih wrote:
> I am OK with moving away from "jumble" in-lieu of something else, but
> my thoughts are we should actually call this process "fingerprint"
> ( a term we already use in the queryjumblefuncs.c comment ).
> A fingerprint consists of all the interesting parts of a node tree that are
> appended and the final product is a hash of this fingerprint ( i.e. queryId )
> For node attributes we can specify "fingerprint_ignore"
> or "no_fingerprint". What do you think?

I think that I have a long history of showing a bad naming sense, that
I've done some follow-up API renames even on stable branches because
folks didn't like some names, and that I have a reputation for that on
these lists.  :D

Wikipedia seems to agree with you that "fingerprint" would fit for
this purpose, though:
https://en.wikipedia.org/wiki/Fingerprint_(computing)

Has anybody any comments about that?  That would be a large renaming,
but in the long term is makes sense if we want to apply that to more
than just parse nodes and query strings.  If you do that, it impacts
the file names and the properties, that are hidden in the backend for
most of it, except the entry API and JumbleState.  This last part
impacts some extensions and I have been maintaining one a bit
(pg_hint_plan).

>> The concept of location does not apply to plans, based on the
>> current proposal, so perhaps we should talk about "query normalization
>> location"?
> 
> Are you referring to JUMBLE_LOCATION? and whether to keep it in
> queryjumblefuncs.c ( or jumblefuncs.c as is being proposed )?

Yes, I am referring to the existing jumble location.  I don't quite
see how it fits with the plan part because we don't really have
locations to track.

Point worth noting, Alvaro has mentioned that he was planning to look
at the pgss patch with IN clauses:
https://www.postgresql.org/message-id/202502111214.fcfodex6t3sy@alvherre.pgsql

Adding him in CC here for awareness, but I can see that both of you
are involved on the other thread, as well.  Also adding Julien in CC,
as he has some out-of-core extension code that depends on the jumbling
structures if I recall correctly.
--
Michael

  26. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Julien Rouhaud <rjuju123@gmail.com> — 2025-02-12T01:20:53Z

    On Wed, Feb 12, 2025 at 09:08:00AM +0900, Michael Paquier wrote:
> Wikipedia seems to agree with you that "fingerprint" would fit for
> this purpose, though:
> https://en.wikipedia.org/wiki/Fingerprint_(computing)
>
> Has anybody any comments about that?  That would be a large renaming,
> but in the long term is makes sense if we want to apply that to more
> than just parse nodes and query strings.  If you do that, it impacts
> the file names and the properties, that are hidden in the backend for
> most of it, except the entry API and JumbleState.  This last part
> impacts some extensions and I have been maintaining one a bit
> (pg_hint_plan).

I agree that fingerprint is a good improvement.

>
> Also adding Julien in CC,
> as he has some out-of-core extension code that depends on the jumbling
> structures if I recall correctly.

I do have an extension to support custom fingerprinting logic, but the
introduction of the pg_node_attr based jumbling kind of broke it.

FTR my main motivation was to be able to deal with queries referencing
temporary relations, as if your application creates a lot of those it basically
means that you cannot use pg_stat_statements anymore.

  27. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-02-12T01:59:04Z

    On Wed, Feb 12, 2025 at 09:20:53AM +0800, Julien Rouhaud wrote:
> On Wed, Feb 12, 2025 at 09:08:00AM +0900, Michael Paquier wrote:
>> Wikipedia seems to agree with you that "fingerprint" would fit for
>> this purpose, though:
>> https://en.wikipedia.org/wiki/Fingerprint_(computing)
>>
>> Has anybody any comments about that?  That would be a large renaming,
>> but in the long term is makes sense if we want to apply that to more
>> than just parse nodes and query strings.  If you do that, it impacts
>> the file names and the properties, that are hidden in the backend for
>> most of it, except the entry API and JumbleState.  This last part
>> impacts some extensions and I have been maintaining one a bit
>> (pg_hint_plan).
> 
> I agree that fingerprint is a good improvement.

Okay, thanks.  So this would mean something for the file names, the
node_attr names, the structures and the APIs if we put all that under
the same label.

> >
> > Also adding Julien in CC,
> > as he has some out-of-core extension code that depends on the jumbling
> > structures if I recall correctly.
> 
> I do have an extension to support custom fingerprinting logic, but the
> introduction of the pg_node_attr based jumbling kind of broke it.
> 
> FTR my main motivation was to be able to deal with queries referencing
> temporary relations, as if your application creates a lot of those it basically
> means that you cannot use pg_stat_statements anymore.

Do you have an issue more details about your problem?  If we can
improve the situation in core without impacting the existing cases
that we need to support in pgss, that may be worth looking at.
--
Michael

  28. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Julien Rouhaud <rjuju123@gmail.com> — 2025-02-12T02:22:53Z

    On Wed, Feb 12, 2025 at 10:59:04AM +0900, Michael Paquier wrote:
> On Wed, Feb 12, 2025 at 09:20:53AM +0800, Julien Rouhaud wrote:
> >
> > FTR my main motivation was to be able to deal with queries referencing
> > temporary relations, as if your application creates a lot of those it basically
> > means that you cannot use pg_stat_statements anymore.
>
> Do you have an issue more details about your problem?  If we can
> improve the situation in core without impacting the existing cases
> that we need to support in pgss, that may be worth looking at.

I thought this was a well known limitation.  The basic is that if you rely on
temp tables, you usually end up with a virtually infinite number of queryids
since all temp tables get a different oid and that oid is used in the queryid
computation.  And in that case the overhead of pg_stat_statements is insanely
high.  The last figures I saw was by Andres many years ago, with a mention 40%
overhead, and I don't think it's hard to get way worse overhead than that if
you have lengthier query texts.

As a prototype in my extension I think I just entirely ignored such queries,
but another (and probably friendlier for the actual pg_stat_statements
statistics) approach would be to use the relation name to compute the queryid
rather than its oid.  This would add some overhead, but I think it would have
very limited impact especially compared to the current situation.

Of course some people may want to keep the current behavior, if they have
limited number of temp tables or similar, so I had a GUC for that.  I don't
think that the community would really welcome such GUC for core-postgres,
especially since it wouldn't be pg_stat_statements specific.

  29. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Greg Sabino Mullane <htamfids@gmail.com> — 2025-02-12T02:54:33Z

    On Tue, Feb 11, 2025 at 7:08 PM Michael Paquier <michael@paquier.xyz> wrote:

> On Mon, Feb 10, 2025 at 02:02:10PM -0600, Sami Imseih wrote:
> > I am OK with moving away from "jumble" in-lieu of something else, but my
> thoughts are we should actually call this process "fingerprint"
>

I agree fingerprint is the right final word. But "jumble" conveys the
*process* better than "fingerprinting". I view it as jumbling produces an
object that can be fingerprinted.

> For node attributes we can specify "fingerprint_ignore" or
> "no_fingerprint". What do you think?
>

Still should be jumble_ignore.

Cheers,
Greg


--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support

  30. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Sami Imseih <samimseih@gmail.com> — 2025-02-12T02:57:46Z

    > Of course some people may want to keep the current behavior, if they have
> limited number of temp tables or similar, so I had a GUC for that.  I don't
> think that the community would really welcome such GUC for core-postgres,
> especially since it wouldn't be pg_stat_statements specific.

FWIW, I think options to tweak queryId computation is something
that should be in core. It was discussed earlier in the context
of IN list merging; the patch for this currently has the guc
for the feature in pg_stat_statements, but there was a discussion
about actually moving this to core [1] Giving the user a way
to control certain behavior about the queryId computation
is a good thing to do in core; especially queryId is no longer
just consumed in pg_stat_statements. Maybe the right answer
is an enum GUC, not sure yet.

Specifically for the use-case you mention, using names vs OIDs in
queryId computation is a valid use case for more than temporary tables,
I can also think of upgrade, dump/restore, logical replication cases which
can then allow for a consistent queryId.

[1] https://www.postgresql.org/message-id/202502111852.btskmr7nhien%40alvherre.pgsql

-- 
Sami

  31. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Julien Rouhaud <rjuju123@gmail.com> — 2025-02-12T03:50:47Z

    On Tue, Feb 11, 2025 at 08:57:46PM -0600, Sami Imseih wrote:
> > Of course some people may want to keep the current behavior, if they have
> > limited number of temp tables or similar, so I had a GUC for that.  I don't
> > think that the community would really welcome such GUC for core-postgres,
> > especially since it wouldn't be pg_stat_statements specific.
>
> FWIW, I think options to tweak queryId computation is something
> that should be in core. It was discussed earlier in the context
> of IN list merging; the patch for this currently has the guc
> for the feature in pg_stat_statements, but there was a discussion
> about actually moving this to core [1] Giving the user a way
> to control certain behavior about the queryId computation
> is a good thing to do in core; especially queryId is no longer
> just consumed in pg_stat_statements. Maybe the right answer
> is an enum GUC, not sure yet.
>
> Specifically for the use-case you mention, using names vs OIDs in
> queryId computation is a valid use case for more than temporary tables,
> I can also think of upgrade, dump/restore, logical replication cases which
> can then allow for a consistent queryId.

Well, the ability for extensions to override the actual queryid calculation was
the result of more than half a decade of strong disagreements about it.   And
I'm definitely not volunteering to reopen that topic :)

  32. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-12T12:57:47Z

    On 2025-Feb-12, Julien Rouhaud wrote:

> > FWIW, I think options to tweak queryId computation is something that
> > should be in core. It was discussed earlier in the context of IN
> > list merging; the patch for this currently has the guc for the
> > feature in pg_stat_statements, but there was a discussion about
> > actually moving this to core [1] Giving the user a way to control
> > certain behavior about the queryId computation is a good thing to do
> > in core; especially queryId is no longer just consumed in
> > pg_stat_statements. Maybe the right answer is an enum GUC, not sure
> > yet.

> Well, the ability for extensions to override the actual queryid
> calculation was the result of more than half a decade of strong
> disagreements about it.   And I'm definitely not volunteering to
> reopen that topic :)

Sorry, Michael already did.

Anyway, I think that's different.  We do support compute_query_id=off as
a way for a custom module to compute completely different query IDs
using their own algorithm, which I think is what you're referring to.
However, the ability to affect the way the in-core algorithm works is a
different thing: you still want in-core code to compute the query ID.

Right now, the proposal in the other thread is that if you want to
affect that algorithm in order to merge arrays to be considered a single
query element regardless of its length, you set the GUC for that.
Initially the GUC was in the core code.  Then, based on review, the GUC
was moved to the extension, _BUT_ the implementation was still in the
core code: in order to activate it, the extension calls a function that
modifies core code behavior.  So there are more moving parts than
before, and if you for whatever reason want that behavior but not the
extension, then you need to write a C function.  To me this is absurd.
So what I suggest we do is return to having the GUC in the core code.

Now I admit I'm not sure what the solution would be for the problem
discussed in this subthread.  Apparently the problem is related to temp
tables and their changing OIDs.  I'm not sure what exactly the proposal
for a GUC is.  I mean, what would the behavior change be?  Maybe what
you want is something like "if this table reference here is to a temp
table, then instead of jumbling the OID then jumble the string
'pg_temp.tablename' instead", which would make the query ID be the same
for all occurrences of that query in whatever backend return the same
number, regardless both of what OID the temp schema for that backend is,
and the table OID itself.  Is there more to it than that?  (The only
difficulty I see here is how to get the table name when the only thing
you have is the RangeTblEntry, which doesn't have the name but just the
OID.  I see in [1] that you simply do a syscache lookup, but it would be
good to avoid that.)

Maybe that sounds pretty obscure if you try to describe it too
precisely, but if you don't think too hard about it it probably natural
-- at least to me.  So my next question is, do we really need this
behavior to be configurable?  Wouldn't it be better to make the default
way to deal with temp tables in all cases?  The current behavior seems
rather unhelpful.  I do note that what you do in pg_queryid, which is
simply to ignore the table altogether, is probably not a great idea.


Anyway, assuming we make a GUC of it (a big if!), let me talk a bit
about GUC names.  In the other thread, the list of GUC names in the
submitted patch plus the ones I suggested are:

query_id_const_merge
query_id_merge_values
query_id_merge_value_lists
query_id_squash_constant_lists

so maybe here I would consider something like

query_id_merge_temp_tables
query_id_squash_temporary_tables

[1] https://github.com/rjuju/pg_queryid/blob/master/pg_queryid.c#L941

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The important things in the world are problems with society that we don't
understand at all. The machines will become more complicated but they won't
be more complicated than the societies that run them."    (Freeman Dyson)

  33. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Julien Rouhaud <rjuju123@gmail.com> — 2025-02-12T13:46:06Z

    On Wed, Feb 12, 2025 at 01:57:47PM +0100, Alvaro Herrera wrote:
> On 2025-Feb-12, Julien Rouhaud wrote:
>
> > > FWIW, I think options to tweak queryId computation is something that
> > > should be in core. It was discussed earlier in the context of IN
> > > list merging; the patch for this currently has the guc for the
> > > feature in pg_stat_statements, but there was a discussion about
> > > actually moving this to core [1] Giving the user a way to control
> > > certain behavior about the queryId computation is a good thing to do
> > > in core; especially queryId is no longer just consumed in
> > > pg_stat_statements. Maybe the right answer is an enum GUC, not sure
> > > yet.
>
> > Well, the ability for extensions to override the actual queryid
> > calculation was the result of more than half a decade of strong
> > disagreements about it.   And I'm definitely not volunteering to
> > reopen that topic :)
>
> Anyway, I think that's different.  We do support compute_query_id=off as
> a way for a custom module to compute completely different query IDs
> using their own algorithm, which I think is what you're referring to.
> However, the ability to affect the way the in-core algorithm works is a
> different thing: you still want in-core code to compute the query ID.

I don't think that's the actual behavior, or at least not what it was supposed
to be.

What we should have is the ability to compute queryid, which can be either in
core or done by an external module, but one only one can / should be done.  And
then you have stuff that use that queryid, e.g. pg_stat_statements,
pg_stat_activity and whatnot, no matter what generated it.  That's per the
original commit 5fd9dfa5f50e message:

Add compute_query_id GUC to control whether a query identifier should be
computed by the core (off by default).  It's thefore now possible to
disable core queryid computation and use pg_stat_statements with a
different algorithm to compute the query identifier by using a
third-party module.

To ensure that a single source of query identifier can be used and is
well defined, modules that calculate a query identifier should throw an
error if compute_query_id specified to compute a query id and if a query
idenfitier was already calculated.

> Right now, the proposal in the other thread is that if you want to
> affect that algorithm in order to merge arrays to be considered a single
> query element regardless of its length, you set the GUC for that.
> Initially the GUC was in the core code.  Then, based on review, the GUC
> was moved to the extension, _BUT_ the implementation was still in the
> core code: in order to activate it, the extension calls a function that
> modifies core code behavior.  So there are more moving parts than
> before, and if you for whatever reason want that behavior but not the
> extension, then you need to write a C function.  To me this is absurd.
> So what I suggest we do is return to having the GUC in the core code.

I agree, although that probably breaks the queryid extensibility.  I haven't
read the patch but IIUC if you want the feature to work you need to both change
the queryid calculation but also the way the constants are recorded and the
query text is normalized, and I don't know if extensions have access to it.  If
they have access and fail to do what the GUC asked then of course that's just a
bug in that extension.

> Now I admit I'm not sure what the solution would be for the problem
> discussed in this subthread.  Apparently the problem is related to temp
> tables and their changing OIDs.  I'm not sure what exactly the proposal
> for a GUC is.

I'm not proposing anything, just explaining why pg_stat_statements is generally
useless if you use temp tables as someone asked.

> I do note that what you do in pg_queryid, which is
> simply to ignore the table altogether, is probably not a great idea.

Yeah, that's also why I said in my previous message that using its name for the
queryid would be better.  Note that in pg_queryid it's already possible to use
relation names rather than oid for the queryid (which I wouldn't recommend, but
it's good for testing).  I just never implemented it for a temp-only
granularity.

  34. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-12T16:00:19Z

    On 2025-Feb-12, Julien Rouhaud wrote:

> On Wed, Feb 12, 2025 at 01:57:47PM +0100, Alvaro Herrera wrote:

> > Anyway, I think that's different.  We do support compute_query_id=off as
> > a way for a custom module to compute completely different query IDs
> > using their own algorithm, which I think is what you're referring to.
> > However, the ability to affect the way the in-core algorithm works is a
> > different thing: you still want in-core code to compute the query ID.
> 
> I don't think that's the actual behavior, or at least not what it was
> supposed to be.
> 
> What we should have is the ability to compute queryid, which can be
> either in core or done by an external module, but one only one can /
> should be done.

Yes, that's what I tried to say, but I don't understand why you say I
said something different.

> > Right now, the proposal in the other thread is that if you want to
> > affect that algorithm in order to merge arrays to be considered a single
> > query element regardless of its length, you set the GUC for that.
> > Initially the GUC was in the core code.  Then, based on review, the GUC
> > was moved to the extension, _BUT_ the implementation was still in the
> > core code: in order to activate it, the extension calls a function that
> > modifies core code behavior.  So there are more moving parts than
> > before, and if you for whatever reason want that behavior but not the
> > extension, then you need to write a C function.  To me this is absurd.
> > So what I suggest we do is return to having the GUC in the core code.
> 
> I agree, although that probably breaks the queryid extensibility.

It does?

> I haven't read the patch but IIUC if you want the feature to work you
> need to both change the queryid calculation but also the way the
> constants are recorded and the query text is normalized, and I don't
> know if extensions have access to it.

Hmm.  As for the query text: with Andrey's feature with the GUC in core,
a query like this
SELECT foo FROM tab WHERE col1 IN (1,2,3,4)
will have in pg_stat_activity an identical query_id to a query like this
SELECT foo WHERE tab WHERE col1 IN (1,2,3,4,5)
even though the query texts differ (in the number of elements in the
array).  I don't think this is a problem.  This means that the query_id
for two different queries can be identical, but that should be no
surprise, precisely because the GUC that controls it is documented to do
that.

If pg_stat_statements is enabled with Andrey's patch, then the same
query_id will have a single entry (which has stats for both execution of
those queries) with that query_id, with a normalized query text that is
going to be different from those two above; without Andrey's feature,
the text would be 
SELECT foo WHERE tab WHERE col1 IN ($1,$2,$3,$4);
SELECT foo WHERE tab WHERE col1 IN ($1,$2,$3,$4,$5);
(that is, pg_stat_statements transformed the values into placeholders,
but using exactly the same number of items in the array as the original
queries).  With Andrey's feature, it will be 
SELECT foo WHERE tab WHERE col1 IN (...);
that is, the query text has been modified and no longer matches exactly
any of the queries in pg_stat_activity.  But note that the query text
already does not match what's in pg_stat_activity, even before Andrey's
patch.

I don't understand what you mean with "the way the constants are
recorded".  What constants are you talking about?  pg_stat_statements
purposefully discards any constants used in the query (obviously).

> If they have access and fail to do what the GUC asked then of course
> that's just a bug in that extension.

I don't understand what bug are you thinking that such hypothetical
extension would have.  (pg_stat_statements does of course have access to
the query text and to the location of all constants).

> > Now I admit I'm not sure what the solution would be for the problem
> > discussed in this subthread.  Apparently the problem is related to temp
> > tables and their changing OIDs.  I'm not sure what exactly the proposal
> > for a GUC is.
> 
> I'm not proposing anything, just explaining why pg_stat_statements is
> generally useless if you use temp tables as someone asked.

Ah, okay.  Well, where you see a deficiency, I see an opportunity for
improvement :-)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  35. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Sami Imseih <samimseih@gmail.com> — 2025-02-13T02:50:08Z

    >> On Mon, Feb 10, 2025 at 02:02:10PM -0600, Sami Imseih wrote:
>> > I am OK with moving away from "jumble" in-lieu of something else, but my thoughts are we should actually call this process "fingerprint"
>
>
> I agree fingerprint is the right final word. But "jumble" conveys the *process* better than "fingerprinting".
> I view it as jumbling produces an object that can be fingerprinted.

hmm, "jumble" describes something that is scrambled
or not in order, such as the 64-bit hash produced. It
sounds like the final product.

Fingerprinting on the other hand [1] sounds more of the process
to add all the pieces that will eventually be hashed ( or jumbled ).
hash and jumble are synonyms according to Merriam-Webster [2]

--

Sami

[1] https://en.wikipedia.org/wiki/Fingerprint_(computing)
[2] https://www.merriam-webster.com/thesaurus/hash

  36. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-13T11:25:25Z

    On 2025-Feb-12, Sami Imseih wrote:

> Greg S. Mullane wrote:
>
> > I agree fingerprint is the right final word. But "jumble" conveys
> > the *process* better than "fingerprinting".  I view it as jumbling
> > produces an object that can be fingerprinted.
> 
> hmm, "jumble" describes something that is scrambled
> or not in order, such as the 64-bit hash produced. It
> sounds like the final product.

I don't understand why we would change any naming here at all.  I think
you should be looking at a much broader consensus and plus-ones that a
renaming is needed.  -1 from me.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No renuncies a nada. No te aferres a nada."

  37. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Sami Imseih <samimseih@gmail.com> — 2025-02-13T16:44:33Z

    > I don't understand why we would change any naming here at all.  I think
> you should be looking at a much broader consensus and plus-ones that a
> renaming is needed.  -1 from me.

The reason for the change is because "query jumble" will no longer
make sense if the jumble code can now be used for other types of
trees, such as Plan.

I do agree that this needs a single-threaded discussion to achieve a
consensus.

--

Sami

  38. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-03-18T07:31:06Z

    On Thu, Feb 13, 2025 at 10:44:33AM -0600, Sami Imseih wrote:
> The reason for the change is because "query jumble" will no longer
> make sense if the jumble code can now be used for other types of
> trees, such as Plan.
> 
> I do agree that this needs a single-threaded discussion to achieve a
> consensus.

FWIW, I was playing with a sub-project where I was jumbling a portion
of nodes other than Query, and it is annoying to not have a direct
access to jumbleNode().  So, how about doing the refactoring proposed
in v5-0002 with an initialization routine and JumbleNode() as the
entry point for the jumbling, but not rename the existing files 
queryjumblefuncs.c and queryjumble.h?  That seems doable for this
release, at least.

I don't think that we should expose AppendJumble(), either.
--
Michael

  39. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Andrei Lepikhov <lepihov@gmail.com> — 2025-03-18T09:12:10Z

    On 3/18/25 08:31, Michael Paquier wrote:
> On Thu, Feb 13, 2025 at 10:44:33AM -0600, Sami Imseih wrote:
>> The reason for the change is because "query jumble" will no longer
>> make sense if the jumble code can now be used for other types of
>> trees, such as Plan.
>>
>> I do agree that this needs a single-threaded discussion to achieve a
>> consensus.
> 
> FWIW, I was playing with a sub-project where I was jumbling a portion
> of nodes other than Query, and it is annoying to not have a direct
> access to jumbleNode().  So, how about doing the refactoring proposed
> in v5-0002 with an initialization routine and JumbleNode() as the
> entry point for the jumbling, but not rename the existing files
> queryjumblefuncs.c and queryjumble.h?  That seems doable for this
> release, at least.
It seems pretty helpful to me. Having a code for hashing an expression 
or subquery, we may design new optimisations. I personally have such a 
necessity in a couple of planner extensions.

At the same time, generalising jumbling code we may decide to work on 
the JumbleState structure: code related to constant locations may be 
replaced with callbacks - let the caller decide what action to take on 
each node (not only constants). Of course, it is not for current release.

> 
> I don't think that we should expose AppendJumble(), either.
Agree


-- 
regards, Andrei Lepikhov

  40. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Ilia Evdokimov <ilya.evdokimov@tantorlabs.com> — 2025-03-18T19:27:47Z

    On 12.02.2025 19:00, Alvaro Herrera wrote:
> On 2025-Feb-12, Julien Rouhaud wrote:
>
>> On Wed, Feb 12, 2025 at 01:57:47PM +0100, Alvaro Herrera wrote:
>>> Anyway, I think that's different.  We do support compute_query_id=off as
>>> a way for a custom module to compute completely different query IDs
>>> using their own algorithm, which I think is what you're referring to.
>>> However, the ability to affect the way the in-core algorithm works is a
>>> different thing: you still want in-core code to compute the query ID.
>> I don't think that's the actual behavior, or at least not what it was
>> supposed to be.
>>
>> What we should have is the ability to compute queryid, which can be
>> either in core or done by an external module, but one only one can /
>> should be done.
> Yes, that's what I tried to say, but I don't understand why you say I
> said something different.
>
>>> Right now, the proposal in the other thread is that if you want to
>>> affect that algorithm in order to merge arrays to be considered a single
>>> query element regardless of its length, you set the GUC for that.
>>> Initially the GUC was in the core code.  Then, based on review, the GUC
>>> was moved to the extension, _BUT_ the implementation was still in the
>>> core code: in order to activate it, the extension calls a function that
>>> modifies core code behavior.  So there are more moving parts than
>>> before, and if you for whatever reason want that behavior but not the
>>> extension, then you need to write a C function.  To me this is absurd.
>>> So what I suggest we do is return to having the GUC in the core code.
>> I agree, although that probably breaks the queryid extensibility.
> It does?
>
>> I haven't read the patch but IIUC if you want the feature to work you
>> need to both change the queryid calculation but also the way the
>> constants are recorded and the query text is normalized, and I don't
>> know if extensions have access to it.
> Hmm.  As for the query text: with Andrey's feature with the GUC in core,
> a query like this
> SELECT foo FROM tab WHERE col1 IN (1,2,3,4)
> will have in pg_stat_activity an identical query_id to a query like this
> SELECT foo WHERE tab WHERE col1 IN (1,2,3,4,5)
> even though the query texts differ (in the number of elements in the
> array).  I don't think this is a problem.  This means that the query_id
> for two different queries can be identical, but that should be no
> surprise, precisely because the GUC that controls it is documented to do
> that.
>
> If pg_stat_statements is enabled with Andrey's patch, then the same
> query_id will have a single entry (which has stats for both execution of
> those queries) with that query_id, with a normalized query text that is
> going to be different from those two above; without Andrey's feature,
> the text would be
> SELECT foo WHERE tab WHERE col1 IN ($1,$2,$3,$4);
> SELECT foo WHERE tab WHERE col1 IN ($1,$2,$3,$4,$5);
> (that is, pg_stat_statements transformed the values into placeholders,
> but using exactly the same number of items in the array as the original
> queries).  With Andrey's feature, it will be
> SELECT foo WHERE tab WHERE col1 IN (...);
> that is, the query text has been modified and no longer matches exactly
> any of the queries in pg_stat_activity.  But note that the query text
> already does not match what's in pg_stat_activity, even before Andrey's
> patch.
>
> I don't understand what you mean with "the way the constants are
> recorded".  What constants are you talking about?  pg_stat_statements
> purposefully discards any constants used in the query (obviously).
>
>> If they have access and fail to do what the GUC asked then of course
>> that's just a bug in that extension.
> I don't understand what bug are you thinking that such hypothetical
> extension would have.  (pg_stat_statements does of course have access to
> the query text and to the location of all constants).
>
>>> Now I admit I'm not sure what the solution would be for the problem
>>> discussed in this subthread.  Apparently the problem is related to temp
>>> tables and their changing OIDs.  I'm not sure what exactly the proposal
>>> for a GUC is.
>> I'm not proposing anything, just explaining why pg_stat_statements is
>> generally useless if you use temp tables as someone asked.
> Ah, okay.  Well, where you see a deficiency, I see an opportunity for
> improvement :-)
>

Hi everyone,

I support the idea of computing the planid  for temporary tables using 
'pg_temp.rel_name'. Moreover, we have already started using this 
approach for computing queryid [0]. It seems reasonable to apply the 
same logic to the planid calculation as well.

[0]: 
https://www.postgresql.org/message-id/flat/Z9mkqplmUpQ4xG52%40msg.df7cb.de#efb20f01bec32aeafd58e5d4ab0dfc16

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.

  41. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Андрей Казачков <andrey.kazachkov@tantorlabs.ru> — 2025-12-25T14:27:19Z

      
    Dear PostgreSQL Hackers,
  
  
     
  
  
    I’d like to propose a follow-up fix for a crash caused by jumbling of empty
  
  
    array fields in Plan structures that was introduced in the v5-0003 patch.
  
  
     
  
  
    For your convenience, I attached a minimal reproducible example:
  
  
    ```
  
  
    create table foo as select i as num from generate_series(1, 1000) i;
  
  
    set compute_plan_id to true;
  
  
     
  
  
    -- server closed the connection unexpectedly
  
  
    explain (costs off, verbose)
  
  
    select min(num) from foo;
  
  
    ```
  
  
    In this case the scalar aggregate operator `min(num)` is expressed as an Agg
  
  
    node with an empty array of GROUP BY attributes. The jumble logic can’t handle
  
  
    emptiness and terminates the backend. Similar behavior could happen during
  
  
    jumbling an arbitrary plan node which contains an array.
  
  
     
  
  
    Patchset overview:
  
  
    v6-0001
  
  
    -----------
  
  
    Aggregates changes from v5-0001..v5-0003 and rebases onto master
  
  
    commit b39013b7b1b116b5d9be51f0919b472b58b3a28d.
  
  
     
  
  
    v6-0002
  
  
    -----------
  
  
    Fixes the lack of empty-array handling during jumbling (originally applies on
  
  
    top of v6-0000). For anyone testing on the v5 versions, v6-0001 can be easily
  
  
    applied on top of v5-0003.
  
  
     
  
  
    --
  
  
    Sincerely, Andrey Kazachkov

  42. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Андрей Казачков <andrey.kazachkov@tantorlabs.ru> — 2025-12-25T14:33:11Z

      
    Dear PostgreSQL Hackers,
  
  
     
  
  
    I’ve been testing the proposed v5 plan id work and found out instability of
  
  
    computing the plan identifier after feeding different query texts that
  
  
    produces the same physical plan trees but with different plan ids. The main
  
  
    pattern regards with fields of Plan node structures that depend on positions of
  
  
    RTEs in a RTE list.
  
  
     
  
  
    For your convenience, I’ve attached minimal reproducible examples that
  
  
    demonstrate this pattern. Both are based on the v6-0001 patch, which
  
  
    incorporates v5-0001..v5-0003 changes and fixes jumbling empty arrays.
  
  
     
  
  
    ```
  
  
    create table foo1 (num int);
  
  
    create table foo2 (num int);
  
  
     
  
  
    insert into foo1 (num)
  
  
    select *
  
  
    from generate_series(1, 1000);
  
  
     
  
  
    insert into foo2 (num)
  
  
    select *
  
  
    from generate_series(1, 10);
  
  
     
  
  
    analyze foo1;
  
  
    analyze foo2;
  
  
     
  
  
    set compute_plan_id to true;
  
  
    ```
  
  
     
  
  
    The first example changes join order between two tables:
  
  
    ```
  
  
    explain (costs off, verbose)
  
  
    select 1 from foo1 join foo2 on foo1.num = foo2.num;
  
  
     
  
  
                QUERY PLAN
  
  
    -------------------------------------
  
  
     Hash Join
  
  
       Output: 1
  
  
       Hash Cond: (foo1.num = foo2.num)
  
  
       ->  Seq Scan on public.foo1
  
  
             Output: foo1.num
  
  
       ->  Hash
  
  
             Output: foo2.num
  
  
             ->  Seq Scan on public.foo2
  
  
                   Output: foo2.num
  
  
     Plan Identifier: 538643160186222168
  
  
     
  
  
    explain (costs off, verbose)
  
  
    select 1 from foo2 join foo1 on foo1.num = foo2.num;
  
  
     
  
  
                 QUERY PLAN
  
  
    --------------------------------------
  
  
     Hash Join
  
  
       Output: 1
  
  
       Hash Cond: (foo1.num = foo2.num)
  
  
       ->  Seq Scan on public.foo1
  
  
             Output: foo1.num
  
  
       ->  Hash
  
  
             Output: foo2.num
  
  
             ->  Seq Scan on public.foo2
  
  
                   Output: foo2.num
  
  
     Plan Identifier: -953143034841089498
  
  
    ```
  
  
    Here the reordering of relations in the JOIN operator changes their order in
  
  
    the RTE list, which in turn changes RTE position-sensitive fields like `varno`
  
  
    in Vars, and those differences leak into jumbling.
  
  
     
  
  
    The second example assumes transformation of some table expressions to a
  
  
    new form like CTE-inlining or completely its eliminating. Let's see example:
  
  
    ```
  
  
    explain (costs off, verbose)
  
  
    WITH foo_cte as (
  
  
      SELECT num FROM foo1)
  
  
    select * from foo_cte;
  
  
     
  
  
                  QUERY PLAN
  
  
    --------------------------------------
  
  
     Seq Scan on public.foo1
  
  
       Output: foo1.num
  
  
     Plan Identifier: 3494394630757173099
  
  
     
  
  
    explain (costs off, verbose)
  
  
    select * from foo1;
  
  
     
  
  
                  QUERY PLAN
  
  
    --------------------------------------
  
  
     Seq Scan on public.foo1
  
  
       Output: num
  
  
     Plan Identifier: 8116143677260771228
  
  
    ```
  
  
    In the example with a CTE, RTE list contains the outdated subquery item and the
  
  
    relation one, whereas without a CTE we have just the relation item. Although
  
  
    the CTE is inlined, its RTE is not removed from the rtable; as a result,
  
  
    position-sensitive fields (such as `varno` for Vars) differ from the no-CTE
  
  
    case.
  
  
     
  
  
    A possible way to address it during jumbling process:
  
  
    1. Remove/skip unused RTE list elements.
  
  
    2. Sort active RTE list elements by some stable criteria.
  
  
    3. Adjust fields referring to active RTEs.
  
  
     
  
  
    But the main challenge is identifying all “position-sensitive” fields across
  
  
    node types efficiently and maintainably. I’d happy to see your feedback on
  
  
    this issue.
  
  
     
  
  
    Additionally, I noticed that the `location` field is being jumbled for several
  
  
    structures (for example, `Var`). Since it’s only a token location, I believe we
  
  
    should not include it in the final plan id value.
  
  
     
  
  
    --
  
  
    Sincerely,
  
  
    Andrey Kazachkov

  43. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Michael Paquier <michael@paquier.xyz> — 2025-12-25T23:01:41Z

    On Thu, Dec 25, 2025 at 05:33:11PM +0300, Андрей Казачков wrote:
> I’ve been testing the proposed v5 plan id work and found out instability of
> computing the plan identifier after feeding different query texts that
> produces the same physical plan trees but with different plan ids. The main
> pattern regards with fields of Plan node structures that depend on positions of
> RTEs in a RTE list.

FWIW, I don't think that we have a clear agreement about what would be
a good enough ID for plan trees, as it may be also a per-vendor
computation that fills specific user requirements.

+    /*
+     * COMPUTE_PLAN_ID_REGRESS means COMPUTE_PLAN_ID_YES, but we don't show
+     * the queryid in any of the EXPLAIN plans to keep stable the results
+     * generated by regression test suites.
+     */
+    if (es->verbose && queryDesc->plannedstmt->planId != UINT64CONST(0) &&
+        compute_plan_id != COMPUTE_PLAN_ID_REGRESS)
+    {
+        /*
+         * Output the queryid as an int64 rather than a uint64 so we match
+         * what would be seen in the BIGINT pg_stat_activity.plan_id column.
+         */
+        ExplainPropertyInteger("Plan Identifier", NULL,
+                               queryDesc->plannedstmt->planId, es);
+    }

Now, looking at this block of code, I am wondering if you don't have a
point here even without compute_plan_id..  Could there be merit in
showing this information for an EXPLAIN if this field is not zero?
With EXPLAIN being pluggable in a hook, I doubt that it matters much,
but I am wondering if providing this information could make the work
of some extensions easier.
--
Michael

  44. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Lukas Fittl <lukas@fittl.com> — 2026-03-19T07:15:07Z

    On Thu, Dec 25, 2025 at 3:02 PM Michael Paquier <michael@paquier.xyz> wrote:
> +    /*
> +     * COMPUTE_PLAN_ID_REGRESS means COMPUTE_PLAN_ID_YES, but we don't show
> +     * the queryid in any of the EXPLAIN plans to keep stable the results
> +     * generated by regression test suites.
> +     */
> +    if (es->verbose && queryDesc->plannedstmt->planId != UINT64CONST(0) &&
> +        compute_plan_id != COMPUTE_PLAN_ID_REGRESS)
> +    {
> +        /*
> +         * Output the queryid as an int64 rather than a uint64 so we match
> +         * what would be seen in the BIGINT pg_stat_activity.plan_id column.
> +         */
> +        ExplainPropertyInteger("Plan Identifier", NULL,
> +                               queryDesc->plannedstmt->planId, es);
> +    }
>
> Now, looking at this block of code, I am wondering if you don't have a
> point here even without compute_plan_id..  Could there be merit in
> showing this information for an EXPLAIN if this field is not zero?
> With EXPLAIN being pluggable in a hook, I doubt that it matters much,
> but I am wondering if providing this information could make the work
> of some extensions easier.

I missed this at the time, but happened to run across this by coincidence.

Consider this a late +1 on the idea, i.e. I do think that emitting the
plan ID as "plan identifier" in EXPLAIN seems reasonable when a plugin
sets it - the cost is negligible, and it'd make it easier to work with
extensions like pg_stat_plans.


Thanks,
Lukas

--
Lukas Fittl

  45. Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

    Sami Imseih <samimseih@gmail.com> — 2026-03-19T15:13:50Z

    > I missed this at the time, but happened to run across this by coincidence.
>
> Consider this a late +1 on the idea, i.e. I do think that emitting the
> plan ID as "plan identifier" in EXPLAIN seems reasonable when a plugin
> sets it - the cost is negligible, and it'd make it easier to work with
> extensions like pg_stat_plans.

FWIW, the per plan hooks introduced in 4fd02bf7cf94c can allow an extension
to emit the plan identifier in EXPLAIN as well.

--
Sami Imseih
Amazon Web Services (AWS)