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

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

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

The thing to do here is modify pg_operator.dat and give both of these
operators an "oid_symbol". Perhaps Int4NegOperator is ok.

Thanks for suggestions, I replaced hacking oids with Int4NegOperator and NumericNegOperator.
And I also updated some comments and commit message.

Please feel free to review latest version patch v3-0001-Printing-const-folder-expression-in-ruleutils.c.patch

Best regards,
Haotian

From: David Rowley <dgrowleyml@gmail.com>
Date: Monday, December 4, 2023 at 06:08
To: Haotian Chen <charliett2233@outlook.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: Dumped SQL failed to execute with ERROR "GROUP BY position -1 is not in select list"
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