Postgres query plan fix #45230
Postgres query plan fix #45230
Conversation
github-actions
bot
requested review from
Caleb-Hurshman,
antonblock and
ishleenk17
akshays-19
force-pushed
the
Postgres_Query_Plan_Fix
branch
from
0cb7d3b to
5453fd6
Compare
| - Adds tests to verify proper connection closure. | ||
| Previously, the EXPLAIN query was using obfuscated queries with ? placeholders, which PostgreSQL does not recognize. | ||
| Now uses the raw query with $1, $2 placeholders that PostgreSQL expects. | ||
| Also orders top query results by calls DESC, stats_since DESC. |
There was a problem hiding this comment.
Also orders top query results by calls DESC, stats_since DESC.
I suggest we move this order change to another individual PR, as it is unrelated to issue #45190
There was a problem hiding this comment.
ORDER BY calls desc is to get queries with highest execution count in Top N, there is really no change in the data collected
There was a problem hiding this comment.
My point is this change has nothing to do with issue #45190 (query plan issue). This is more like an additional enhancement combined with the fix. It is good to separate these, though I am fine to keep it combined.
ORDER BY calls desc is to get queries with highest execution count in Top N, there is really no change in the data collected
It may not change the result from a test environment, but it does change the collection behavior.
There was a problem hiding this comment.
I understand the concern. However, given the slower review cycle for the PostgreSQL PRs, I chose to include this small change in the same PR to prevent additional delays. thanks!
Remove ORDER BY stats_since DESC in topQuery template
akshays-19
force-pushed
the
Postgres_Query_Plan_Fix
branch
from
c290965 to
01e4234
Compare
4 participants
Description
Fixing the bug where postgresql query plan were not getting generated. Reason being, the obfuscation done prior to EXPLAIN was causing errors. Now EXPLAIN gets unonfuscated query andpland generate correctly.
Another issue fixed here was adding of correct ORDER BY clause to the query to ensure correct set of TopN queries are collected.
ORDER BY CALLS DESC ensures we get queries with highest execution count in Top N, additionally we are sub-ordering by stats_since to ensure the query most recently first executed is also caught in Top N for the last 60 sec
Link to tracking issue
Fixes #45190
Testing
Unit tests updated. Also ran in internal environment for extended testing.