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

Hi hackers,

I found that dumped view SQL failed to execute due to the explicit cast

of negative number, and I took a look at the defined SQL in view and then

found -1 in the group by clause. I suppose it’s the main reason the sql

cannot be executed and raised ERROR "GROUP BY position -1 is not in select list"

postgres=# create view v1 as select * from t1 group by a,b,-1::int;

CREATE VIEW

postgres=# \d+ v1;

                             View "public.v1"

 Column |  Type   | Collation | Nullable | Default | Storage | Description 

--------+---------+-----------+----------+---------+---------+-------------

 a      | integer |           |          |         | plain   | 

 b      | integer |           |          |         | plain   | 

View definition:

 SELECT a,

    b

   FROM t1

  GROUP BY a, b, (- 1);

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, BTW do you

guys have any suggestions and another approach?

--
Best Regards,
Haotian

Haotian Chen <charliett2233@outlook.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

On Mon, 4 Dec 2023 at 02:38, Haotian Chen <charliett2233@outlook.com> wrote:
> Yes, I updated my patch and just used oid numbers 558 and 1751 stand for
> int4um and numeric_uminus. Maybe we could define a macro for them,
> but seems unnecessary.

The thing to do here is modify pg_operator.dat and give both of these
operators an "oid_symbol". Perhaps Int4NegOperator is ok.  (I think
Int4UnaryMinusOperator might be on the verbose side.).  The code that
parses pg_operator.dat will then define that constant in
pg_operator_d.h.  You can then use that and the other ones you defined
for the numeric operator instead of hard coding the Oids in the patch.

David