Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PG -156: replace query placeholders with actual arguments for prepared statements #481

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

darkfronza
Copy link
Contributor

@darkfronza darkfronza commented Aug 1, 2024

PG-156

Before this PR, pg_stat_monitor was unable to replace query placeholders when tracking prepared statements if pg_stat_monitor.pgsm_normalized_query is off.

With this update pg_stat_monitor now correctly store query argument values for prepared statements.

@darkfronza darkfronza added the enhancement New feature or request label Aug 1, 2024
@darkfronza darkfronza self-assigned this Aug 1, 2024
@it-percona-cla
Copy link

it-percona-cla commented Aug 1, 2024

CLA assistant check
All committers have signed the CLA.

@darkfronza darkfronza force-pushed the PG-156-replace-query-placeholders branch 2 times, most recently from 627e075 to 2487f44 Compare August 9, 2024 19:28
@darkfronza darkfronza marked this pull request as ready for review August 9, 2024 19:29
@darkfronza darkfronza force-pushed the PG-156-replace-query-placeholders branch from 2487f44 to 3d0a3cf Compare August 9, 2024 19:36
Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 92.64706% with 5 lines in your changes missing coverage. Please review.

Project coverage is 85.38%. Comparing base (467394f) to head (43d5548).

Files Patch % Lines
pg_stat_monitor.c 92.64% 0 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #481      +/-   ##
==========================================
+ Coverage   84.88%   85.38%   +0.50%     
==========================================
  Files           3        3              
  Lines        1297     1362      +65     
  Branches      198      208      +10     
==========================================
+ Hits         1101     1163      +62     
+ Misses         98       97       -1     
- Partials       98      102       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pg_stat_monitor.c Outdated Show resolved Hide resolved
pg_stat_monitor.c Outdated Show resolved Hide resolved
pg_stat_monitor.c Outdated Show resolved Hide resolved
pg_stat_monitor.c Outdated Show resolved Hide resolved
pg_stat_monitor.c Show resolved Hide resolved
regression/expected/pgsm_query_id.out Outdated Show resolved Hide resolved
regression/sql/denorm_prepared_statements.sql Outdated Show resolved Hide resolved
regression/sql/denorm_prepared_statements.sql Show resolved Hide resolved
@artemgavrilov artemgavrilov self-requested a review August 19, 2024 13:48
@darkfronza darkfronza force-pushed the PG-156-replace-query-placeholders branch 2 times, most recently from 4223d61 to 97535ef Compare August 19, 2024 20:47
Added support for extracting query arguments for prepared statements
when `pg_stat_monitor.pgsm_normalized_query` is off.

Previously pg_stat_monitor was unable to extract the arguments for
prepared statements, thus leaving queries with placeholders $1
.. $N instead of the actual arguments.
Instead of copying original query text byte by byte, copy data between
query placeholders in chunks, example:

`INSERT INTO foo(a, b, c) VALUES('test', 100, 'test again)'`

Would result in normalized query:

`INSERT INTO foo(a, b, c) VALUES($1, $2, $3)`

The original patch would copy the parts between placeholders byte by
byte, e.g. `INSERT INTO foo(a, b, c) VALUES(`, instead we can copy this
whole block at once, 1 function call and maybe 1 buffer re-allocation
per call.

Also make use of `appendBinaryStringInfo` to avoid calculating string
length as we have this info already.
Avoid allocating an array of strings for extracting query argument
values, instead append the current parameter value directly in the
buffer used to store the denormalized query.

This avoids not only unnecessary memory allocations, but also copying
data between temporary memory and the buffer.
This commit introduces a little optimization along with a feature, it
stores the query in denormalized form only under the circumstances
below:

- The psgm_normalized_query GUC is disabled (off).
- The query is seem for the first time, or the query total
  execution time exceeds the mean execution time calculated for
  the previous queries.

Having the query which took most execution time along with it's
arguments could help users in further investigating performance issues.
When query normalization is disabled utility queries like SELECT 10+20
are now stored as is, instead of SELECT $1+$2.

Also when functions or sub queries are created the arguments used
internally by the function or subqueries will be replaced by NULL instead
of $1..$N. The actual arguments will be displayed when the function or
subquery is actually invoked.
Ensures that the denormalization of prepared statements is working, also
ensure that a query which takes more time to execute replaces the
previous denormalized query.
With the query dernomalization feature, having integer literals used in
sql like 1, or 2 could create some confusion on whether those are
placeholders or constant values, thus this commit updates the
pgsm_query_id regression test to use different integer literals to avoid
confusion.
Add a new test case:

1. Execute a prepared statement with larger execution time first.
2. Execute the same prepared statement with cheap execution time.
3. Ensures that the denormalized heavy query is not replaced by the
   cheaper.
On PG 12, 13, the internal return instruction in the following function:
```
CREATE OR REPLACE FUNCTION add(int, int) RETURNS INTEGER AS
  $$
  BEGIN
     return (select $1 + $2);
  END; $$ language plpgsql;
```

Is stored as SELECT (select expr1 + expr2)

On PG 14 onward it's stored just as SELECT (expr1 + expr2)
@darkfronza darkfronza force-pushed the PG-156-replace-query-placeholders branch from 97535ef to 43d5548 Compare August 26, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants