From: | Sami Imseih <samimseih(at)gmail(dot)com> |
---|---|
To: | Nikolay Samokhvalov <nik(at)postgres(dot)ai> |
Cc: | Ilia Evdokimov <ilya(dot)evdokimov(at)tantorlabs(dot)com>, Greg Sabino Mullane <htamfids(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: track generic and custom plans in pg_stat_statements |
Date: | 2025-06-02 15:52:25 |
Message-ID: | CAA5RZ0vGBFHvGb4iZdmSADuybkHGNrxVHknTGaEhS2=2LcM56g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> 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 executed using a generic plan" vs. what we already have for `calls`
> here, in "Number of times the statement was executed", I see some inconsistencies:
> - the word "total" should be removed, I think
> - and maybe we should make wording consistent with the existing text – "number of times the statement ...")
good point. Changed to:
> + Number of times the statement was executed using a generic plan
> + Number of times the statement was executed using a custom plan
> - Also very minor, the test queries have duplicate `calls` columns:
good catch. fixed.
> > plan->status = PLAN_CACHE_STATUS_UNKNOWN;
> (here I'm not 100% sure, as palloc0 in CreateCachedPlan should zero-initialize it to PLAN_CACHE_STATUS_UNKNOWN anyway)
Yeah, good catch also. The initialization actually occurs in
GetCachedPlan which returns
a CachedPlan for the CachedPlanSource, so I fixed this by initializing
the status
there.
> Ah, one more thing: the subject here and in CommitFest entry, "track generic and custom plans"
> slightly confused me at first, I think it's worth including words "calls" or "execution" there, and in
> the commit message, for clarity. Or just including the both column names as is.
changed the subject of the CF entry to be more clear.
attached v7 which addresses the comments.
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 counters. I prefer the former since we do it
once in a major version
and do not have to worry about it once new counters are added.
It just feels odd that they sit in between the counters as they have a
high level purpose.
Thanks!
Sami Imseih
Amazon Web Services (AWS)
Attachment | Content-Type | Size |
---|---|---|
v7-0001-Add-plan_cache-counters-to-pg_stat_statements.patch | application/octet-stream | 37.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alexander Korotkov | 2025-06-02 16:25:28 | Re: Vacuum statistics |
Previous Message | Nathan Bossart | 2025-06-02 15:32:19 | pg_upgrade: warn about roles with md5 passwords |