> 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)
v7-0001-Add-plan_cache-counters-to-pg_stat_statements.patch
Description: Binary data
