Re: Row pattern recognition

Hi Henson,

> Hi Tatsuo, Jian,
>
> I think there's a correctness problem in the RPR patch: a window function's
> result can change depending on which other, unrelated window functions are
> in the same query.
>
> Pattern matching only advances when a window function reads the frame.
> nth_value(x, n) returns NULL without reading the frame when n is NULL
> (correct per the standard), so if it is the only window function in an RPR
> window, the match never advances over those rows and the reduced frame no
> longer matches a full scan.

Ouch.

> Example -- one partition, 60 rows, price = id * 10:
>
> CREATE TABLE rpr_dormant (id int, price int);
> INSERT INTO rpr_dormant SELECT g, g*10 FROM generate_series(1,60) g;
>
> SELECT id, nth_value(price, CASE WHEN id < 50 THEN NULL ELSE 1 END) OVER w
> FROM rpr_dormant
> WINDOW w AS (
> ORDER BY id
> ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
> AFTER MATCH SKIP PAST LAST ROW
> PATTERN (A+)
> DEFINE A AS price > PREV(FIRST(price), 50)
> );
>
> Run alone, the nth_value column does not follow the actual match structure;
> adding an unrelated first_value(id) OVER w, which reads the frame every row,
> changes it. And while the match is dormant the mark position keeps
> advancing, running ahead and trimming rows that the backward navigation
> later needs -- so the same query can instead fail with "cannot fetch row N
> before WindowObject's mark position".
>
> I think an RPR window should perform the match for each row up front,
> building its reduced frame during the row scan before the window functions
> are evaluated, regardless of whether any function reads the frame. The fix
> belongs in the executor, not in nth_value -- the early return is standard,
> and the same gap is reachable from any user-defined function that skips the
> frame.
>
> Does this direction seem right, or is the lazy, frame-driven matching
> intentional in a way I'm missing? Happy to prepare a patch.

Yes, I think the direction is correct. Probably the patch would someting like this?

diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index cb6a484b7de..b6c12096c85 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -2523,6 +2523,9 @@ ExecWindowAgg(PlanState *pstate)
{
if (winstate->rpSkipTo == ST_NEXT_ROW)
clear_reduced_frame(winstate);
+
+ update_reduced_frame(winstate->nav_winobj,
+ winstate->frameheadpos);
}

/*

> is the lazy, frame-driven matching
> intentional in a way I'm missing?

Not intentional.

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp