Hi Tatsuo, > I accidentaly noticed that v47-0002 changes findTargetlistEntrySQL99 > from static to extern. > [...] > I think this is not necessary anymore since findTargetlistEntrySQL99 > is not used outside parse_clause.c.
Good catch -- you're right, and I'll revert it to static in v48. I double-checked the current tree: the only callers left are inside parse_clause.c (the findTargetlistEntry wrapper, plus the GROUP BY and ORDER BY paths), so the extern prototype in parse_clause.h is now unused. The revert is just dropping that prototype and restoring the static qualifier with the in-file forward declaration it used to have. It's worth saying why it went extern in the first place, since the reason is no longer visible in the tree: The DEFINE clause needs its referenced columns present in the plan's targetlist to be evaluable at run time. The original implementation did that from the RPR side, in parse_rpr.c, by calling findTargetlistEntrySQL99() with resjunk = true to add the missing entry -- and since that function was static in parse_clause.c, reaching it across files is what required exposing it as extern. That approach added the whole DEFINE expression to the targetlist, and that turned out to be the source of a SIGSEGV: when an RPR window and a plain window coexist, the non-RPR WindowAgg inherited targetlist entries carrying RPRNavExpr nodes it has no way to evaluate. The fix was to add only the Vars a DEFINE references (with a guard in allpaths.c to keep those columns from being pruned), and that is what removed the cross-file call. So the extern has simply outlived its caller -- exactly as you spotted. The one loose end there is an optimization, not a correctness issue: the allpaths.c guard is deliberately coarse, so it also blocks removing a WindowAgg whose RPR WindowFuncs are all unused. Doing that precisely means restructuring remove_unused_subquery_outputs(), which runs for every subquery and not just RPR -- broad enough that I'm treating it as a longer-term item rather than part of this work. It doesn't bring the external call back -- the Var-only path stays -- so the revert to static is safe independently of it. I'll fold the static revert into v48. Thanks, Henson
