Re: Proposal - Allow extensions to set a Plan Identifier

On Thu, Feb 20, 2025 at 05:03:19PM -0600, Sami Imseih wrote:
> v2-0001:
>
> This adds a planId to pg_stat_get_activity ( not pg_stat_activity ).
> An extension
> can offer its own view, similar to pg_stat_activity, which can include planId.
>
> Note that there are no documentation updates required here as we don't
> have per-field documentation for pg_stat_get_activity.
>
> These 2 patches can probably be combined , but will leave them like
> this for now.

The split makes more sense to me, and FWIW I see more value in 0001
that provides an anchor on the backend side for extensions to plug in
a plan ID that can be tracked across multiple layers. I am less
convinced about the need for 0002 that adds this information to
pg_stat_activity at this stage, as the getter routine in 0001 is
enough to know what's happening, roughly.

+void
+pgstat_report_plan_id(uint64 plan_id, bool force)
+{
+ volatile PgBackendStatus *beentry = MyBEEntry;
+
+ if (!beentry || !pgstat_track_activities)
+ return;
+
+ if (beentry->st_plan_id != 0 && !force)
+ return;

Right, and that's in line with the cases I've seen because we wanted
to be able to optionally control if only the top-level plan ID should
be stored or not, which is useful when going through the planner hook
at some sub-level. Hmm. Shouldn't we document all such expectations,
which are similar to a query ID? Our experience with pgss in this
area offers some insights, which I guess folks are rather used to and
could be applied to plan IDs.

In short, it would be nice to do 0001 for this release and start from
that (with more documentation around the expectations of these
interfaces). For 0002 we could always see later about exposing it in
a system view, even if it would offer the advantage of more
consistency across a single call of
pgstat_get_local_beentry_by_index() for each backend entry.
--
Michael