git.postgresql.org Git - postgresql.git/commitdiff

git projects / postgresql.git / commitdiff

Fix possible crash during WindowAgg evaluation

author David Rowley <drowley@postgresql.org>

Mon, 9 Dec 2024 01:23:21 +0000 (14:23 +1300)

committer David Rowley <drowley@postgresql.org>

Mon, 9 Dec 2024 01:23:21 +0000 (14:23 +1300)

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

src/backend/executor/nodeWindowAgg.c patch | blob | blame | history

src/test/regress/expected/window.out patch | blob | blame | history

src/test/regress/sql/window.sql patch | blob | blame | history

diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c

index e75e8576725715ee19799b0cd96d5a0e3a6ee6da..70a7025818f7c0497de57ef1349a70c7640f3c34 100644 (file)

--- a/src/backend/executor/nodeWindowAgg.c

+++ b/src/backend/executor/nodeWindowAgg.c

@@ -2353,6 +2353,23 @@ ExecWindowAgg(PlanState *pstate)

if (winstate->use_pass_through)

{

+ /*

+ * When switching into a pass-through mode, we'd better

+ * NULLify the aggregate results as these are no longer

+ * updated and NULLifying them avoids the old stale

+ * results lingering. Some of these might be byref types

+ * so we can't have them pointing to free'd memory. The

+ * planner insisted that quals used in the runcondition

+ * are strict, so the top-level WindowAgg will always

+ * filter these NULLs out in the filter clause.

+ */

+ numfuncs = winstate->numfuncs;

+ for (i = 0; i < numfuncs; i++)

+ {

+ econtext->ecxt_aggvalues[i] = (Datum) 0;

+ econtext->ecxt_aggnulls[i] = true;

+ }

* STRICT pass-through mode is required for the top window

* when there is a PARTITION BY clause. Otherwise we must

@@ -2367,24 +2384,6 @@ ExecWindowAgg(PlanState *pstate)

else

{

winstate->status = WINDOWAGG_PASSTHROUGH;

- /*

- * If we're not the top-window, we'd better NULLify

- * the aggregate results. In pass-through mode we no

- * longer update these and this avoids the old stale

- * results lingering. Some of these might be byref

- * types so we can't have them pointing to free'd

- * memory. The planner insisted that quals used in

- * the runcondition are strict, so the top-level

- * WindowAgg will filter these NULLs out in the filter

- * clause.

- */

- numfuncs = winstate->numfuncs;

- for (i = 0; i < numfuncs; i++)

- {

- econtext->ecxt_aggvalues[i] = (Datum) 0;

- econtext->ecxt_aggnulls[i] = true;

- }

}

else

diff --git a/src/test/regress/expected/window.out b/src/test/regress/expected/window.out

index 8b447aa01e59fd740adae7e97c1808a14031fcf1..23d1463df22ba790132e9012ffb2256a22c75960 100644 (file)

--- a/src/test/regress/expected/window.out

+++ b/src/test/regress/expected/window.out

@@ -4136,6 +4136,16 @@ WHERE c = 1;

-> Seq Scan on empsalary

(3 rows)

+-- Try another case with a WindowFunc with a byref return type

+SELECT * FROM

+ (SELECT row_number() OVER (PARTITION BY salary) AS rn,

+ lead(depname) OVER (PARTITION BY salary) || ' Department' AS n_dep

+ FROM empsalary) emp

+WHERE rn < 1;

+ rn | n_dep

+----+-------

+(0 rows)

-- Some more complex cases with multiple window clauses

EXPLAIN (COSTS OFF)

SELECT * FROM

diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql

index 6de5493b05bb281d4f8fbd5ba0af9239a42965f6..02f105f070e8b419b9884b74d853139a6ffe43db 100644 (file)

--- a/src/test/regress/sql/window.sql

+++ b/src/test/regress/sql/window.sql

@@ -1345,6 +1345,13 @@ SELECT * FROM

FROM empsalary) emp

WHERE c = 1;