Re: Row pattern recognition - Mailing list pgsql-hackers

Hi Henson,

>> Question is, should we fix optimize_window_clause as well?
> 
> Yes, we should. The comment in optimize_window_clauses explicitly
> states "Perform the same duplicate check that is done in
> transformWindowFuncCall", so the two checks must remain in sync.
> Even if omitting the RPR fields produces no visible bug today (because
> the early return guard in nocfbot-0006 already skips RPR windows), the
> code would be misleading and fragile: a future change could remove
> that guard and silently reintroduce the bug.
> 
> Adding the RPR fields explicitly also serves as defensive coding and
> makes the intent clear to anyone reading or modifying the code later.

Agreed.

> When incorporating nocfbot-0006 as patch 06/12, this fix was included:
> 
>     wc->rpSkipTo == existing_wc->rpSkipTo &&
>     wc->initial == existing_wc->initial &&
>     equal(wc->defineClause, existing_wc->defineClause) &&
>     equal(wc->rpPattern, existing_wc->rpPattern)
> 
> Patch 06/12 now covers both the frame optimization guard and the
> optimize_window_clauses deduplication fix, with EXPLAIN-based regression
> tests added to verify the behavior.

Good.

> Thank you for finding this. Your patch has been incorporated as patch
> 09/12 ("Fix window deduplication for RPR windows at parse time"),
> modified to add regression test cases covering the scenario. The fix
> adds the missing RPR check in transformWindowFuncCall:
> 
>     equal(refwin->rpCommonSyntax, windef->rpCommonSyntax)
> 
> This prevents incorrect merging of an RPR window with a non-RPR window
> that has identical frame options.

Good.

> The previous submission (nocfbot-0001..0008) has been extended to
> 12 patches. Changes since the last submission:
> 
>   06/12  Disable frame optimization for RPR windows  [updated]
>          optimize_window_clauses deduplication fix added, as discussed.
>
>   09/12  Fix window deduplication for RPR windows at parse time  [new]
>          Incorporates your fix with test cases added.

Thanks for adding test cases.

>   10/12  Walk DEFINE clause in window tree traversal  [new]
>          A newly discovered issue: nodeFuncs.c was not visiting the
>          DEFINE clause in expression_tree_walker, query_tree_walker,
>          and their mutator counterparts. The demonstrated case is SQL
>          function inlining: a SQL function with a parameter used in
>          DEFINE (e.g., DEFINE A AS v > $1) would fail to substitute
>          the actual argument, producing wrong results.

Excellnt findings!  BTW, I realized that we cannot use $1 of function
in PATTERN clause like: A{$1}.

ERROR:  42601: syntax error at or near "$1"
LINE 10:         PATTERN (A{$1})
                            ^
LOCATION:  scanner_yyerror, scan.l:1211

Should we document somewhere?

>   11/12  Use SQL standard term "empty match" in RPR comments  [new]
>          Align NFA comment terminology with the SQL standard: replace
>          implementation-specific terms "zero-length match" and
>          "zero-consumption" with the standard term "empty match".

Ok.

>   12/12  Extract RPR NFA engine into execRPR.c  [new]
>          Following the earlier discussion about file layout, this
>          implements option (b): move the NFA engine to a dedicated
>          src/backend/executor/execRPR.c, keeping it close to
>          nodeWindowAgg.c while making the boundary explicit. Functions
>          still take WindowAggState as an argument (no signature change).
>          The public interface is declared in execRPR.h using the ExecRPR*
>          naming convention, consistent with other executor headers.
>          The NFA algorithm guide shared earlier in this thread has also
>          been incorporated as structured comments at the top of the file,
>          as suggested.

Excellent work! Now the exported NFA engine API is cleary separated
from NFA internal static fnctions, which greatly enhances the
readability.

> The full updated series (12 patches on top of v44) is attached.

Thank you! Patch applied cleanly and passed all the regression tests.

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