> In exec_bind_message(), the comment at the top of PortalDefineQuery()
> tells to not put any code between this call and the GetCachedPlan()
> that could issue an error. pgstat_report_plan_id() is OK, but I'd
> rather play it safe and set the ID once the portal is defined, based
> on portal->stmts instead. That's the same as your patch, but slightly
> safer in the long-term, especially if pgstat_report_plan_id() is
> twisted in such a way that it introduces a path where it ERRORs.
Yes that makes sense. As long as we report plan_id before
PortalStart, for obvious reasons.
> planner() is the sole place in the core code where the planner hook
> can be called. Shouldn't we have at least a call to
> pgstat_report_plan_id() after planning a query? At least that should
> be the behavior I'd expect, where a module pushes a planId to a
> PlannedStmt, then core publishes it to the backend entry in non-force
> mode.
I agree. I was just thinking we rely on the exec_ routines to report the plan_id
at the start. But, besides the good reason you give, reporting
(slightly) earlier is
better for monitoring tools; as it reduces the likelihood they find an empty
plan_id.
Overall, v3 LGTM
--
Sami Imseih
Amazon Web Services (AWS)
