8 Commits (0fbfd37cefb7eb30c0fa8a158751c19ddeddf1f0)

Author	SHA1	Message	Date
Michael Paquier	e217dc7484	Fix query jumbling with GROUP BY clauses ... RangeTblEntry.groupexprs was marked with the node attribute query_jumble_ignore, causing a list of GROUP BY expressions to be ignored during the query jumbling. For example, these two queries could be grouped together within the same query ID: SELECT count(*) FROM t GROUP BY a; SELECT count(*) FROM t GROUP BY b; However, as such queries use different GROUP BY clauses, they should be split across multiple entries. This fixes an oversight in 247dea89f7, that has introduced an RTE for GROUP BY clauses. Query IDs are documented as being stable across minor releases, but as this is a regression new to v18 and that we are still early in its support cycle, a backpatch is exceptionally done as this has broken a behavior that exists since query jumbling is supported in core, since its introduction in pg_stat_statements. The tests of pg_stat_statements are expanded to cover this area, with patterns involving GROUP BY and GROUPING clauses. Author: Jian He <jian.universality@gmail.com> Discussion: https://postgr.es/m/CACJufxEy2W+tCqC7XuJ94r3ivWsM=onKJp94kRFx3hoARjBeFQ@mail.gmail.com Backpatch-through: 18	2 months ago
Álvaro Herrera	0f65f3eec4	Fix squashing algorithm for query texts ... The algorithm to squash lists of constants added by commit 62d712ecfd was a bit too simplistic; we wanted to avoid adding unnecessary complexity, but cases like direct function calls of typecasting functions (and others) were missed, and bogus SQL syntax was being shown in pg_stat_statements normalized query text field. To fix normalization for those cases, we need the parser to transmit information about were each list of constant values starts and ends, so add that to a couple of nodes. Also add a few more test cases to make sure we're doing the right thing. The patch initially submitted by Sami added a new private struct in gram.y to carry the start/end information for A_Expr, but I (Álvaro) decided that a better fix was to remove the parser indirection via the in_expr production, and instead create separate components in the a_expr rule. I'm surprised that this works and doesn't require more changes, but I assume (without checking) that the grammar used to be more complex and got simplified at some point. Bump catversion. Author: Sami Imseih <samimseih@gmail.com> Author: Dmitry Dolgov <9erthalion6@gmail.com> Reviewed-by: Michael Paquier <michael@paquier.xyz> Discussion: https://postgr.es/m/CAA5RZ0tRXoPG2y6bMgBCWNDt0Tn=unRerbzYM=oW0syi1=C1OA@mail.gmail.com	9 months ago
Michael Paquier	35a428f30b	pg_stat_statements: Fix parameter number gaps in normalized queries ... pg_stat_statements anticipates that certain constant locations may be recorded multiple times and attempts to avoid calculating a length for these locations in fill_in_constant_lengths(). However, during generate_normalized_query() where normalized query strings are generated, these locations are not excluded from consideration. This could increment the parameter number counter for every recorded occurrence at such a location, leading to an incorrect normalization in certain cases with gaps in the numbers reported. For example, take this query: SELECT WHERE '1' IN ('2'::int, '3'::int::text) Before this commit, it would be normalized like that, with gaps in the parameter numbers: SELECT WHERE $1 IN ($3::int, $4::int::text) However the correct, less confusing one should be like that: SELECT WHERE $1 IN ($2::int, $3::int::text) This commit fixes the computation of the parameter numbers to track the number of constants replaced with an $n by a separate counter instead of the iterator used to loop through the list of locations. The underlying query IDs are not changed, neither are the normalized strings for existing PGSS hash entries. New entries with fresh normalized queries would automatically get reshaped based on the new parameter numbering. Issue discovered while discussing a separate problem for HEAD, but this affects all the stable branches. Author: Sami Imseih <samimseih@gmail.com> Discussion: https://postgr.es/m/CAA5RZ0tzxvWXsacGyxrixdhy3tTTDfJQqxyFBRFh31nNHBQ5qA@mail.gmail.com Backpatch-through: 13	10 months ago
David Rowley	f31aad9b07	Fix query jumbling to account for NULL nodes ... Previously NULL nodes were ignored. This could cause issues where the computed query ID could match for queries where fields that are next to each other in their Node struct where one field was NULL and the other non-NULL. For example, the Query struct had distinctClause and sortClause next to each other. If someone wrote; SELECT DISTINCT c1 FROM t; and then; SELECT c1 FROM t ORDER BY c1; these would produce the same query ID since, in the first query, we ignored the NULL sortClause and appended the jumble bytes for the distictClause. In the latter query, since we did nothing for the NULL distinctClause then jumble the non-NULL sortClause, and since the node representation stored is the same in both cases, the query IDs were identical. Here we fix this by always accounting for NULL nodes by recording that we saw a NULL in the jumble buffer. This fixes the issue as the order that the NULL is recorded isn't the same in the above two queries. Author: Bykov Ivan <i.bykov@modernsys.ru> Author: Michael Paquier <michael@paquier.xyz> Author: David Rowley <dgrowleyml@gmail.com> Discussion: https://postgr.es/m/aafce7966e234372b2ba876c0193f1e9%40localhost.localdomain	12 months ago
Michael Paquier	3430215fe3	pg_stat_statements: Add more tests with temp tables and namespaces ... These tests provide coverage for RangeTblEntry and how query jumbling works with search_path, as well as the case where relations are re-created, generating a different query ID as the relation OID is used in the computation. A patch is under discussion to switch to a different approach based on the relation name, and there was no test coverage for this area, including how queries are currently grouped with search_path. This is useful to track how the situation changes between HEAD and any patches proposed. Christoph has proposed the test with ON COMMIT DROP temporary tables, and I have written the second part. Author: Christoph Berg <myon@debian.org> Author: Michael Paquier <michael@paquier.xyz> Discussion: https://postgr.es/m/Z9iWXKGwkm8RAC93@msg.df7cb.de	12 months ago
Tom Lane	14e5680eee	Improve parser's reporting of statement start locations. ... Up to now, the parser's reporting of a statement's stmt_location included any preceding whitespace or comments. This isn't really desirable but was done to avoid accounting honestly for nonterminals that reduce to empty. It causes problems for pg_stat_statements, which partially compensates by manually stripping whitespace, but is not bright enough to strip /*-style comments. There will be more problems with an upcoming patch to improve reporting of errors in extension scripts, so it's time to do something about this. The thing we have to do to make it work right is to adjust YYLLOC_DEFAULT to scan the inputs of each production to find the first one that has a valid location (i.e., did not reduce to empty). In theory this adds a little bit of per-reduction overhead, but in practice it's negligible. I checked by measuring the time to run raw_parser() on the contents of information_schema.sql, and there was basically no change. Having done that, we can rely on any nonterminal that didn't reduce to completely empty to have a correct starting location, and we don't need the kluges the stmtmulti production formerly used. This should have a side benefit of allowing parse error reports to include an error position in some cases where they formerly failed to do so, due to trying to report the position of an empty nonterminal. I did not go looking for an example though. The one previously known case where that could happen (OptSchemaEltList) no longer needs the kluge it had; but I rather doubt that that was the only case. Discussion: https://postgr.es/m/ZvV1ClhnbJLCz7Sm@msg.df7cb.de	1 year ago
Alexander Korotkov	6ab1dbd26b	Add NOT NULL checking of pg_stat_statements_reset() in tests ... This is preliminary patch. It adds NOT NULL checking for the result of pg_stat_statements_reset() function. It is needed for upcoming patch "Track statement entry timestamp" that will change the result type of this function to the timestamp of a reset performed. Discussion: https://postgr.es/m/flat/72e80e7b160a6eb189df9ef6f068cce3765d37f8.camel%40moonset.ru Author: Andrei Zubkov Reviewed-by: Julien Rouhaud, Hayato Kuroda, Yuki Seino, Chengxi Sun Reviewed-by: Anton Melnikov, Darren Rush, Michael Paquier, Sergei Kornilov Reviewed-by: Alena Rybakina, Andrei Lepikhov	2 years ago
Michael Paquier	d0028e35a0	Refactor more the regression tests of pg_stat_statements ... This commit expands more the refactoring of the regression tests of pg_stat_statements, with tests moved out of pg_stat_statements.sql into separate files. The following file structure is now used: - select is mostly the former pg_stat_statements.sql, renamed. - dml for INSERT/UPDATE/DELETE and MERGE - user_activity, to test role-level checks and stat resets. - wal, to check the WAL generation after some queries. Like e8dbdb1, there is no change in terms of code coverage or results, and this finishes the split I was aiming for in these tests. Most of the tests used "test" of "pgss_test" as names for the tables used, these are renamed to less generic names. Reviewed-by: Bertrand Drouvot Discussion: https://postgr.es/m/Y/7Y9U/y/keAW3qH@paquier.xyz	3 years ago