Re: track generic and custom plans in pg_stat_statements

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
theincrement. 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)