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