Skip to content

Commit 06450c7

Browse files
michaelpqmichaelpq
committed
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 499edb0. 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 8d5ceb1, which was after 499edb0 (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
1 parent a6060f1 commit 06450c7

File tree

4 files changed

+87
-15
lines changed

4 files changed

+87
-15
lines changed

‎contrib/pg_overexplain/expected/pg_overexplain.out

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ $$);
119119
Subplans Needing Rewind: none
120120
Relation OIDs: NNN...
121121
Executor Parameter Types: none
122-
Parse Location: 0 to end
122+
Parse Location: 41 to end
123123
RTI 1 (relation, inherited, in-from-clause):
124124
Eref: vegetables (id, name, genus)
125125
Relation: vegetables
@@ -240,7 +240,7 @@ $$);
240240
<Subplans-Needing-Rewind>none</Subplans-Needing-Rewind> +
241241
<Relation-OIDs>NNN...</Relation-OIDs> +
242242
<Executor-Parameter-Types>none</Executor-Parameter-Types> +
243-
<Parse-Location>0 to end</Parse-Location> +
243+
<Parse-Location>53 to end</Parse-Location> +
244244
</PlannedStmt> +
245245
<Range-Table> +
246246
<Range-Table-Entry> +
@@ -344,7 +344,7 @@ $$);
344344
Subplans Needing Rewind: none
345345
Relation OIDs: NNN...
346346
Executor Parameter Types: none
347-
Parse Location: 0 to end
347+
Parse Location: 28 to end
348348
(37 rows)
349349

350350
SET debug_parallel_query = false;
@@ -372,7 +372,7 @@ $$);
372372
Subplans Needing Rewind: none
373373
Relation OIDs: NNN...
374374
Executor Parameter Types: 0
375-
Parse Location: 0 to end
375+
Parse Location: 28 to end
376376
(15 rows)
377377

378378
-- Create an index, and then attempt to force a nested loop with inner index

‎contrib/pg_stat_statements/expected/level_tracking.out

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1319,6 +1319,57 @@ SELECT toplevel, calls, query FROM pg_stat_statements
13191319
t | 1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
13201320
(4 rows)
13211321

1322+
-- DO block --- multiple inner queries with separators
1323+
SET pg_stat_statements.track = 'all';
1324+
SET pg_stat_statements.track_utility = TRUE;
1325+
CREATE TABLE pgss_do_util_tab_1 (a int);
1326+
CREATE TABLE pgss_do_util_tab_2 (a int);
1327+
SELECT pg_stat_statements_reset() IS NOT NULL AS t;
1328+
t
1329+
---
1330+
t
1331+
(1 row)
1332+
1333+
DO $$
1334+
DECLARE BEGIN
1335+
EXECUTE 'CREATE TABLE pgss_do_table (id INT); DROP TABLE pgss_do_table';
1336+
EXECUTE 'SELECT a FROM pgss_do_util_tab_1; SELECT a FROM pgss_do_util_tab_2';
1337+
END $$;
1338+
SELECT toplevel, calls, rows, query FROM pg_stat_statements
1339+
WHERE toplevel IS FALSE
1340+
ORDER BY query COLLATE "C";
1341+
toplevel | calls | rows | query
1342+
----------+-------+------+-------------------------------------
1343+
f | 1 | 0 | CREATE TABLE pgss_do_table (id INT)
1344+
f | 1 | 0 | DROP TABLE pgss_do_table
1345+
f | 1 | 0 | SELECT a FROM pgss_do_util_tab_1
1346+
f | 1 | 0 | SELECT a FROM pgss_do_util_tab_2
1347+
(4 rows)
1348+
1349+
SELECT pg_stat_statements_reset() IS NOT NULL AS t;
1350+
t
1351+
---
1352+
t
1353+
(1 row)
1354+
1355+
-- Note the extra semicolon at the end of the query.
1356+
DO $$
1357+
DECLARE BEGIN
1358+
EXECUTE 'CREATE TABLE pgss_do_table (id INT); DROP TABLE pgss_do_table;';
1359+
EXECUTE 'SELECT a FROM pgss_do_util_tab_1; SELECT a FROM pgss_do_util_tab_2;';
1360+
END $$;
1361+
SELECT toplevel, calls, rows, query FROM pg_stat_statements
1362+
WHERE toplevel IS FALSE
1363+
ORDER BY query COLLATE "C";
1364+
toplevel | calls | rows | query
1365+
----------+-------+------+-------------------------------------
1366+
f | 1 | 0 | CREATE TABLE pgss_do_table (id INT)
1367+
f | 1 | 0 | DROP TABLE pgss_do_table
1368+
f | 1 | 0 | SELECT a FROM pgss_do_util_tab_1
1369+
f | 1 | 0 | SELECT a FROM pgss_do_util_tab_2
1370+
(4 rows)
1371+
1372+
DROP TABLE pgss_do_util_tab_1, pgss_do_util_tab_2;
13221373
-- PL/pgSQL function - top-level tracking.
13231374
SET pg_stat_statements.track = 'top';
13241375
SET pg_stat_statements.track_utility = FALSE;

‎contrib/pg_stat_statements/sql/level_tracking.sql

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,32 @@ END; $$;
334334
SELECT toplevel, calls, query FROM pg_stat_statements
335335
ORDER BY query COLLATE "C", toplevel;
336336

337+
-- DO block --- multiple inner queries with separators
338+
SET pg_stat_statements.track = 'all';
339+
SET pg_stat_statements.track_utility = TRUE;
340+
CREATE TABLE pgss_do_util_tab_1 (a int);
341+
CREATE TABLE pgss_do_util_tab_2 (a int);
342+
SELECT pg_stat_statements_reset() IS NOT NULL AS t;
343+
DO $$
344+
DECLARE BEGIN
345+
EXECUTE 'CREATE TABLE pgss_do_table (id INT); DROP TABLE pgss_do_table';
346+
EXECUTE 'SELECT a FROM pgss_do_util_tab_1; SELECT a FROM pgss_do_util_tab_2';
347+
END $$;
348+
SELECT toplevel, calls, rows, query FROM pg_stat_statements
349+
WHERE toplevel IS FALSE
350+
ORDER BY query COLLATE "C";
351+
SELECT pg_stat_statements_reset() IS NOT NULL AS t;
352+
-- Note the extra semicolon at the end of the query.
353+
DO $$
354+
DECLARE BEGIN
355+
EXECUTE 'CREATE TABLE pgss_do_table (id INT); DROP TABLE pgss_do_table;';
356+
EXECUTE 'SELECT a FROM pgss_do_util_tab_1; SELECT a FROM pgss_do_util_tab_2;';
357+
END $$;
358+
SELECT toplevel, calls, rows, query FROM pg_stat_statements
359+
WHERE toplevel IS FALSE
360+
ORDER BY query COLLATE "C";
361+
DROP TABLE pgss_do_util_tab_1, pgss_do_util_tab_2;
362+
337363
-- PL/pgSQL function - top-level tracking.
338364
SET pg_stat_statements.track = 'top';
339365
SET pg_stat_statements.track_utility = FALSE;

‎src/backend/parser/analyze.c

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -253,20 +253,14 @@ parse_sub_analyze(Node *parseTree, ParseState *parentParseState,
253253
* statements. However, we have the statement's location plus the length
254254
* (p_stmt_len) and location (p_stmt_location) of the top level RawStmt,
255255
* stored in pstate. Thus, the statement's length is the RawStmt's length
256-
* minus how much we've advanced in the RawStmt's string.
256+
* minus how much we've advanced in the RawStmt's string. If p_stmt_len
257+
* is 0, the SQL string is used up to its end.
257258
*/
258259
static void
259260
setQueryLocationAndLength(ParseState *pstate, Query *qry, Node *parseTree)
260261
{
261262
ParseLoc stmt_len = 0;
262263

263-
/*
264-
* If there is no information about the top RawStmt's length, leave it at
265-
* 0 to use the whole string.
266-
*/
267-
if (pstate->p_stmt_len == 0)
268-
return;
269-
270264
switch (nodeTag(parseTree))
271265
{
272266
case T_InsertStmt:
@@ -308,11 +302,12 @@ setQueryLocationAndLength(ParseState *pstate, Query *qry, Node *parseTree)
308302
/* Statement's length is known, use it */
309303
qry->stmt_len = stmt_len;
310304
}
311-
else
305+
else if (pstate->p_stmt_len > 0)
312306
{
313307
/*
314-
* Compute the statement's length from the statement's location and
315-
* the RawStmt's length and location.
308+
* The top RawStmt's length is known, so calculate the statement's
309+
* length from the statement's location and the RawStmt's length and
310+
* location.
316311
*/
317312
qry->stmt_len = pstate->p_stmt_len - (qry->stmt_location - pstate->p_stmt_location);
318313
}

0 commit comments

Comments
 (0)