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

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)