Hi,
There are several spots where pg_stat_statements is modifying core queryId.
1/ In pgss_post_parse_analyze, if the statement is an EXECUTE
```
/*
* If it's EXECUTE, clear the queryId so that stats will accumulate for
* the underlying PREPARE. But don't do this if we're not tracking
* utility statements, to avoid messing up another extension that might be
* tracking them.
*/
if (query->utilityStmt)
{
if (pgss_track_utility && IsA(query->utilityStmt, ExecuteStmt))
{
//query->queryId = INT64CONST(0);
return;
}
```
2/ In pgss_ProcessUtility, when track_utility is enabled
```
if (enabled)
pstmt->queryId = INT64CONST(0);
```
These 2 calls have always bothered me a bit since I don't think an
extension should be
messing with the core queryId.
But as I was looking into something else in pg_s_s, I removed these
calls and the tests
worked. EXECUTE and other utility statements have proper coverage, so it's not a
lack of coverage.
Digging in a bit, it turns out they may have been needed in the past,
but that does not seem
to hold true any longer.
For #1, due to 76db9cb6368e, we no longer track PREPARE and EXECUTE
statements ( PREPARE is tracked by the underlying queryId ).
```
/*
* If it's an EXECUTE statement, we don't track it and don't increment the
* nesting level. This allows the cycles to be charged to the underlying
* PREPARE instead (by the Executor hooks), which is much more useful.
*
* We also don't track execution of PREPARE. If we did, we would get one
* hash table entry for the PREPARE (with hash calculated from the query
* string), and then a different one with the same query string (but hash
* calculated from the query tree) would be used to accumulate costs of
* ensuing EXECUTEs. This would be confusing. Since PREPARE doesn't
* actually run the planner (only parse+rewrite), its costs are generally
* pretty negligible and it seems okay to just ignore it.
*/
if (enabled &&
!IsA(parsetree, ExecuteStmt) &&
!IsA(parsetree, PrepareStmt))
```
For #2,
The nesting_level is incremented before we call
standard_ProcessUtility, so the underlying SQL will not be tracked
unless pg_stat_statements.track = 'all', and we have tests added
in 45e0ba30fc40 and f85f6ab051 that test this behavior is correct.
i.e.
```
+SELECT toplevel, calls, query FROM pg_stat_statements
+ ORDER BY query COLLATE "C";
+ toplevel | calls | query
+----------+-------+---------------------------------------------------------------------
+ t | 1 | EXPLAIN (COSTS OFF) (SELECT $1, $2)
+ f | 1 | EXPLAIN (COSTS OFF) (SELECT $1, $2);
```
I think it will be good to remove pg_stat_statements manipulation
of queryId, but I am not sure if I missed a case where this might break.
I can prepare a patch to remove the calls mentioned above if there
is agreement.
It will be good to make the queryId a Const to extensions, but
that is a bigger lift.
--
Sami Imseih
Amazon Web Services (AWS)
