Re: Dumped SQL failed to execute with ERROR "GROUP BY position -1 is not in select list"

Haotian Chen <charliett2233(at)outlook(dot)com> writes:
> postgres=# create view v1 as select * from t1 group by a,b,-1::int;
> CREATE VIEW

Hmm, surely that is a contrived case?

> After exploring codes, I suppose we should treat operator plus constant
> as -'nnn'::typename instead of const, my patch just did this by handling
> Opexpr especially, but I am not sure it's the best way or not,

Yeah, after some time looking at alternatives, I agree that hacking up
get_rule_sortgroupclause some more is the least risky way to make this
work. We could imagine changing the parser instead but people might
be depending on the current parsing behavior.

I don't like your patch too much though, particularly not the arbitrary
(and undocumented) change in get_const_expr; that seems way too likely
to have unexpected side-effects. Also, I think that relying on
generate_operator_name to produce exactly '-' (and not, say,
'pg_catalog.-') is unwise as well as unduly expensive.

There are, I think, precisely two operators we need to worry about here,
namely int4um and numeric_uminus. It'd be cheaper and more reliable to
identify those by OID. (If the underlying Const is neither int4 nor
numeric, it'll end up getting labeled with a typecast, so that we don't
need to worry about anything else.)

As for getting the right thing to be printed, I think what we might
want is to effectively const-fold the expression into a negative
Const, and then we can just apply get_const_expr with showtype=1.
(So we'd end with output like '-1'::integer or similar.)

We could do worse than to implement that by actual const-folding,
ie call expression_planner. Efficiency-wise that's not great, but
this is such a weird edge case that I don't think we need to sweat
about making it fast. The alternative of hand-rolled constant
folding code isn't very appealing.

regards, tom lane