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
signature.asc
Description: PGP signature