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 this. > FILE: contrib/pg_stat_statements/expected/plan_cache.out > > These tests seem very verbose. Why not just prepare a simple query: > > prepare foo as select $1 > 0; > execute foo(1); > ...then tweak things via plan_cache_mode to ensure we test the right things? Yeah, I went overboard to see what others think. I toned it down drastically for v2 following your advice. > oldextversions should still have a trailing comma done > FILE: contrib/pg_stat_statements/pg_stat_statements.c > > + if (cplan && cplan->status > PLAN_CACHE_STATUS_CUSTOM_PLAN) > > Not really comfortable with using the enum like this. Better would be > explicitly listing the two states that lead to the increment. Not as good but > still better: > cplan->status >= PLAN_CACHE_STATUS_GENERIC_PLAN_BUILD fair, I use explicit values for each plan type. > + PlanCacheStatus status; /* is it a reused generic plan? */ > > The comment should be updated done > > FILE: contrib/pg_stat_statements/sql/plan_cache.sql > > Missing a newline at the end done > FILE: doc/src/sgml/pgstatstatements.sgml > > + Total number of non-utility statements executed using a generic plan > > I'm not sure we need to specify non-utility here. > fair, I did not have strong feeling about this either. Please see v2 Thanks! -- Sami Imseih Amazon Web Services (AWS)
v2-0001-add-plan_cache-counters-to-pg_stat_statements.patch
Description: Binary data
