Re: Proposal - Allow extensions to set a Plan Identifier

On Thu, Mar 20, 2025 at 09:46:55PM -0400, Sami Imseih wrote:
> * For the same reasons as the query identifiers (see above),
> 
> but, I went ahead and commented it similar to how we document
> pgstat_report_query_id and pgstat_get_my_query_id routines.
> attached is v2-0001

Looks mostly OK from here..  Except for two things.

@@ -2016,6 +2017,17 @@ exec_bind_message(StringInfo input_message)
      */
     cplan = GetCachedPlan(psrc, params, NULL, NULL);
 
+    foreach(lc, cplan->stmt_list)
+    {
+        PlannedStmt *plan = lfirst_node(PlannedStmt, lc);
+
+        if (plan->planId != UINT64CONST(0))
+        {
+            pgstat_report_plan_id(plan->planId, false);
+            break;
+        }
+    }

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.

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.

bind and execute message paths are OK as they stand, where we set a
plan ID once their portal is defined from its planned statements.

With some adjustments to some comments and the surroundings of the
code, I get the attached.  What do you think?
--
Michael