Hi Henson,
> 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.
Thank for eplaining the history.
BTW, in v47-0002 patch,
there are some non ASCII characters ("§").
+ /*
+ * Qualified column references in DEFINE are not supported. This covers
+ * both FROM-clause range variables (prohibited by §6.5) and pattern
+ * variable qualified names (e.g. UP.price), which are valid per §4.16
+ * but not yet implemented.
+ */
Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
