Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 1fe5a34

Browse files
committed
Fix possible crash during WindowAgg evaluation
When short-circuiting WindowAgg node evaluation on the top-level WindowAgg node using quals on monotonic window functions, because the WindowAgg run condition can mean there's no need to evaluate subsequent window function results in the same partition once the run condition becomes false, it was possible that the executor would use stale results from the previous invocation of the window function in some cases. A fix for this was partially done by a5832722, but that commit only fixed the issue for non-top-level WindowAgg nodes. I mistakenly thought that the top-level WindowAgg didn't have this issue, but Jayesh's example case clearly shows that's incorrect. At the time, I also thought that this only affected 32-bit systems as all window functions which then supported run conditions returned BIGINT, however, that's wrong as ExecProject is still called and that could cause evaluation of any other window function belonging to the same WindowAgg node, one of which may return a byref type. The only queries affected by this are WindowAggs with a "Run Condition" which contains at least one window function with a byref result type, such as lead() or lag() on a byref column. The window clause must also contain a PARTITION BY clause (without a PARTITION BY, execution of the WindowAgg stops immediately when the run condition becomes false and there's no risk of using the stale results). Reported-by: Jayesh Dehankar Discussion: https://postgr.es/m/193261e2c4d.3dd3cd7c1842.871636075166132237@zohocorp.com Backpatch-through: 15, where WindowAgg run conditions were added
1 parent 3f9b962 commit 1fe5a34

File tree

3 files changed

+34
-18
lines changed

3 files changed

+34
-18
lines changed

src/backend/executor/nodeWindowAgg.c

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2353,6 +2353,23 @@ ExecWindowAgg(PlanState *pstate)
23532353
*/
23542354
if (winstate->use_pass_through)
23552355
{
2356+
/*
2357+
* When switching into a pass-through mode, we'd better
2358+
* NULLify the aggregate results as these are no longer
2359+
* updated and NULLifying them avoids the old stale
2360+
* results lingering. Some of these might be byref types
2361+
* so we can't have them pointing to free'd memory. The
2362+
* planner insisted that quals used in the runcondition
2363+
* are strict, so the top-level WindowAgg will always
2364+
* filter these NULLs out in the filter clause.
2365+
*/
2366+
numfuncs = winstate->numfuncs;
2367+
for (i = 0; i < numfuncs; i++)
2368+
{
2369+
econtext->ecxt_aggvalues[i] = (Datum) 0;
2370+
econtext->ecxt_aggnulls[i] = true;
2371+
}
2372+
23562373
/*
23572374
* STRICT pass-through mode is required for the top window
23582375
* when there is a PARTITION BY clause. Otherwise we must
@@ -2367,24 +2384,6 @@ ExecWindowAgg(PlanState *pstate)
23672384
else
23682385
{
23692386
winstate->status = WINDOWAGG_PASSTHROUGH;
2370-
2371-
/*
2372-
* If we're not the top-window, we'd better NULLify
2373-
* the aggregate results. In pass-through mode we no
2374-
* longer update these and this avoids the old stale
2375-
* results lingering. Some of these might be byref
2376-
* types so we can't have them pointing to free'd
2377-
* memory. The planner insisted that quals used in
2378-
* the runcondition are strict, so the top-level
2379-
* WindowAgg will filter these NULLs out in the filter
2380-
* clause.
2381-
*/
2382-
numfuncs = winstate->numfuncs;
2383-
for (i = 0; i < numfuncs; i++)
2384-
{
2385-
econtext->ecxt_aggvalues[i] = (Datum) 0;
2386-
econtext->ecxt_aggnulls[i] = true;
2387-
}
23882387
}
23892388
}
23902389
else

src/test/regress/expected/window.out

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4136,6 +4136,16 @@ WHERE c = 1;
41364136
-> Seq Scan on empsalary
41374137
(3 rows)
41384138

4139+
-- Try another case with a WindowFunc with a byref return type
4140+
SELECT * FROM
4141+
(SELECT row_number() OVER (PARTITION BY salary) AS rn,
4142+
lead(depname) OVER (PARTITION BY salary) || ' Department' AS n_dep
4143+
FROM empsalary) emp
4144+
WHERE rn < 1;
4145+
rn | n_dep
4146+
----+-------
4147+
(0 rows)
4148+
41394149
-- Some more complex cases with multiple window clauses
41404150
EXPLAIN (COSTS OFF)
41414151
SELECT * FROM

src/test/regress/sql/window.sql

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1345,6 +1345,13 @@ SELECT * FROM
13451345
FROM empsalary) emp
13461346
WHERE c = 1;
13471347

1348+
-- Try another case with a WindowFunc with a byref return type
1349+
SELECT * FROM
1350+
(SELECT row_number() OVER (PARTITION BY salary) AS rn,
1351+
lead(depname) OVER (PARTITION BY salary) || ' Department' AS n_dep
1352+
FROM empsalary) emp
1353+
WHERE rn < 1;
1354+
13481355
-- Some more complex cases with multiple window clauses
13491356
EXPLAIN (COSTS OFF)
13501357
SELECT * FROM

0 commit comments

Comments
 (0)