On Wed, Aug 07, 2019 at 09:03:21AM +0000, Evgeny Efimkin wrote:
> The new status of this patch is: Ready for Committer
I may be wrong of course, but it looks that this is wanted and the
current shape of the patch looks sensible:
- Register the query ID using a backend entry.
- Only consider the top-level query.
An invalid query ID is assumed to be 0 in the patch, per the way it is
defined in pg_stat_statements. However this also maps with the case
where we have a utility statement.
+ * We only report the top-level query identifiers. The stored queryid is
+ * reset when a backend call pgstat_report_activity(STATE_RUNNING), or with
s/call/calls/
+ /*
+ * We only report the top-level query identifiers. The stored queryid is
+ * reset when a backend call pgstat_report_activity(STATE_RUNNING), or with
+ * an explicit call to this function. If the saved query identifier is not
+ * zero it means that it's not a top-level command, so ignore the one
+ * provided unless it's an explicit call to reset the identifier.
+ */
+ if (queryId != 0 && beentry->st_queryid != 0)
+ return;
Hmm. I am wondering if we shouldn't have an API dedicated to the
reset of the query ID. That logic looks rather brittle..
Wouldn't it be better (and more consistent) to update the query ID in
parse_analyze_varparams() and parse_analyze() as well after going
through the post_parse_analyze hook instead of pg_analyze_and_rewrite?
+ /*
+ * If a new query is started, we reset the query identifier as it'll only
+ * be known after parse analysis, to avoid reporting last query's
+ * identifier.
+ */
+ if (state == STATE_RUNNING)
+ beentry->st_queryid = 0
I don't quite get why you don't reset the counter in other cases as
well. If the backend entry is idle in transaction or in an idle
state, it seems to me that we should not report the query ID of the
last query run in the transaction. And that would make the reset in
exec_simple_query() unnecessary, no?
--
Michael