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

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

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

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-thre

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

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 > con

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

> 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

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

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 i

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

>> 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

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

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

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

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 currentl

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

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

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

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, >

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

> 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,

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

On Tue, Feb 11, 2025 at 7:08 PM Michael Paquier 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

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

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

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

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 anybod

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

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 th

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

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 th

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

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

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

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 signature.asc Descrip

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

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

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

> 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 comm

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

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. > D

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

On 2/5/25 09:16, Lukas Fittl wrote: Hi Andrei, On Fri, Jan 24, 2025 at 1:23 AM Andrei Lepikhov > 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 se

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

Hi Andrei, On Fri, Jan 24, 2025 at 1:23 AM Andrei Lepikhov 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

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

>> 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

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

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 pars

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

>> 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 top

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

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 > whethe

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

On Thu, Jan 30, 2025 at 8:37 PM Michael Paquier 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 h

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

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 t

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

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 attenti

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

On Tue, Jan 21, 2025 at 10:47 AM Artem Gavrilov 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 ano

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

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

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

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

On Thu, Jan 2, 2025 at 10:47 PM Lukas Fittl 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 stati

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

On Thu, 2 Jan 2025 12:46:04 -0800 Lukas Fittl 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