8000 Revert support for improved tracking of nested queries · petergeoghegan/postgres@f85f6ab · GitHub
[go: up one dir, main page]
Skip to content

Commit f85f6ab

Browse files
michaelpqmichaelpq
committed
Revert support for improved tracking of nested queries
This commit reverts the two following commits: - 499edb0, track more precisely query locations for nested statements. - 06450c7, a follow-up fix of 499edb0 with query locations. The test introduced in this commit is not reverted. This is proving useful to track a problem that only pgaudit was able to detect. These prove to have issues with the tracking of SELECT statements, when these use multiple parenthesis which is something supported by the grammar. Incorrect location and lengths are causing pg_stat_statements to become confused, failing its job in query normalization with potential out-of-bound writes because the location and the length may not match with what can be handled. A lot of the query patterns discussed when this issue was reported have no test coverage in the main regression test suite, or the recovery test 027_stream_regress.pl would have caught the problems as pg_stat_statements is loaded by the node running the regression tests. A first step would be to improve the test coverage to stress more the query normalization logic. A different portion of this work was done in 45e0ba3, with the addition of tests for nested queries. These can be left in the tree. They are useful to track the way inner queries are currently tracked by PGSS with non-top-level entries, and will be useful when reconsidering in the future the work reverted here. Reported-by: Alexander Kozhemyakin <a.kozhemyakin@postgrespro.ru> Discussion: https://postgr.es/m/18947-cdd2668beffe02bf@postgresql.org
1 parent dd2ce37 commit f85f6ab

File tree

10 files changed

+119
-289
lines changed

10 files changed

+119
-289
lines changed

‎contrib/pg_overexplain/expected/pg_overexplain.out

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ EXPLAIN (DEBUG) SELECT 1;
3737
Subplans Needing Rewind: none
3838
Relation OIDs: none
3939
Executor Parameter Types: none
40-
Parse Location: 16 for 8 bytes
40+
Parse Location: 0 to end
4141
(11 rows)
4242

4343
EXPLAIN (RANGE_TABLE) SELECT 1;
@@ -119,7 +119,7 @@ $$);
119119
Subplans Needing Rewind: none
120120
Relation OIDs: NNN...
121121
Executor Parameter Types: none
122-
Parse Location: 41 to end
122+
Parse Location: 0 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>53 to end</Parse-Location> +
243+
<Parse-Location>0 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: 28 to end
347+
Parse Location: 0 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: 28 to end
375+
Parse Location: 0 to end
376376
(15 rows)
377377

378378
-- Create an index, and then attempt to force a nested loop with inner index
@@ -436,7 +436,7 @@ $$);
436436
Subplans Needing Rewind: none
437437
Relation OIDs: NNN...
438438
Executor Parameter Types: 23
439-
Parse Location: 75 for 62 bytes
439+
Parse Location: 0 to end
440440
(47 rows)
441441

442442
RESET enable_hashjoin;

‎contrib/pg_stat_statements/expected/level_tracking.out

Lines changed: 93 additions & 89 deletions
Large diffs are not rendered by default.

‎contrib/pg_stat_statements/expected/planning.out

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ SELECT 42;
5858
(1 row)
5959

6060
SELECT plans, calls, rows, query FROM pg_stat_statements
61-
WHERE query NOT LIKE 'SELECT COUNT%' ORDER BY query COLLATE "C";
61+
WHERE query NOT LIKE 'PREPARE%' ORDER BY query COLLATE "C";
6262
plans | calls | rows | query
6363
-------+-------+------+----------------------------------------------------------
6464
0 | 1 | 0 | ALTER TABLE stats_plan_test ADD COLUMN x int
@@ -72,10 +72,10 @@ SELECT plans, calls, rows, query FROM pg_stat_statements
7272
-- for the prepared statement we expect at least one replan, but cache
7373
-- invalidations could force more
7474
SELECT plans >= 2 AND plans <= calls AS plans_ok, calls, rows, query FROM pg_stat_statements
75-
WHERE query LIKE 'SELECT COUNT%' ORDER BY query COLLATE "C";
76-
plans_ok | calls | rows | query
77-
----------+-------+------+--------------------------------------
78-
t | 4 | 4 | SELECT COUNT(*) FROM stats_plan_test
75+
WHERE query LIKE 'PREPARE%' ORDER BY query COLLATE "C";
76+
plans_ok | calls | rows | query
77+
----------+-------+------+-------------------------------------------------------
78+
t | 4 | 4 | PREPARE prep1 AS SELECT COUNT(*) FROM stats_plan_test
7979
(1 row)
8080

8181
-- Cleanup

‎contrib/pg_stat_statements/expected/select.out

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,7 @@ DEALLOCATE pgss_test;
208208
SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
209209
calls | rows | query
210210
-------+------+------------------------------------------------------------------------------
211+
1 | 1 | PREPARE pgss_test (int) AS SELECT $1, $2 LIMIT $3
211212
4 | 4 | SELECT $1 +
212213
| | -- but this one will appear +
213214
| | AS "text"
@@ -221,7 +222,6 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
221222
2 | 2 | SELECT $1 AS "int" ORDER BY 1
222223
1 | 2 | SELECT $1 AS i UNION SELECT $2 ORDER BY i
223224
1 | 1 | SELECT $1 || $2
224-
1 | 1 | SELECT $1, $2 LIMIT $3
225225
2 | 2 | SELECT DISTINCT $1 AS "int"
226226
0 | 0 | SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C"
227227
1 | 1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t

‎contrib/pg_stat_statements/expected/utility.out

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -540,7 +540,7 @@ SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
540540
-------+------+----------------------------------------------------
541541
2 | 0 | DEALLOCATE $1
542542
2 | 0 | DEALLOCATE ALL
543-
2 | 2 | SELECT $1 AS a
543+
2 | 2 | PREPARE stat_select AS SELECT $1 AS a
544544
1 | 1 | SELECT $1 as a
545545
1 | 1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
546546
(5 rows)

‎contrib/pg_stat_statements/sql/planning.sql

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,11 @@ SELECT 42;
2020
SELECT 42;
2121
SELECT 42;
2222
SELECT plans, calls, rows, query FROM pg_stat_statements
23-
WHERE query NOT LIKE 'SELECT COUNT%' ORDER BY query COLLATE "C";
23+
WHERE query NOT LIKE 'PREPARE%' ORDER BY query COLLATE "C";
2424
-- for the prepared statement we expect at least one replan, but cache
2525
-- invalidations could force more
2626
SELECT plans >= 2 AND plans <= calls AS plans_ok, calls, rows, query FROM pg_stat_statements
27-
WHERE query LIKE 'SELECT COUNT%' ORDER BY query COLLATE "C";
27+
WHERE query LIKE 'PREPARE%' ORDER BY query COLLATE "C";
2828

2929
-- Cleanup
3030
DROP TABLE stats_plan_test;

‎src/backend/parser/analyze.c

Lines changed: 5 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -238,103 +238,24 @@ parse_sub_analyze(Node *parseTree, ParseState *parentParseState,
238238
return query;
239239
}
240240

241-
/*
242-
* setQueryLocationAndLength
243-
* Set query's location and length from statement and ParseState
244-
*
245-
* Some statements, like PreparableStmt, can be located within parentheses.
246-
* For example "(SELECT 1)" or "COPY (UPDATE ...) to x;". For those, we
247-
* cannot use the whole string from the statement's location or the SQL
248-
* string would yield incorrectly. The parser will set stmt_len, reflecting
249-
* the size of the statement within the parentheses. Thus, when stmt_len is
250-
* available, we need to use it for the Query's stmt_len.
251-
*
252-
* For other cases, the parser can't provide the length of individual
253-
* statements. However, we have the statement's location plus the length
254-
* (p_stmt_len) and location (p_stmt_location) of the top level RawStmt,
255-
* 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. If p_stmt_len
257-
* is 0, the SQL string is used up to its end.
258-
*/
259-
static void
260-
setQueryLocationAndLength(ParseState *pstate, Query *qry, Node *parseTree)
261-
{
262-
ParseLoc stmt_len = 0;
263-
264-
switch (nodeTag(parseTree))
265-
{
266-
case T_InsertStmt:
267-
qry->stmt_location = ((InsertStmt *) parseTree)->stmt_location;
268-
stmt_len = ((InsertStmt *) parseTree)->stmt_len;
269-
break;
270-
271-
case T_DeleteStmt:
272-
qry->stmt_location = ((DeleteStmt *) parseTree)->stmt_location;
273-
stmt_len = ((DeleteStmt *) parseTree)->stmt_len;
274-
break;
275-
276-
case T_UpdateStmt:
277-
qry->stmt_location = ((UpdateStmt *) parseTree)->stmt_location;
278-
stmt_len = ((UpdateStmt *) parseTree)->stmt_len;
279-
break;
280-
281-
case T_MergeStmt:
282-
qry->stmt_location = ((MergeStmt *) parseTree)->stmt_location;
283-
stmt_len = ((MergeStmt *) parseTree)->stmt_len;
284-
break;
285-
286-
case T_SelectStmt:
287-
qry->stmt_location = ((SelectStmt *) parseTree)->stmt_location;
288-
stmt_len = ((SelectStmt *) parseTree)->stmt_len;
289-
break;
290-
291-
case T_PLAssignStmt:
292-
qry->stmt_location = ((PLAssignStmt *) parseTree)->location;
293-
break;
294-
295-
default:
296-
qry->stmt_location = pstate->p_stmt_location;
297-
break;
298-
}
299-
300-
if (stmt_len > 0)
301-
{
302-
/* Statement's length is known, use it */
303-
qry->stmt_len = stmt_len;
304-
}
305-
else if (pstate->p_stmt_len > 0)
306-
{
307-
/*
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.
311-
*/
312-
qry->stmt_len = pstate->p_stmt_len - (qry->stmt_location - pstate->p_stmt_location);
313-
}
314-
315-
/* The calculated statement length should be calculated as positive. */
316-
Assert(qry->stmt_len >= 0);
317-
}
318-
319241
/*
320242
* transformTopLevelStmt -
321243
* transform a Parse tree into a Query tree.
322244
*
323-
* This function is just responsible for storing location data
324-
* from the RawStmt into the ParseState.
245+
* This function is just responsible for transferring statement location data
246+
* from the RawStmt into the finished Query.
325247
*/
326248
Query *
327249
transformTopLevelStmt(ParseState *pstate, RawStmt *parseTree< 93C6 /span>)
328250
{
329251
Query *result;
330252

331-
/* Store RawStmt's length and location in pstate */
332-
pstate->p_stmt_len = parseTree->stmt_len;
333-
pstate->p_stmt_location = parseTree->stmt_location;
334-
335253
/* We're at top level, so allow SELECT INTO */
336254
result = transformOptionalSelectInto(pstate, parseTree->stmt);
337255

256+
result->stmt_location = parseTree->stmt_location;
257+
result->stmt_len = parseTree->stmt_len;
258+
338259
return result;
339260
}
340261

@@ -503,7 +424,6 @@ transformStmt(ParseState *pstate, Node *parseTree)
503424
/* Mark as original query until we learn differently */
504425
result->querySource = QSRC_ORIGINAL;
505426
result->canSetTag = true;
506-
setQueryLocationAndLength(pstate, result, parseTree);
507427

508428
return result;
509429
}

0 commit comments

Comments
 (0)
0