Re: BUG #18589: pg_get_viewdef returns wrong query

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> This is broken starting from 1b4d280ea. The "new" and "old" entries
> in a view's rangetable were removed, which results in the varprefix
> flag being set to false in this case, as there is now only one rtable
> entry.

Yeah. Interesting that that old behavior accidentally hid this
ambiguity. I wonder whether there is a way to provoke mis-deparsing
even before 1b4d280ea.

The simplest fix would be to force varprefix on whenever deparsing
an ORDER BY list, as in the first patch attached. However, that
turns out to cause quite a lot of changes in regression test outputs
(which I didn't bother to include in the patch), which is unsurprising
given that it's reverting some of the 1b4d280ea output changes.
I don't like that too much, first because it seems like kind of a lot
of behavior churn for a stable branch, and second because it's just
plain ugly that the same variable is printed in different ways
depending on where it is in the query.

The second patch attached is what I think we should really do.
What it does is to prefix Vars only in the actually-troublesome
case where there is a conflicting SELECT-list entry, so that you
get the clutter only when doing something that's fairly ill-advised.
To do that, get_variable needs access to the SELECT list. We already
have that available in deparse_context, but the field name is
"windowTList" which seems rather misleading if it's to be used for
other purposes too. So I renamed and relocated that field to make
things less confusing. The other debatable point in the patch is
how to tell get_variable that we're considering an ORDER BY item.
I chose to re-use the special_exprkind field that's already there,
but I'm not totally convinced that that's a better solution than
adding another bool field. The problem is that we also use
get_rule_sortgroupclause for GROUP BY, so that it's temporarily hiding
the EXPR_KIND_GROUP_BY setting. That doesn't break anything as long
as we only change it while deparsing a Var, but it seems kind of
fragile. (On the whole, I don't think special_exprkind is all that
well thought out: aside from having no useful documentation, this
experience shows that it's not nearly as extensible as the author
probably believed. I wonder if we should replace it with
"bool in_group_by" or the like.)

With the second patch, there are no changes in existing regression
test outputs, so I added a new test.

Thoughts?

regards, tom lane