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
