Re: Normalize queries starting with SET for pg_stat_statements - Mailing list pgsql-hackers
From | Michael Paquier |
---|---|
Subject | Re: Normalize queries starting with SET for pg_stat_statements |
Date | |
Msg-id | Zp8XRfSzgYtQPX93@paquier.xyz Whole thread Raw |
In response to | Normalize queries starting with SET for pg_stat_statements (Greg Sabino Mullane <htamfids@gmail.com>) |
Responses |
Re: Normalize queries starting with SET for pg_stat_statements
|
List | pgsql-hackers |
On Mon, Jul 22, 2024 at 03:23:50PM -0400, Greg Sabino Mullane wrote: > I saw a database recently where some app was inserting the source port into > the application_name field, which meant that pg_stat_statements.max was > quickly reached and queries were simply pouring in and out of > pg_stat_statements, dominated by some "SET application_name = 'myapp > 10.0.0.1:1234'" calls. Which got me thinking, is there really any value to > having non-normalized 'SET application_name' queries inside of > pg_stat_statements? Or any SET stuff, for that matter? Thanks for beginning this discussion. This has been mentioned in the past, like here, but I never came back to it: https://www.postgresql.org/message-id/B44FA29D-EBD0-4DD9-ABC2-16F1CB087074@amazon.com I've seen complaints about that in the field myself, and like any other specific workloads, the bloat this stuff creates can be really annoying when utilities are tracked. So yes, I really want more stuff to happen here. > Attached please find a small proof-of-concept for normalizing/de-jumbling > certain SET queries. Because we only want to cover the VAR_SET_VALUE parts > of VariableSetStmt, a custom jumble func was needed. There are a lot of > funky SET things inside of gram.y as well that don't do the standard SET X > = Y formula (e.g. SET TIME ZONE, SET SCHEMA). I tried to handle those as > best I could, and carved a couple of exceptions for time zones and xml. Agreed about the use of a custom jumble function. A huge issue that I have with this parsing node is how much we want to control back in the monitoring reports while not making the changes too invasive in the structure of VariableSetStmt. The balance between invasiveness and level of normalization was the tricky part for me. > I'm not sure where else to possibly draw lines. Obviously calls to time > zone have a small and finite pool of possible values, so easy enough to > exclude them, while things like application_name and work_mem are fairly > infinite, so great candidates for normalizing. One could argue for simply > normalizing everything, as SET is trivially fast for purposes of > performance tracking via pg_stat_statements, so who cares if we don't have > the exact string? That's what regular logging is for, after all. Most > importantly, less unique queryids means less chance that errant SETs will > crowd out the more important stuff. > In summary, we want to change this: > > SELECT calls, query from pg_stat_statements where query ~ 'set' order by 1; > 1 | set application_name = 'alice' > 1 | set application_name = 'bob' > 1 | set application_name = 'eve' > 1 | set application_name = 'mallory' > > to this: > > SELECT calls, query from pg_stat_statements where query ~ 'set' order by 1; > 4 | set application_name = $1 Yep. That makes sense to me. We should keep the parameter name, hide the value. CallStmt does that. > I haven't updated the regression tests yet, until we reach a consensus on > how thorough the normalizing should be. But there is a new test to exercise > the changes in gram.y. It would be nice to maximize the contents of the tests at SQL level. The number of patterns to track makes the debuggability much harder to track correctly in TAP as we may rely on more complex quals in pg_stat_statements or even other catalogs. + if (expr->kind != VAR_SET_VALUE || + strcmp(expr->name, "timezone") == 0 + || strcmp(expr->name, "xmloption") == 0) + JUMBLE_NODE(args); We should do this kind of tracking with a new flag in the structure itself, not in the custom function. The point would be to have folks introducing new hardcoded names in VariableSetStmt in the parser think about what they want to do, not second-guess it by tweaking the custom jumbling function. Relying on the location would not be enough as we need to cover document_or_content for xmloption or the default keyword for TIME ZONE. Let's just use the same trick as DeallocateStmt.isall, with a new field to differentiate all these cases :) There are some funky cases you are not mentioning, though, like SET in a CREATE FUNCTION: CREATE OR REPLACE FUNCTION foo_function(data1 text) RETURNS text AS $$ DECLARE res text; BEGIN SELECT data1::text INTO res; RETURN res; END; $$ LANGUAGE plpgsql IMMUTABLE SET search_path = 'pg_class,public'; Your patch silences the SET value, but perhaps we should not do that for this case. I am not against normalizing that, actually, I am in favor of it, because it makes the implementation much easier and the FunctionParameter List of parameters is jumbled with CreateFunctionStmt. All that requires test coverage. It's nice to see that you are able to keep SET TRANSACTION at their current level with the location trick. You have issues with RESET SESSION AUTHORIZATION. This one is easy: update the location field to -1 in reset_rest for all the subcommands. The coverage of the regression tests in pg_stat_statements is mostly right. I may be missing something, but all the SQL queries you have in your 020_jumbles.pl would work fine with SQL tests, and some like `SET param = value` are already covered. -- Michael
Attachment
pgsql-hackers by date: