8 Commits (0f4f45772c5a87ca2f228ee3bea36d313dca0b51)

Author	SHA1	Message	Date
Michael Paquier	06450c7b8c	Fix regression with location calculation of nested statements ... The statement location calculated for some nested query cases was wrong when multiple queries are sent as a single string, these being separated by semicolons. As pointed by Sami Imseih, the location calculation was incorrect when the last query of nested statement with multiple queries does **NOT** finish with a semicolon for the last statement. In this case, the statement length tracked by RawStmt is 0, which is equivalent to say that the string should be used until its end. The code previously discarded this case entirely, causing the location to remain at 0, the same as pointing at the beginning of the string. This caused pg_stat_statements to store incorrect query strings. This issue has been introduced in 499edb0974. I have looked at the diffs generated by pgaudit back then, and noticed the difference generated for this nested query case, but I have missed the point that it was an actual regression with an existing case. A test case is added in pg_stat_statements to provide some coverage, restoring the pre-17 behavior for the calculation of the query locations. Special thanks to David Steele, who, through an analysis of the test diffs generated by pgaudit with the new v18 logic, has poked me about the fact that my original analysis of the matter was wrong. The test output of pg_overexplain is updated to reflect the new logic, as the new locations refer to the beginning of the argument passed to the function explain_filter(). When the module was introduced in 8d5ceb113e, which was after 499edb0974 (for the new calculation method), the locations of the test were not actually right: the plan generated for the query string given in input of the function pointed to the top-level query, not the nested one. Reported-by: David Steele <david@pgbackrest.org> Author: Michael Paquier <michael@paquier.xyz> Reviewed-by: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com> Reviewed-by: Jian He <jian.universality@gmail.com> Reviewed-by: Sami Imseih <samimseih@gmail.com> Reviewed-by: David Steele <david@pgbackrest.org> Discussion: https://postgr.es/m/844a3b38-bbf1-4fb2-9fd6-f58c35c09917@pgbackrest.org	7 months ago
David Rowley	89988ac589	Fix further fallout from EXPLAIN ANALYZE BUFFERS change ... c2a4078eb adjusted EXPLAIN ANALYZE to default the BUFFERS to ON. This (hopefully) fixes the last remaining issue with regression test failures with -D RELCACHE_FORCE_RELEASE -D CATCACHE_FORCE_RELEASE builds, where the planner accesses more buffers due to the cold caches. Discussion: https://postgr.es/m/CAApHDvqLdzgz77JsE-yTki3w9UiKQ-uTMLRctazcu+99-ips3g@mail.gmail.com	1 year ago
Michael Paquier	45e0ba30fc	pg_stat_statements: Add tests for nested queries with level tracking ... There have never been any regression tests in PGSS for various query patterns for nested queries combined with level tracking, like: - Multi-statements. - CREATE TABLE AS - CREATE/REFRESH MATERIALIZED VIEW - DECLARE CURSOR - EXPLAIN, with a subset of the above supported. - COPY. All the tests added here track historical, sometimes confusing, existing behaviors. For example, EXPLAIN stores two PGSS entries with the same top-level query string but two different query IDs as one is calculated for the top-level EXPLAIN (this part is right) and a second one for the inner query in the EXPLAIN (this part is not right). A couple of patches are under discussion to improve the situation, and all the tests added here will prove useful to evaluate the changes discussed. Author: Anthonin Bonnefoy Reviewed-by: Michael Paquier, Jian He Discussion: https://postgr.es/m/CAO6_XqqM6S9bQ2qd=75W+yKATwoazxSNhv5sjW06fjGAtHbTUA@mail.gmail.com	1 year ago
Michael Paquier	c145f321b6	Propagate query IDs of utility statements in functions ... For utility statements defined within a function, the query tree is copied to a PlannedStmt as utility commands do not require planning. However, the query ID was missing from the information passed down. This leads to plugins relying on the query ID like pg_stat_statements to not be able to track utility statements within function calls. Tests are added to check this behavior, depending on pg_stat_statements.track. This is an old bug. Now, query IDs for utilities are compiled using their parsed trees rather than the query string since v16 (3db72ebcbe), leading to less bloat with utilities, so backpatch down only to this version. Author: Anthonin Bonnefoy Discussion: https://postgr.es/m/CAO6_XqrGp-uwBqi3vBPLuRULKkddjC7R5QZCgsFren=8E+m2Sg@mail.gmail.com Backpatch-through: 16	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
Tom Lane	76db9cb636	Fix some issues with tracking nesting level in pg_stat_statements. ... When we decide that we don't want to track execution time of a specific planner or ProcessUtility call, we still have to increment the nesting depth, or we'll make the wrong determination of whether we are at top level when considering nested statements. (PREPARE and EXECUTE are exceptions, for reasons explained in the code.) Counting planner nesting depth separately from executor nesting depth was a mistake: it causes us to make the wrong determination of whether we are at top level when considering nested statements that get executed during planning (as a result of constant-folding of functions, for example). Merge those counters into one. In passing, get rid of the PGSS_HANDLED_UTILITY macro in favor of explicitly listing statement types. It seems somewhat coincidental that PREPARE and EXECUTE are handled alike in each of the places where that was used: the reasoning tends to be different for each one. Thus, the macro seems as likely to encourage future bugs as prevent them, since it's quite unclear whether any future statement type that might need special-casing here would also need the same choices at each spot. Sergei Kornilov, Julien Rouhaud, and Tom Lane, per bug #17552 from Maxim Boguk. This is pretty clearly a bug fix, but it's also a behavioral change that might surprise somebody, so no back-patch. Discussion: https://postgr.es/m/17552-213b534c56ab5d02@postgresql.org	2 years ago
Michael Paquier	9a714b9d6e	Improve cleanup phases in regression tests of pg_stat_statements ... As shaped, two DROP ROLE queries included in "user_activity" were showing in the reports for "wal". The intention is to keep each test isolated and independent, so this is incorrect. This commit adds some calls to pg_stat_statements_reset() to clean up the statistics once each test finishes, so as there are no risks of overlap in the reports for individial scenarios. The addition in "user_activity" fixes the output of "wal". The new resets done in "level_tracking" and "utility" are added for consistency with the rest, though they do not affect the stats generated in the other tests. Oversight in d0028e3. Reported-by: Andrei Zubkov Discussion: https://postgr.es/m/7beb722dd016bf54f1c78bfd6d44a684e28da624.camel@moonset.ru	3 years ago
Michael Paquier	e8dbdb15db	Refactor tests of pg_stat_statements for planning, utility and level tracking ... pg_stat_statements.sql acts as the main file for all the core tests of the module, but things have become complicated to follow over the years as some of the sub-scenarios tested in this file rely on assumptions that come from completely different areas of it, like a GUC setup or a relation created previously. For example, row tracking for CTAS/COPY was looking at the number of plans, which was not necessary, or level tracking was mixed with checks on planner counts. This commit refactors the tests of pg_stat_statements, by moving test cases out of pg_stat_statements.sql into their own file, as of: - Planning-related tests in planning.sql, for [re]plan counts and top-level handling. These depend on pg_stat_statements.track_planning. - Utilities in utility.sql (pg_stat_statements.track_utility), that includes now the tests for: -- Row tracking for CTAS, CREATE MATERIALIZED VIEW, COPY. -- Basic utility statements. -- SET statements. - Tracking level, depending on pg_stat_statements.track. This part has been looking at scenarios with DO blocks, PL functions and SQL functions. pg_stat_statements.sql (still named the same for now) still includes some checks for role-level tracking and WAL generation metrics, that ought to become independent in the long term for clarity. While on it, this switches the order of the attributes when querying pg_stat_statements, the query field becoming last. This makes much easier the tracking of changes related to normalization, as queries are the only variable-length attributes queried (unaligned mode would be one extra choice, but that reduces the checks on the other fields). Test scenarios and their results match exactly with what was happening before this commit in terms of calls, number of plans, number of rows, cached data or level tracking, so this has no effect on the coverage in terms of what is produced by the reports in the table pg_stat_statements. A follow-up patch will extend more the tests of pg_stat_statements around utilities, so this split creates a foundation for this purpose, without complicating more pg_stat_statements.sql. Reviewed-by: Bertrand Drouvot Discussion: https://postgr.es/m/Y+MRdEq9W9XVa2AB@paquier.xyz	3 years ago