> Hmm, surely that is a contrived case?
Sorry for forgetting to paste the reproduction. It should be as follows.
###
psql -d postgres -c 'create table t1(a int, b int, c int)'
psql -d postgres -c 'create view v1 as select a,b,c, -1::int from t1 group by 1,2,3,4'
pg_dumpall > /tmp/ddl.sql
psql -d postgres -c 'drop view v1'
psql -d postgres -c 'drop table t1'
psql -d postgres -f /tmp/ddl.sql
psql:/tmp/ddl.sql:111: ERROR: GROUP BY position -1 is not in select list
LINE 7: GROUP BY a, b, c, (- 1);
^
psql:/tmp/ddl.sql:114: ERROR: relation "public.v1" does not exist
###
> 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.
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.
> We could do worse than to implement that by actual const-folding,
> ie call expression_planner.
After exploring more codes, I also suppose expression_planner is a good choice.
Regards,
Haotian
发件人: Tom Lane <tgl@sss.pgh.pa.us>
日期: 星期六, 2023年12月2日 03:57
收件人: Haotian Chen <charliett2233@outlook.com>
抄送: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
主题: Re: Dumped SQL failed to execute with ERROR "GROUP BY position -1 is not in select list"
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