From b5c6f60d4c64eea8b90b7e3b2506fb7b2b1f117e Mon Sep 17 00:00:00 2001 From: Henson Choi Date: Mon, 9 Nov 2026 09:45:27 +0900 Subject: [PATCH v2 8/8] Advance the window mark when a frame accessor finds no row. When the frame is empty or the requested row is outside it, WinGetFuncArgInFrame() and ignorenulls_getfuncarginframe() return without moving the mark, even if the caller asked them to set it. The tuplestore then keeps every row since the last successful call, so a long partition spills to disk although the frame is small, e.g. first_value() over a one-row frame emptied by EXCLUDE CURRENT ROW, or nth_value() beyond the frame length. Move the mark to the frame start in that case, but not past the last row read. This relies on no frame access reading a row before the frame start. The EXCLUDE TIES fix earlier in this series removed the one that did, and the WINDOW_SEEK_TAIL branch now checks the frame start also when there is no exclusion. The WINDOW_SEEK_TAIL branch of ignorenulls_getfuncarginframe() set the mark to 0, which would now fail with "cannot move WindowObject's mark position backward". Use the frame start instead; the backward walk stops there. The storage shown by the tests of the previous commit changes from disk to memory. Author: Henson Choi Discussion: https://postgr.es/m/CAHGQGwFTmdD5T+7z8kZekrjVkA=Z2yNhq43ff3AuoxPVxDXP1A@mail.gmail.com --- src/backend/executor/nodeWindowAgg.c | 49 +++++++++++++++++++++++---- src/test/regress/expected/explain.out | 10 +++--- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c index 49f2d268dcb..708ce5d336a 100644 --- a/src/backend/executor/nodeWindowAgg.c +++ b/src/backend/executor/nodeWindowAgg.c @@ -212,6 +212,7 @@ static bool are_peers(WindowAggState *winstate, TupleTableSlot *slot1, static bool window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot); +static void advance_mark_to_framehead(WindowObject winobj); static Datum ignorenulls_getfuncarginframe(WindowObject winobj, int argno, int relpos, int seektype, bool set_mark, bool *isnull, @@ -3399,6 +3400,32 @@ gettuple_eval_partition(WindowObject winobj, int argno, econtext, isnull); } +/* + * advance_mark_to_framehead + * Advance the mark of a WindowObject to the current frame start, if it + * is behind it. + * + * Used when a frame accessor asked to set the mark finds no row to return. + * The frame start is a position before which no later frame access fetches, + * so the rows before it can be released by tuplestore_trim() even though no + * row was fetched. Don't move the mark past the last row read so far, + * though: the read pointers would reach the end of the tuplestore and lose + * their place once more rows are appended. + */ +static void +advance_mark_to_framehead(WindowObject winobj) +{ + WindowAggState *winstate = winobj->winstate; + int64 markpos; + + update_frameheadpos(winstate); + + /* the current row has been read, so spooled_rows is at least 1 */ + markpos = Min(winstate->frameheadpos, winstate->spooled_rows - 1); + if (markpos > winobj->markpos) + WinSetMarkPosition(winobj, markpos); +} + /* * ignorenulls_getfuncarginframe * For IGNORE NULLS, get the next nonnull value in the frame, moving forward @@ -3448,8 +3475,9 @@ ignorenulls_getfuncarginframe(WindowObject winobj, int argno, goto out_of_frame; update_frametailpos(winstate); abs_pos = winstate->frametailpos - 1; + /* the backward walk never fetches a row before the frame start */ update_frameheadpos(winstate); - mark_pos = 0; /* keep compiler quiet */ + mark_pos = winstate->frameheadpos; forward = -1; break; default: @@ -3584,6 +3612,8 @@ advance: return datum; out_of_frame: + if (set_mark) + advance_mark_to_framehead(winobj); if (isout) *isout = true; *isnull = true; @@ -4054,8 +4084,11 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno, * argno: argument number to evaluate (counted from 0) * relpos: signed rowcount offset from the seek position * seektype: WINDOW_SEEK_HEAD or WINDOW_SEEK_TAIL - * set_mark: If the row is found/in frame and set_mark is true, the mark is - * moved to the row as a side-effect. + * set_mark: If true, the mark is moved forward as a side-effect, to a + * position before which no later frame access will fetch. That is + * the row returned if there is no exclusion clause and nulls are + * respected, and otherwise a position no later than the frame start. + * The mark is moved even if no row is found. * isnull: output argument, receives isnull status of result * isout: output argument, set to indicate whether target row position * is out of frame (can pass NULL if caller doesn't care about this) @@ -4177,21 +4210,23 @@ WinGetFuncArgInFrame(WindowObject winobj, int argno, * exclusion, we can safely set the mark at the accessed row. But * if there is, we can only mark the frame start, because we can't * be sure how far back in the frame the exclusion might cause us - * to fetch in future. Furthermore, we have to actually check + * to fetch in future. Either way, we have to actually check * against frameheadpos here, since it's unsafe to try to fetch a * row before frame start if the mark might be there already. */ + update_frameheadpos(winstate); switch (winstate->frameOptions & FRAMEOPTION_EXCLUSION) { case 0: /* no adjustment needed */ + if (abs_pos < winstate->frameheadpos) + goto out_of_frame; mark_pos = abs_pos; break; case FRAMEOPTION_EXCLUDE_CURRENT_ROW: if (abs_pos <= winstate->currentpos && winstate->currentpos < winstate->frametailpos) abs_pos--; - update_frameheadpos(winstate); if (abs_pos < winstate->frameheadpos) goto out_of_frame; mark_pos = winstate->frameheadpos; @@ -4206,7 +4241,6 @@ WinGetFuncArgInFrame(WindowObject winobj, int argno, abs_pos -= overlapend - winstate->groupheadpos; } - update_frameheadpos(winstate); if (abs_pos < winstate->frameheadpos) goto out_of_frame; mark_pos = winstate->frameheadpos; @@ -4229,7 +4263,6 @@ WinGetFuncArgInFrame(WindowObject winobj, int argno, else abs_pos -= overlapend - winstate->groupheadpos; } - update_frameheadpos(winstate); if (abs_pos < winstate->frameheadpos) goto out_of_frame; mark_pos = winstate->frameheadpos; @@ -4263,6 +4296,8 @@ WinGetFuncArgInFrame(WindowObject winobj, int argno, econtext, isnull); out_of_frame: + if (set_mark) + advance_mark_to_framehead(winobj); if (isout) *isout = true; *isnull = true; diff --git a/src/test/regress/expected/explain.out b/src/test/regress/expected/explain.out index 45e1096593a..5054e6d75d3 100644 --- a/src/test/regress/expected/explain.out +++ b/src/test/regress/expected/explain.out @@ -839,7 +839,7 @@ select explain_filter('explain (analyze,buffers off,costs off) select first_valu ---------------------------------------------------------------------------------- WindowAgg (actual time=N.N..N.N rows=N.N loops=N) Window: w1 AS (ROWS BETWEEN CURRENT ROW AND CURRENT ROW EXCLUDE CURRENT ROW) - Storage: Disk Maximum Storage: NkB + Storage: Memory Maximum Storage: NkB -> Function Scan on generate_series a (actual time=N.N..N.N rows=N.N loops=N) Planning Time: N.N ms Execution Time: N.N ms @@ -850,7 +850,7 @@ select explain_filter('explain (analyze,buffers off,costs off) select nth_value( ---------------------------------------------------------------------------------- WindowAgg (actual time=N.N..N.N rows=N.N loops=N) Window: w1 AS (ROWS BETWEEN CURRENT ROW AND 'N'::bigint FOLLOWING) - Storage: Disk Maximum Storage: NkB + Storage: Memory Maximum Storage: NkB -> Function Scan on generate_series a (actual time=N.N..N.N rows=N.N loops=N) Planning Time: N.N ms Execution Time: N.N ms @@ -863,7 +863,7 @@ select explain_filter('explain (analyze,buffers off,costs off) select last_value ---------------------------------------------------------------------------------- WindowAgg (actual time=N.N..N.N rows=N.N loops=N) Window: w1 AS (ROWS BETWEEN CURRENT ROW AND 'N'::bigint FOLLOWING) - Storage: Disk Maximum Storage: NkB + Storage: Memory Maximum Storage: NkB -> Function Scan on generate_series a (actual time=N.N..N.N rows=N.N loops=N) Planning Time: N.N ms Execution Time: N.N ms @@ -876,7 +876,7 @@ select explain_filter('explain (analyze,buffers off,costs off) select first_valu ---------------------------------------------------------------------------------- WindowAgg (actual time=N.N..N.N rows=N.N loops=N) Window: w1 AS (ROWS BETWEEN CURRENT ROW AND CURRENT ROW EXCLUDE CURRENT ROW) - Storage: Disk Maximum Storage: NkB + Storage: Memory Maximum Storage: NkB -> Function Scan on generate_series a (actual time=N.N..N.N rows=N.N loops=N) Planning Time: N.N ms Execution Time: N.N ms @@ -887,7 +887,7 @@ select explain_filter('explain (analyze,buffers off,costs off) select last_value ---------------------------------------------------------------------------------- WindowAgg (actual time=N.N..N.N rows=N.N loops=N) Window: w1 AS (ROWS BETWEEN CURRENT ROW AND 'N'::bigint FOLLOWING) - Storage: Disk Maximum Storage: NkB + Storage: Memory Maximum Storage: NkB -> Function Scan on generate_series a (actual time=N.N..N.N rows=N.N loops=N) Planning Time: N.N ms Execution Time: N.N ms -- 2.54.0 (Apple Git-157)