On 7/30/25 21:05, Sami Imseih wrote:
The term "NOT_SET" makes me itch a little bit, even if there is an
existing parallel with OverridingKind. Perhaps your proposal is OK,
still how about "UNKNOWN" instead to use as term for the default?
+1 to "UNKNOWN".
We currently use both UNKNOWN and NOT_
> > The term "NOT_SET" makes me itch a little bit, even if there is an
> > existing parallel with OverridingKind. Perhaps your proposal is OK,
> > still how about "UNKNOWN" instead to use as term for the default?
> +1 to "UNKNOWN".
We currently use both UNKNOWN and NOT_SET in different places.
Ho
On 30/7/2025 09:20, Michael Paquier wrote:
On Tue, Jul 29, 2025 at 05:08:09PM -0500, Sami Imseih wrote:
The only comment I have is I think we need a NOT_SET
member, so it can simplify the life of extensions that have code
paths which may or may not have a PlannedStmt, such as
pgss_store.
Okay
On Tue, Jul 29, 2025 at 05:08:09PM -0500, Sami Imseih wrote:
> The only comment I have is I think we need a NOT_SET
> member, so it can simplify the life of extensions that have code
> paths which may or may not have a PlannedStmt, such as
> pgss_store.
Okay by me for having a default that maps to
> > Attached is my counter-proposal, where I have settled down to four
> > categories of PlannedStmt:
> > - "standard" PlannedStmt, when going through the planner.
> > - internally-generated "fake" PlannedStmt.
> > - custom cache
> > - generic cache
>
> Thanks for the update! I plan on reviewing th
> Attached is my counter-proposal, where I have settled down to four
> categories of PlannedStmt:
> - "standard" PlannedStmt, when going through the planner.
> - internally-generated "fake" PlannedStmt.
> - custom cache
> - generic cache
Thanks for the update! I plan on reviewing this tomorrow.
-
> The current pg_stat_statements change may be a
> good example of the employment of such infrastructure, isn't it?
So, the current set of patches now will help move forward the specific
use-case of this thread. If we need something different that will be
useful for more use-cases, perhaps it's be
On 28/7/2025 08:46, Michael Paquier wrote:
On Mon, Jul 28, 2025 at 08:41:29AM +0200, Andrei Lepikhov wrote:
It looks good, but doesn't it seem too narrow?
For the use case of the thread which is to count the number of custom
vs generic plans, it would be good enough.
Sure, no objections.
T
On Mon, Jul 28, 2025 at 08:41:29AM +0200, Andrei Lepikhov wrote:
> It looks good, but doesn't it seem too narrow?
For the use case of the thread which is to count the number of custom
vs generic plans, it would be good enough.
> This minor change allows an extension to track a specific query from
On 28/7/2025 08:18, Michael Paquier wrote:
> We could decide if a few more of the internal "fake" ones are worth
> subdividing, but I am feeling that this is kind of OK to start with.
> If we want more granularity, I was wondering about some bits to be
> able to mix one or more of these categories,
On Fri, Jul 25, 2025 at 02:34:07PM -0500, Sami Imseih wrote:
> Sami Imseih writes:
>> I think Michael's got a point. As of HEAD there are seven different
>> places that are setting this to PLAN_CACHE_NONE; who's to say that
>> pg_stat_statements or some other extension might not wish to
>> distin
> Sami Imseih writes:
> >> Perhaps CachedPlanType is
> >> misnamed, though, would it be more suited to name that as a sort of
> >> "origin" or "source" field concept? We want to know which which
> >> source we have retrieved a plan that a PlannedStmt refers to.
>
> > Hmm, I’m not sure I see this
Sami Imseih writes:
>> Perhaps CachedPlanType is
>> misnamed, though, would it be more suited to name that as a sort of
>> "origin" or "source" field concept? We want to know which which
>> source we have retrieved a plan that a PlannedStmt refers to.
> Hmm, I’m not sure I see this as an improve
> Perhaps CachedPlanType is
> misnamed, though, would it be more suited to name that as a sort of
> "origin" or "source" field concept? We want to know which which
> source we have retrieved a plan that a PlannedStmt refers to.
Hmm, I’m not sure I see this as an improvement. In my opinion,
Cached
On 24/7/2025 17:05, Tom Lane wrote:
> Andrei Lepikhov writes:
>> I see you have chosen a variant with a new enum instead of a pointer to
>> a plan cache entry. I wonder if you could write the arguments
>> supporting this choice?
>
> Pointing to a plan cache entry would often mean that the data
> s
On Thu, Jul 24, 2025 at 01:14:47PM -0500, Sami Imseih wrote:
>> Sami Imseih writes:
That is not to say that I think 719dcf3c4 was a good idea: it looks
rather useless from here. It seems to me that the right place to
accumulate these sorts of stats is in CachedPlanSources, and I do
> One option might be to use a local hash table, keyed the same way as the
> shared pgss hash (excluding dbid), to handle cases where a backend has
> more than one active cached plan. Then at ExecutorEnd, the local entry could
> be looked up and passed to pgss_store. Not sure if this is worth the e
> Sami Imseih writes:
> >> That is not to say that I think 719dcf3c4 was a good idea: it looks
> >> rather useless from here. It seems to me that the right place to
> >> accumulate these sorts of stats is in CachedPlanSources, and I don't
> >> see how this helps. What likely *would* help is some
Sami Imseih writes:
>> That is not to say that I think 719dcf3c4 was a good idea: it looks
>> rather useless from here. It seems to me that the right place to
>> accumulate these sorts of stats is in CachedPlanSources, and I don't
>> see how this helps. What likely *would* help is some hooks in
> Andrei Lepikhov writes:
> > I see you have chosen a variant with a new enum instead of a pointer to
> > a plan cache entry. I wonder if you could write the arguments
> > supporting this choice?
>
> Pointing to a plan cache entry would often mean that the data
> structure as a whole is circular (
Andrei Lepikhov writes:
> I see you have chosen a variant with a new enum instead of a pointer to
> a plan cache entry. I wonder if you could write the arguments
> supporting this choice?
Pointing to a plan cache entry would often mean that the data
structure as a whole is circular (since a plan
On 24/7/2025 09:03, Michael Paquier wrote:
> On Wed, Jul 23, 2025 at 12:15:06PM +0900, Michael Paquier wrote:
>> A small thing that would be cleaner is to split the patch into two
>> parts: one for the in-core backend addition and a second for PGSS.
>> Code paths are different, so it's simple to do
On Wed, Jul 23, 2025 at 12:15:06PM +0900, Michael Paquier wrote:
> A small thing that would be cleaner is to split the patch into two
> parts: one for the in-core backend addition and a second for PGSS.
> Code paths are different, so it's simple to do.
I have been looking at the backend part of th
On Tue, Jul 22, 2025 at 02:52:49PM -0500, Sami Imseih wrote:
>> We will need a field to store an enum. let's call it CachedPlanType
>> with the types of cached plan. We need to be able to differentiate
>> when cached plans are not used, so a simple boolean is not
>> sufficient.
Guess so.
>> We ca
> > I know Michael opposed the idea of carrying these structures,
> > at least CachedPlan, to Executor hooks ( or maybe just not QueryDesc?? ).
> > It will be good to see what he think, or if others an opinion about this,
> > about
> > adding a pointer to CachedPlanSource in PlannedStmt vs setting
On Tue, Jul 22, 2025 at 03:28:24PM -0500, Sami Imseih wrote:
> I know Michael opposed the idea of carrying these structures,
> at least CachedPlan, to Executor hooks ( or maybe just not QueryDesc?? ).
> It will be good to see what he think, or if others an opinion about this,
> about
> adding a po
> > with the types of cached plan. We need to be able to differentiate
> > when cached plans are not used, so a simple boolean is not
> > sufficient.
> Sure. But I modestly hope you would add a CachedPlanSource pointer
> solely to the PlannedStmt and restructure it a little as we discussed
> above.
On 7/22/25 19:13, Sami Imseih wrote:
It may be more efficient to set the is_generic_plan option at the top
plan node (PlannedStmt) and reference it wherever necessary. To identify
a cached plan, we may consider pushing the CachedPlan/CachedPlanSource
pointer down throughout pg_plan_query and main
> > It may be more efficient to set the is_generic_plan option at the top
> > plan node (PlannedStmt) and reference it wherever necessary. To identify
> > a cached plan, we may consider pushing the CachedPlan/CachedPlanSource
> > pointer down throughout pg_plan_query and maintaining a reference to
> I would like to oppose to the current version a little.
>
> Commits de3a2ea3b264 and 1d477a907e63 introduced elements that are more
> closely related to the execution phase. While the parameter
> es_parallel_workers_to_launch could be considered a planning parameter,
> es_parallel_workers_launche
On 22/7/2025 01:07, Michael Paquier wrote:
On Mon, Jul 21, 2025 at 04:47:31PM -0500, Sami Imseih wrote:
Last week I published a v11 that adds a field to QueryDesc, but as I thought
about this more, how about we just add 2 bool fields in QueryDesc->estate
( es_cached_plan and es_is_generic_plan )
On 22/7/2025 01:22, Sami Imseih wrote:
Note: the size of the change in pg_stat_statements--1.12--1.13.sql
points that we should seriously consider splitting these attributes
into multiple sub-functions.
So we don't lose track of this. This should be a follow-up thread. I do
agree something has
> Note: the size of the change in pg_stat_statements--1.12--1.13.sql
> points that we should seriously consider splitting these attributes
> into multiple sub-functions.
So we don't lose track of this. This should be a follow-up thread. I do
agree something has to be done about the exploding list
> Yes, I think that this is a much better idea to isolate the whole
> concept and let pgss grab these values. We have lived with such
> additions for monitoring in EState a few times already, see for
> example de3a2ea3b264 and 1d477a907e63 that are tainted with my
> fingerprints.
correct, there i
On Mon, Jul 21, 2025 at 04:47:31PM -0500, Sami Imseih wrote:
> Last week I published a v11 that adds a field to QueryDesc, but as I thought
> about this more, how about we just add 2 bool fields in QueryDesc->estate
> ( es_cached_plan and es_is_generic_plan ), a field in CachedPlan (
> is_generic_p
> > "plan cache mode" to be accessible in ExecutorEnd somehow.
> I agree with this - adding references to CachedPlan into the QueryDesc
> looks kludge.
> The most boring aspect of pg_stat_statements for me is the multiple
> statements case: a single incoming query (the only case in the cache of
> a
On 18/7/2025 21:37, Sami Imseih wrote:
Thanks for clearing up my understanding. Essentially, override the
current cost-based
method of determining custom vs. generic by using something like execution time,
which is somehow tracked by the extension. That is how I understand this.
Now, I wonder if
> Hmm, I don't propose modifying costs. The focus is on resetting the plan
> cache decision that PostgreSQL has made in automatic mode. During the
> DBMS operation, various factors may cause a generic plan to be
> suboptimal or make it more desirable as well. Discussions from 2010 to
> 2013 indicat
On 17/7/2025 20:19, Sami Imseih wrote:
Thanks for the clarification. I see what you're getting at now.
Thanks for reading this!
You're suggesting adding CachedPlanSource to QueryDesc instead of
CachedPlan. This would allow extensions to access the statistics and cost
information from the Cache
> >> this for better tracking. By adding a CachedPlanSource::cplan link, we
> >> can establish a connection to the entire PlanCache entry instead of only
> >> CachedPlan within a queryDesc and, consequently, make it accessible from
> >> the executor. This would give an access to statistics on costs
On 17/7/2025 00:58, Sami Imseih wrote:
this for better tracking. By adding a CachedPlanSource::cplan link, we
can establish a connection to the entire PlanCache entry instead of only
CachedPlan within a queryDesc and, consequently, make it accessible from
the executor. This would give an access t
> > Ugh. This is plugging into an executor-related structure a completely
> > different layer, so that looks like an invasive layer violation to
> > me.. This is passed through ProcessQuery() from a Portal, changing
> > while on it ExplainOnePlan. If we want to get access from a cached
> > plan,
> this for better tracking. By adding a CachedPlanSource::cplan link, we
> can establish a connection to the entire PlanCache entry instead of only
> CachedPlan within a queryDesc and, consequently, make it accessible from
> the executor. This would give an access to statistics on costs and the
> n
> Ugh. This is plugging into an executor-related structure a completely
> different layer, so that looks like an invasive layer violation to
> me.. This is passed through ProcessQuery() from a Portal, changing
> while on it ExplainOnePlan. If we want to get access from a cached
> plan, wouldn't
On 6/30/25 13:45, Sami Imseih wrote:
rebased patch.
Only changes to the tests due to the revert of nested query
tracking in f85f6ab051b
Thank you for your efforts.
I would like to share a few thoughts about this patch. First, I believe
the 'status' field could be renamed to 'mode,' as it alig
On Mon, Jun 30, 2025 at 02:45:49PM +0300, Sami Imseih wrote:
> Only changes to the tests due to the revert of nested query
> tracking in f85f6ab051b
@@ -35,6 +36,7 @@ typedef struct QueryDesc
/* These fields are provided by CreateQueryDesc */
CmdType operation;
rebased patch.
Only changes to the tests due to the revert of nested query
tracking in f85f6ab051b
Regards,
Sami
v10-0001-Add-plan_cache-counters-to-pg_stat_statements.patch
Description: Binary data
> Thanks for your work on this.
>
> Since we've changed the placement of these parameters
> between parallel_workers and stats_since, we should also update
> the documentation to reflect their new location.
Absolutely. My miss. Here is v9 with the doc updated.
Thanks!
--
Sami
v9-0001-Add-plan_
On 03.06.2025 06:31, Sami Imseih wrote:
See v8 with the field names reorganized.
Apologies if you received two identical emails.
Since we've changed the placement of these parameters between
parallel_workers and stats_since, we should also update the
documentation to reflect their new loca
On 03.06.2025 06:31, Sami Imseih wrote:
See v8 with the field names reorganized.
Thanks for your work on this.
Since we've changed the placement of these parameters between
parallel_workers and stats_since, we should also update the
documentation to reflect their new location.
--
Best re
> Now, one thing I don't like is the fact that the columns stats_since
> and minmax_stats_since
> are in between counters all of the sudden. I think we either need to
> move them to
> the front of the view, after the query field or within this patch
> move them after the
> new generic/custom plan
> I reviewed v6:
>
> - applies to master cleanly, builds, tests pass and all works as expected
> - overall, the patch looks great and I found no major issues
> - tests and docs look good overall
Thanks for the valuable feedback!
> - in docs, one minor comment:
> > "Total number of statements
On Sat, May 31, 2025 at 5:06 PM Nikolay Samokhvalov wrote:
> On Thu, May 29, 2025 at 11:56 AM Sami Imseih wrote:
>
>> It turns out that 1722d5eb05d8e reverted 525392d5727f, which
>> made CachedPlan available in QueryDesc and thus
>> available to pgss_ExecutorEnd.
>>
>> So now we have to make Cac
On Thu, May 29, 2025 at 11:56 AM Sami Imseih wrote:
> It turns out that 1722d5eb05d8e reverted 525392d5727f, which
> made CachedPlan available in QueryDesc and thus
> available to pgss_ExecutorEnd.
>
> So now we have to make CachedPlan available to QueryDesc as
> part of this change. The reason t
It turns out that 1722d5eb05d8e reverted 525392d5727f, which
made CachedPlan available in QueryDesc and thus
available to pgss_ExecutorEnd.
So now we have to make CachedPlan available to QueryDesc as
part of this change. The reason the patch was reverted is related
to a memory leak [0] in the Buil
> It turns out that 1722d5eb05d8e reverted 525392d5727f, which
> made CachedPlan available in QueryDesc and thus
> available to pgss_ExecutorEnd.
I also forgot to mention, the revert also removes the generic plan
is_reused logic:
```
boolis_reused; /* is it a reused gener
> After the introduction of pg_overexplain extension and Robert's comment
> [0], I'm starting to have doubts about whether it's still appropriate to
> add this information to EXPLAIN itself. If there are compelling reasons
> that showing the plan type would be broadly useful to users in EXPLAIN,
>
rebased in the attached v5.
--
Sami Imseih
Amazon Web Services (AWS)
v5-0001-Add-plan_cache-counters-to-pg_stat_statements.patch
Description: Binary data
After the introduction of pg_overexplain extension and Robert's comment
[0], I'm starting to have doubts about whether it's still appropriate to
add this information to EXPLAIN itself. If there are compelling reasons
that showing the plan type would be broadly useful to users in EXPLAIN,
I’m ha
> Thank you for your patch. It is really useful for tracking the history
> of generic and custom plan usage.
Thanks for the review!
> 1. Is there any reason for the double check of cplan != NULL? It seems
> unnecessary, and we could simplify it to:
>
> -if (cplan && cplan->status == PLAN_CACHE_ST
> I don't quite understand why do we need to differentiate between
> PLAN_CACHE_STATUS_GENERIC_PLAN_BUILD and
> PLAN_CACHE_STATUS_GENERIC_PLAN_REUSE?
> We could simply keep PLAN_CACHE_STATUS_GENERIC_PLAN_REUSE.
> I don't think users would see much of a difference in either
> pg_stat_statements or
On 06.03.2025 18:04, Sami Imseih wrote:
2. Should we add Assert(kind == PGSS_EXEC) at this place to ensure that
generic_plan_calls and custom_plan_calls are only incremented when
appropriate?
I don't think an assert is needed here. There is an assert at the start of
the block for PGSS_EXEC an
Hi,
Thank you for your patch. It is really useful for tracking the history
of generic and custom plan usage.
At first glance, I have the following suggestions for improvement:
1. Is there any reason for the double check of cplan != NULL? It seems
unnecessary, and we could simplify it to:
-
>
> Please see v2
>
oops, had to fix a typo in meson.build. Please see v3.
--
Sami
v3-0001-add-plan_cache-counters-to-pg_stat_statements.patch
Description: Binary data
Thanks for the review!
> I could see EXPLAIN being somewhat useful (especially for non-interactive
> things like auto_explain), so a weak +1 on that.
I'll make this a follow-up to this patch.
> Definitely not useful for pg_stat_database IMHO.
agree as well. I did not have strong feelings about
Overall I like the idea; adds some nice visibility to something that has
been very ephemeral in the past.
Not included in this patch and maybe for follow-up work, is this
> information a good idea also?
can be added to EXPLAIN output and perhaps pg_stat_database.
>
I could see EXPLAIN being some
Hi,
Currently, there is little visibility for queries that are being executed
using generic or custom plans. There is pg_prepared_statements which
show generic_plans and custom_plans as of d05b172a760, but this
information is backend local and not very useful to a DBA that wishes
to track this inf
67 matches
Mail list logo
