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 BuildCachedPlan code and is not related
to the part that made CachedPlan available to QueryDesc.
See v6 for the rebase of the patch and addition of testing for EXPLAIN
and EXPLAIN ANALYZE which was missing from v5.
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
- 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 ...")
- Also very minor, the test queries have duplicate `calls` columns:
> SELECT calls, generic_plan_calls, custom_plan_calls, toplevel, calls, ...
- plan->status is set in GetCachedPlan() but I don't see explicit initialization in plan creation paths; maybe it's worth having a defensive initialization for possible edge cases:
> 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)
that's all I could find – and overall it's a great addition,
thank you, looking forward to having these two columns in prod.
Nik
