Re: Row pattern recognition

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