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

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(at)paquier(dot)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(at)gmail(dot)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