Hi Jian, Thanks for the misc-refactoring patch (v48-0001-v48-misc-refactoring) and the whole-row Var note. As before, now that v48 is posted I'll rebase on it and send these as incremental patches on top (v49); per-item disposition below, with the per-commit breakdown to come with the patch.
> RPRNavExpr->resulttype should also marked as pg_node_attr(query_jumble_ignore) Agreed -- every other derived result-type field in primnodes.h already carries it, and resulttype is derived from the (jumbled) arg type, so ignoring it loses nothing. I'll add it. > collectPatternVariables is not needed. > The parser already ensures every DEFINE variable appears in PATTERN ... > See nfa_evaluate_row the for loop break. Confirmed -- the parser already rejects a DEFINE variable not used in PATTERN, so every DEFINE var is in PATTERN; filtering is redundant and cost_windowagg can iterate defineClause directly, so I'll remove it. > buildDefineVariableList is trivial. No need to export it as an external function. Agreed -- I'll drop it and inline the list-building into create_windowagg_plan(). > Rename WindowAggState.defineClauseList to defineClauseExprs Agreed (the elements are ExprState, so it matches the ...Exprs convention) -- I'll do it, including the additional site the dormant-match fix touches. > Flatten a needlessly nested block in show_window_def(). > Replace a post-loop ListCell NULL check in remove_unused_subquery_outputs() > with a boolean flag. > Reduce the number of arguments in make_windowagg. > Minor refactoring of regress test comments. I'll do all of these. One exception: the parsenodes.h RPRPatternNode comment names the data structure, so rather than "clause" I'll change it to "parse tree node". > dvar, var both can be whole-row Vars, but this seems to work for whole-row vars. > We need some simple regress tests for cases where both are whole-row vars or one > of them is a whole-row var. Good catch -- I'll add regress coverage for the whole-row Var cases (both sides whole-row, and one side whole-row) in the needed_by_define check, in v49. I'll send the items above as a first incremental patch on top of v48, and take up the later reviews after that. Thanks, Henson
