Hi Henson,
> 2XXX -- awaiting review from Tatsuo
I have finished reviewing 2015-2026 patches.
> 2015 Tidy up the row pattern unbounded-quantifier sentinel
> (= v50-0014)
> Revised per your 07-19 comment. The macro is no longer a local
> copy in gram.y: RPR_QUANTITY_INF is now defined once in
> nodes/parsenodes.h, next to RPRPatternNode whose max field
> stores it, and gram.y and optimizer/rpr.h use that one
> definition. On the placement -- you suggested primnodes.h; I
> put it in parsenodes.h because that is where RPRPatternNode
> lives, and parsenodes.h includes primnodes.h, so every stage
> that saw the macro before still does. Happy to move it to
> primnodes.h if you would rather keep it there.
Looks Ok. parsenodes.h is fine for me.
> 2016 Simplify row pattern compilation by passing the
> WindowClause (= v50-0015)
> buildRPRPattern() took rpPattern, rpSkipTo, frameOptions, and a
> pre-built defineVariableList as separate arguments. Pass the
> WindowClause directly instead: it carries all of these, and
> buildRPRPattern can walk wc->defineClause itself to collect the DEFINE
> variable names. This removes the intermediate List that
I am not sure if replacing those arguments with WindowClause is a good
idea. This would simplify buildRPRPattern() but at the same time it
will make it hard to call the function from R010 implementation
because MATCH_RECOGNIZE node surely will not have WindowClause node. I
know R010 is out of scope of our work but thinking about re-usability is
courtesy for the future implementer of R010.
> 2017 Reword the row pattern variable-limit error (= v50-0016)
OK.
> 2018 Reformat row pattern regression tests for
> readability (= v50-0017)
Ok.
> 2019 Add row pattern recognition coverage tests and tidy
> unreachable code (= v50-0018)
I think we should not include following patch in our patch set. That
has nothing to do with RPR.
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -4937,12 +4937,11 @@ WinCheckAndInitializeNullTreatment(WindowObject winobj,
{
const char *funcname = get_func_name(fcinfo->flinfo->fn_oid);
- if (!funcname)
- elog(ERROR, "could not get function name");
+ /* the executing function's name always resolves; stay safe
regardless */
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("function %s does not allow
RESPECT/IGNORE NULLS",
- funcname)));
+ funcname ? funcname : "?")));
}
else if (winobj->ignore_nulls == PARSER_IGNORE_NULLS)
winobj->ignore_nulls = IGNORE_NULLS;
> 2020 Free RPR NFA states with pfree() under
> USE_VALGRIND (= v50-0019)
Ok.
> 2021 Clarify row pattern recognition comments on "step" and
> no_equal (= v50-0020)
Ok.
> 2022 Extract the reduced-frame guard into ensure_reduced_frame()
Ok.
> 2023 Clean up row pattern recognition internals
> - Pass currentPos into nfa_match so the current row index is visible in a
> debugger; it is the only NFA helper that otherwise lacks it. The
> parameter has no runtime consumer yet.
There's no instance in PostgreSQL code which uses
pg_attribute_unused() with function arguments for debugging
purpose. Can you clarify why this is necessary (except just for
debugging convenience)?
> 2024 Improve row pattern recognition comments and documentation
Ok.
> 2025 Expand row pattern recognition regression tests
Ok.
> 2026 Fix context absorption discarding matches on non-absorbable
> branches
> [behavior change -- wrong results]
Ok.
> You mentioned you would continue through 2016..2021; those, and 2022..2026,
> are unchanged from the 07-13 posting.
Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
