Hi Jian,
My first reaction to the define_walker refactor was that something
about it did not sit right, but I could not put my finger on why, so I
kept putting off a clear answer -- sorry about that. I have taken the
time to think it through now.
To be clear, the patch is good, idiomatic PostgreSQL code:
contain_var_clause() and contain_rprnavexpr() are exactly how we
express these checks elsewhere, and it trims a fair amount of code.
> The allowed nav-expression combination is limited, therefore,
> has_column_ref is not necessary.
This is where I land differently, though. A DEFINE predicate is
context-sensitive: the same column reference is required inside a
navigation argument but rejected inside its offset. In
"DEFINE A AS PREV(price, price) > 0", the first price (the argument)
is required and the second (the offset) is rejected. The
phase-tracking walker models that context directly, in a single pass,
and it generalizes to whatever context-dependent nesting the clause
grows into (MEASURES, further navigation forms). Dropping it works
today only because the allowed combinations are limited, as you note
-- it leans on that limit rather than on the shape of the clause.
It also gives up the single-pass property the walker was written for
("no subtree is walked twice"): each navigation argument and offset is
now walked about twice, once by the main walk and again by the
contain_* check. The extra cost is small at parse time, but it is a
step back from a design that deliberately avoided it.
So my preference is to keep the context-aware walker as it stands.
This is more about which foundation we want for an inherently
context-sensitive clause than about anything being wrong in your
version -- if you see it the other way, I would be glad to hear it.
Best regards,
Henson
