Hi Tatsuo,
> 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.
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.
> The two window definitions are regarded as identical and produces
> wrong result.
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.
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.
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.
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".
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.
The full updated series (12 patches on top of v44) is attached.
Regards,
Henson
