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

Attachment
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>
日期: 星期六, 2023122 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

Attachment
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

Attachment