Re: BUG #18589: pg_get_viewdef returns wrong query

On Tue, Aug 27, 2024 at 1:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

I also think this is a better way.

> 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.

+1 to this part.

> 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.

It seems to me that this might result in different behavior from
previous when deparsing Vars in a GROUP BY list, due to overriding
special_exprkind from EXPR_KIND_GROUP_BY to EXPR_KIND_ORDER_BY.

For example, with this patch:

create table t (x1 int, x2 int);
create view v as select x1 as x2, x2 as x1 from t group by x1, x2;

select pg_get_viewdef('v', true);
     pg_get_viewdef
------------------------
  SELECT x1 AS x2,     +
     x2 AS x1          +
    FROM t             +
   GROUP BY t.x1, t.x2;
(1 row)

It seems that the prefixes in the GROUP BY Vars are unnecessary.

>  (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.)

From my brief 2-minute look at how special_exprkind is used, it seems
that special_exprkind is either EXPR_KIND_GROUP_BY or EXPR_KIND_NONE.
Since it doesn't seem to be extensible as expected, I agree that maybe
a bool field is more straightforward.  Then we can have another bool
field for ORDER BY, avoiding the need to override the in-group-by
setting.

Thanks
Richard