pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedgroup by true now errors with non-integer constant in GROUP BY
20+ messages / 10 participants
[nested] [flat]
* group by true now errors with non-integer constant in GROUP BY
@ 2023-08-28 06:27 David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
0 siblings, 1 reply; 20+ messages in thread
From: David Micallef @ 2023-08-28 06:27 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hi Folks
I'm trying to upgrade our development environment from 13.11 to 15.4 as we
look forward to starting to use merge and a few other new features.
The only error that we have with our pgtap tests with is:
ERROR: non-integer constant in GROUP BY
It is only happening to a particular "group by true" that is dynamically
compiled when the function parameter asks to group by all data instead of
grouping other time series data.
I wrote the following script to reproduce also with an example of what we
do with the group by when the parameter is not all.:
#!/usr/bin/env bash
{
for version in "13.11" "14.9" "15.4" "15.0" "15.1" "15.0" "15.1" "15.2"
"15.3"; do #
docker rm -f postgres || true
echo "Testing postgres:$version"
docker run --rm --name postgres --net host -ePOSTGRES_USER=postgres -e
POSTGRES_PASSWORD=mysecretpassword -e PGPORT=54321 -d postgres:$version
timeout 90s /usr/bin/env bash -c "until docker exec postgres pg_isready ;
do sleep 5 ; done" # wait for db to be ready
psql -v ON_ERROR_STOP=on
postgresql://postgres:mysecretpassword@localhost:54321/postgres
<<EOF
CREATE TABLE IF NOT EXISTS test_data (id serial, proccess_time timestamp
with time zone, value NUMERIC);
INSERT INTO test_data(proccess_time, value)
SELECT test_time, random() * 100
FROM generate_series(now() - interval '30 days', now() + interval '30
days', INTERVAL '30 MIN') d(test_time);
SELECT (array_agg(proccess_time order by proccess_time asc))[1], avg(value)
FROM test_data GROUP BY date_part('week' , proccess_time); -- working
example
SELECT (array_agg(proccess_time order by proccess_time asc))[1], avg(value)
FROM test_data GROUP BY true;
EOF
done
}
The issues appears after 15.0
Thanks
--
Email: david.j.micallef@gmail.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
@ 2023-08-28 09:24 ` John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 20+ messages in thread
From: John Naylor @ 2023-08-28 09:24 UTC (permalink / raw)
To: David Micallef <david.j.micallef@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Mon, Aug 28, 2023 at 1:28 PM David Micallef <david.j.micallef@gmail.com>
wrote:
> The only error that we have with our pgtap tests with is:
> ERROR: non-integer constant in GROUP BY
>
> It is only happening to a particular "group by true" that is dynamically
compiled when the function parameter asks to group by all data instead of
grouping other time series data.
> The issues appears after 15.0
I can confirm the reproducer script fails with commit 941460fcf731a ("Add
Boolean node"). (So actually all 15.x were affected.)
The attached fixes it for me (applies onto v15). It looks simple enough,
but doesn't comfortably fit so might need some tweaking (needs tests, too).
--
John Naylor
EDB: http://www.enterprisedb.com
Attachments:
fix-group-by-bool.patch (text/x-patch, 583B) download
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index d821969a7d..45e0d01a8a 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -1992,6 +1992,10 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist,
int targetlist_pos = 0;
int target_pos;
+ if (IsA(&aconst->val, Boolean))
+ /* treat booleans as expressions */
+ return findTargetlistEntrySQL99(pstate, node, tlist, exprKind);
+
if (!IsA(&aconst->val, Integer))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
@ 2023-08-28 19:56 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-08-28 21:20 ` Re: group by true now errors with non-integer constant in GROUP BY Andrew Dunstan <andrew@dunslane.net>
2023-08-28 23:47 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-10-19 12:07 ` Re: group by true now errors with non-integer constant in GROUP BY Dennis Brouwer <dennis.brouwer@kizitos.com>
0 siblings, 3 replies; 20+ messages in thread
From: Tom Lane @ 2023-08-28 19:56 UTC (permalink / raw)
To: John Naylor <john.naylor@enterprisedb.com>; +Cc: David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
John Naylor <john.naylor@enterprisedb.com> writes:
> On Mon, Aug 28, 2023 at 1:28 PM David Micallef <david.j.micallef@gmail.com>
> wrote:
>> ERROR: non-integer constant in GROUP BY
> I can confirm the reproducer script fails with commit 941460fcf731a ("Add
> Boolean node"). (So actually all 15.x were affected.)
Yeah, same result from bisecting here. I had had the idea that this
was an intentional semantics change to reduce the probability of error,
but that commit didn't mention any such thing, so that's a tough claim
to make as far as the historical intent goes.
Having said that ... it seems to me that it was pure coincidence that
"group by true" worked before, and I'm not sure we should install a
grotty hack to make it work again. In particular, why should we allow
Boolean Consts but not other non-integer Consts? (And if we do, don't
we need to change that error message?)
The bigger picture here is: what is the use-case for grouping by a
constant at all? Assuming that it is an error seems like a good
foolproofing restriction. The reason we felt we could keep the
"group by N" SQL92-ism after SQL99 redefined GROUP BY arguments is
exactly that there's no obvious use-case for grouping by a constant.
As soon as you allow it, "group by N" becomes hopelessly ambiguous.
So my druthers would be to reject this as a non-bug. But if we accept
it as something to fix, we need to revisit exactly which conditions
are errors here. Perhaps rather than "reject all non-integer cases",
we should only reject the Float case, and let others fall through to
the SQL99 code. (I would not be happy allowing Float, because that'd
mean that "group by 4" and "group by 4.0" mean fundamentally different
things.)
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-08-28 21:20 ` Andrew Dunstan <andrew@dunslane.net>
2 siblings, 0 replies; 20+ messages in thread
From: Andrew Dunstan @ 2023-08-28 21:20 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; John Naylor <john.naylor@enterprisedb.com>; +Cc: David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On 2023-08-28 Mo 15:56, Tom Lane wrote:
>
> The bigger picture here is: what is the use-case for grouping by a
> constant at all? Assuming that it is an error seems like a good
> foolproofing restriction. The reason we felt we could keep the
> "group by N" SQL92-ism after SQL99 redefined GROUP BY arguments is
> exactly that there's no obvious use-case for grouping by a constant.
> As soon as you allow it, "group by N" becomes hopelessly ambiguous.
>
> So my druthers would be to reject this as a non-bug. But if we accept
> it as something to fix, we need to revisit exactly which conditions
> are errors here. Perhaps rather than "reject all non-integer cases",
> we should only reject the Float case, and let others fall through to
> the SQL99 code. (I would not be happy allowing Float, because that'd
> mean that "group by 4" and "group by 4.0" mean fundamentally different
> things.)
>
>
I vote for treating it as a non-bug.
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-08-28 23:47 ` David Rowley <dgrowleyml@gmail.com>
2023-08-29 01:02 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 20+ messages in thread
From: David Rowley @ 2023-08-28 23:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On Tue, 29 Aug 2023 at 07:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Having said that ... it seems to me that it was pure coincidence that
> "group by true" worked before, and I'm not sure we should install a
> grotty hack to make it work again. In particular, why should we allow
> Boolean Consts but not other non-integer Consts? (And if we do, don't
> we need to change that error message?)
Is it really a grotty hack? Isn't it just as simple as the attached?
> The bigger picture here is: what is the use-case for grouping by a
> constant at all? Assuming that it is an error seems like a good
> foolproofing restriction. The reason we felt we could keep the
> "group by N" SQL92-ism after SQL99 redefined GROUP BY arguments is
> exactly that there's no obvious use-case for grouping by a constant.
> As soon as you allow it, "group by N" becomes hopelessly ambiguous.
The new behaviour feels a bit inconsistent to me as it stands today.
I can't write GROUP BY true, but I can write GROUP BY 1=1, which gets
it beyond the parser and allows constant folding to turn it into GROUP
BY true, which I couldn't specify because the parser would complain.
> So my druthers would be to reject this as a non-bug. But if we accept
> it as something to fix, we need to revisit exactly which conditions
> are errors here. Perhaps rather than "reject all non-integer cases",
> we should only reject the Float case, and let others fall through to
> the SQL99 code. (I would not be happy allowing Float, because that'd
> mean that "group by 4" and "group by 4.0" mean fundamentally different
> things.)
I had a look on dbfiddle and I see that MySQL 8.0 and SQLlite all
allow GROUP BY true. I think if we used to, and those other databases
do, then we might want to reconsider supporting it again, especially
so now that someone has complained. I'm assuming it's just as simple
as the attached patch, but I'm happy to listen if I've underestimated
the complexity.
David
Attachments:
grotty_hack.patch (application/octet-stream, 876B) download
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 334b9b42bd..cdf9c5c78a 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -2106,20 +2106,12 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist,
}
}
}
- if (IsA(node, A_Const))
+ if (IsA(node, A_Const) && IsA(&castNode(A_Const, node)->val, Integer))
{
A_Const *aconst = castNode(A_Const, node);
int targetlist_pos = 0;
int target_pos;
- if (!IsA(&aconst->val, Integer))
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- /* translator: %s is name of a SQL construct, eg ORDER BY */
- errmsg("non-integer constant in %s",
- ParseExprKindName(exprKind)),
- parser_errposition(pstate, aconst->location)));
-
target_pos = intVal(&aconst->val);
foreach(tl, *tlist)
{
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-28 23:47 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
@ 2023-08-29 01:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-08-29 01:54 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Tom Lane @ 2023-08-29 01:02 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
David Rowley <dgrowleyml@gmail.com> writes:
> On Tue, 29 Aug 2023 at 07:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The bigger picture here is: what is the use-case for grouping by a
>> constant at all? Assuming that it is an error seems like a good
>> foolproofing restriction. The reason we felt we could keep the
>> "group by N" SQL92-ism after SQL99 redefined GROUP BY arguments is
>> exactly that there's no obvious use-case for grouping by a constant.
>> As soon as you allow it, "group by N" becomes hopelessly ambiguous.
> The new behaviour feels a bit inconsistent to me as it stands today.
> I can't write GROUP BY true, but I can write GROUP BY 1=1, which gets
> it beyond the parser and allows constant folding to turn it into GROUP
> BY true, which I couldn't specify because the parser would complain.
Sure, you can write any constant expression, for instance NULL::bool
would work. The question is where do we draw the line between SQL92
and SQL99 behaviors. I think "an undecorated constant is SQL92, and
therefore it must be an integer matching an output column number" is
a nice simple rule. The fact that TRUE and FALSE were not previously
treated as undecorated constants is an unintentional wart of the old
implementation, not something we ought to preserve.
> I had a look on dbfiddle and I see that MySQL 8.0 and SQLlite all
> allow GROUP BY true.
What do they do with GROUP BY 1, or GROUP BY 10000, or GROUP BY 1.0 ?
> I think if we used to, and those other databases
> do, then we might want to reconsider supporting it again, especially
> so now that someone has complained. I'm assuming it's just as simple
> as the attached patch, but I'm happy to listen if I've underestimated
> the complexity.
Sure, changing the behavior is trivial. Whether we *should* change
the behavior, and if so to what, is less so. I'm not really on
board with "we need to maintain bug-compatibility with an old
implementation artifact".
BTW, I poked around and couldn't find anything explaining this
fine point in the SGML docs, although the comments in
findTargetlistEntrySQL92 are clear about it. If we do anything
at all here, I think that ought to include documenting the behavior
more clearly (and I'm curious to see how you'd propose to explain
the behavior you want to users).
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-28 23:47 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 01:02 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-08-29 01:54 ` David Rowley <dgrowleyml@gmail.com>
2023-08-29 06:39 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
0 siblings, 1 reply; 20+ messages in thread
From: David Rowley @ 2023-08-29 01:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On Tue, 29 Aug 2023 at 13:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > I had a look on dbfiddle and I see that MySQL 8.0 and SQLlite all
> > allow GROUP BY true.
>
> What do they do with GROUP BY 1, or GROUP BY 10000, or GROUP BY 1.0 ?
All treat only integer constants as column references. Out-of-range
integer values are reported as errors. Other const types appear to be
treated as expressions rather than column references.
> BTW, I poked around and couldn't find anything explaining this
> fine point in the SGML docs, although the comments in
> findTargetlistEntrySQL92 are clear about it. If we do anything
> at all here, I think that ought to include documenting the behavior
> more clearly (and I'm curious to see how you'd propose to explain
> the behavior you want to users).
The rule and how to explain it seems fairly simple to me. Integer
constants are treated as column references to their corresponding
1-based position in the SELECT clause. Anything else is treated as an
expression.
David
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-28 23:47 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 01:02 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-29 01:54 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
@ 2023-08-29 06:39 ` John Naylor <john.naylor@enterprisedb.com>
2023-09-17 23:36 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: John Naylor @ 2023-08-29 06:39 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On Tue, Aug 29, 2023 at 8:55 AM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Tue, 29 Aug 2023 at 13:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > BTW, I poked around and couldn't find anything explaining this
> > fine point in the SGML docs, although the comments in
> > findTargetlistEntrySQL92 are clear about it. If we do anything
> > at all here, I think that ought to include documenting the behavior
> > more clearly (and I'm curious to see how you'd propose to explain
> > the behavior you want to users).
>
> The rule and how to explain it seems fairly simple to me. Integer
> constants are treated as column references to their corresponding
> 1-based position in the SELECT clause. Anything else is treated as an
> expression.
Seems reasonable to me.
--
John Naylor
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-28 23:47 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 01:02 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-29 01:54 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 06:39 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
@ 2023-09-17 23:36 ` David Rowley <dgrowleyml@gmail.com>
2023-09-18 00:00 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 20+ messages in thread
From: David Rowley @ 2023-09-17 23:36 UTC (permalink / raw)
To: John Naylor <john.naylor@enterprisedb.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On Tue, 29 Aug 2023 at 18:39, John Naylor <john.naylor@enterprisedb.com> wrote:
>
> On Tue, Aug 29, 2023 at 8:55 AM David Rowley <dgrowleyml@gmail.com> wrote:
> >
> > On Tue, 29 Aug 2023 at 13:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > BTW, I poked around and couldn't find anything explaining this
> > > fine point in the SGML docs, although the comments in
> > > findTargetlistEntrySQL92 are clear about it. If we do anything
> > > at all here, I think that ought to include documenting the behavior
> > > more clearly (and I'm curious to see how you'd propose to explain
> > > the behavior you want to users).
> >
> > The rule and how to explain it seems fairly simple to me. Integer
> > constants are treated as column references to their corresponding
> > 1-based position in the SELECT clause. Anything else is treated as an
> > expression.
>
> Seems reasonable to me.
Here's how I think we should proceed:
1. Re-allow Boolean constants in GROUP BY in v15 and v16 by
backpatching John's fix to special case Booleans.
2. In master only, remove the special case added in #1 and allow any
non-integer constants to be treated as expressions.
I think #2 is a good move for the following reasons:
a) Seems consistent with other RDBMSs (See [1]).
b) Gets rid of the special case added in #1 to allow booleans
c) Consistent with things like "JOIN ... ON true".
d) May allow simplified coding in ORMs. Without a GROUP BY clause,
you're at the mercy of there being any aggregate functions in the
target list to define the grouping behaviour.
I've included 2 patches, 0001 for #1 (John's patch with the comment
adjusted to explain the special case) and 0002 for #2.
Does anyone think we should do this differently?
David
[1] https://postgr.es/m/CAApHDvomA1bZy=0AYUcTjDWaCeedcPeDBo6PV0VhpVeo2jG1uQ@mail.gmail.com
Attachments: download all (2 files, .tar.gz)
v1-0001-Re-allow-Boolean-constants-in-GROUP-BY.patch (application/octet-stream, 1.7K) download
From 6ed3cf56e65027696c24c4a931b9d0a5062b9006 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Mon, 18 Sep 2023 10:19:21 +1200
Subject: [PATCH v1 1/2] Re-allow Boolean constants in GROUP BY
941460fcf added a boolean node type which effectively stopped "GROUP BY
true" from working as we only allowed integer constants in the GROUP BY
clause.
Here we add a special case to allow Boolean constants to be specified
again. This may be useful for users who wish to guarantee grouping
takes place even when the targetlist has no aggregate functions. It
also seems good to be consistent as we allow "JOIN ... ON true".
Reported-by: David Micallef
Author: John Naylor
Discussion: https://postgr.es/m/CAJBCwCS7OuEUmGkrnu5Q8SmBP10njnEiX7vJiaxJURZXcCORyw@mail.gmail.com
Backpatch-through: 15, where 941460fcf was added
---
src/backend/parser/parse_clause.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 334b9b42bd..9effe6ae7c 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -2112,6 +2112,14 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist,
int targetlist_pos = 0;
int target_pos;
+ /*
+ * Pre-v15 versions supported boolean constants in the GROUP BY clause.
+ * Here we make a special case to allow these after boolean types were given
+ * their own parse node type in v15.
+ */
+ if (IsA(&aconst->val, Boolean))
+ return findTargetlistEntrySQL99(pstate, node, tlist, exprKind);
+
if (!IsA(&aconst->val, Integer))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
--
2.40.1.windows.1
v1-0002-Allow-any-constant-types-in-GROUP-BY-clause.patch (application/octet-stream, 4.8K) download
From cc47bc21c49a0941f7664b6707a5f9a52b7aa1ce Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Mon, 11 Sep 2023 17:10:41 +1200
Subject: [PATCH v1 2/2] Allow any constant types in GROUP BY clause
In v14 and earlier, only integer Consts were allowed in the GROUP BY
clause. Boolean constants made it through as these had no native node
type and were expressed as TypeCasts. As of 941460fcf, Booleans have
their own node type which resulted in "GROUP BY true" no longer working in
v15 and v16.
Here adjust the code to remove the special Boolean case that was added
to fix the v15/v16 regression and just allow any constant types in the
GROUP BY. Integers, of course, remain a special case to reference the
ordinal position of the column in the target list to group by.
Also, clarify in the documentation that references made in the GROUP BY
to output columns must be done so using integer literals. This should
lessen the chances of any surprises that "GROUP BY 1.0" doesn't group by
the 1st column in the target list.
Discussion: https://postgr.es/m/CAApHDvrSUvb1ODvNcYGHz4O6WEsPChhwmSkJcs_3y5pniN2p+A@mail.gmail.com
---
doc/src/sgml/ref/select.sgml | 8 ++---
src/backend/parser/parse_clause.c | 52 +++++++++++++------------------
2 files changed, 26 insertions(+), 34 deletions(-)
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 0ee0cc7e64..323561cb0f 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -804,10 +804,10 @@ GROUP BY [ ALL | DISTINCT ] <replaceable class="parameter">grouping_element</rep
class="parameter">expression</replaceable> used inside a
<replaceable class="parameter">grouping_element</replaceable>
can be an input column name, or the name or ordinal number of an
- output column (<command>SELECT</command> list item), or an arbitrary
- expression formed from input-column values. In case of ambiguity,
- a <literal>GROUP BY</literal> name will be interpreted as an
- input-column name rather than an output column name.
+ output column (<command>SELECT</command> list item) expressed as an
+ integer literal, or an arbitrary expression formed from input-column
+ values. In case of ambiguity, a <literal>GROUP BY</literal> name will be
+ interpreted as an input-column name rather than an output column name.
</para>
<para>
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 9effe6ae7c..a988e0202c 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -2109,46 +2109,38 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist,
if (IsA(node, A_Const))
{
A_Const *aconst = castNode(A_Const, node);
- int targetlist_pos = 0;
- int target_pos;
/*
- * Pre-v15 versions supported boolean constants in the GROUP BY clause.
- * Here we make a special case to allow these after boolean types were given
- * their own parse node type in v15.
+ * Handle integer constants as the 1-based position in the query's
+ * targetlist. Other constant types are treated as expressions.
*/
- if (IsA(&aconst->val, Boolean))
- return findTargetlistEntrySQL99(pstate, node, tlist, exprKind);
-
- if (!IsA(&aconst->val, Integer))
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- /* translator: %s is name of a SQL construct, eg ORDER BY */
- errmsg("non-integer constant in %s",
- ParseExprKindName(exprKind)),
- parser_errposition(pstate, aconst->location)));
-
- target_pos = intVal(&aconst->val);
- foreach(tl, *tlist)
+ if (IsA(&aconst->val, Integer))
{
- TargetEntry *tle = (TargetEntry *) lfirst(tl);
+ int targetlist_pos = 0;
+ int target_pos;
- if (!tle->resjunk)
+ target_pos = intVal(&aconst->val);
+ foreach(tl, *tlist)
{
- if (++targetlist_pos == target_pos)
+ TargetEntry *tle = (TargetEntry *) lfirst(tl);
+
+ if (!tle->resjunk)
{
- /* return the unique match, after suitable validation */
- checkTargetlistEntrySQL92(pstate, tle, exprKind);
- return tle;
+ if (++targetlist_pos == target_pos)
+ {
+ /* return the unique match, after suitable validation */
+ checkTargetlistEntrySQL92(pstate, tle, exprKind);
+ return tle;
+ }
}
}
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+ /* translator: %s is name of a SQL construct, eg ORDER BY */
+ errmsg("%s position %d is not in select list",
+ ParseExprKindName(exprKind), target_pos),
+ parser_errposition(pstate, aconst->location)));
}
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
- /* translator: %s is name of a SQL construct, eg ORDER BY */
- errmsg("%s position %d is not in select list",
- ParseExprKindName(exprKind), target_pos),
- parser_errposition(pstate, aconst->location)));
}
/*
--
2.40.1.windows.1
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-28 23:47 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 01:02 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-29 01:54 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 06:39 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-09-17 23:36 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
@ 2023-09-18 00:00 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-09-18 00:22 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-09-21 04:58 ` Re: group by true now errors with non-integer constant in GROUP BY Peter Geoghegan <pg@bowt.ie>
0 siblings, 2 replies; 20+ messages in thread
From: Tom Lane @ 2023-09-18 00:00 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
David Rowley <dgrowleyml@gmail.com> writes:
> 2. In master only, remove the special case added in #1 and allow any
> non-integer constants to be treated as expressions.
> I think #2 is a good move for the following reasons:
FTR, I still think this is a bad idea. It will add more confusion
than it removes, and I don't buy that it will confer any advantages,
because nobody asked for it previously.
However, assuming that I'm going to be out-voted: your docs changes
still need work. That phrasing makes it sound like an output column
name could be "expressed as an integer literal". Maybe we should
restructure the whole sentence, perhaps along the lines of
... class="parameter">expression</replaceable> used inside a
<replaceable class="parameter">grouping_element</replaceable>
can be an input column name or an arbitrary expression formed
from input column values. For backwards compatibility with
SQL-92, the <replaceable>expression</replaceable> can also be
an output column name or an integer literal representing the
ordinal number of an output column. In case of ambiguity, a
<literal>GROUP BY</literal> item that is just a name will be
interpreted as an input column name rather than an output column
name.
Neither formulation addresses the case of non-integer literals
head on. I'm not quite sure if it's worth adding another sentence
to do so.
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-28 23:47 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 01:02 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-29 01:54 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 06:39 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-09-17 23:36 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-09-18 00:00 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-09-18 00:22 ` David Rowley <dgrowleyml@gmail.com>
1 sibling, 0 replies; 20+ messages in thread
From: David Rowley @ 2023-09-18 00:22 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On Mon, 18 Sept 2023 at 12:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > 2. In master only, remove the special case added in #1 and allow any
> > non-integer constants to be treated as expressions.
>
> > I think #2 is a good move for the following reasons:
>
> FTR, I still think this is a bad idea. It will add more confusion
> than it removes, and I don't buy that it will confer any advantages,
> because nobody asked for it previously.
I'm not dead set on it. I just don't think we can do exactly nothing
about this. At the very least we need to mention in REL_15_STABLE's
release-15.sgml in the incompatibilities section.
Another reason for #2 which I forgot to add was that it gets rid of
the need to have the "non-integer constant in ..." error message and
saves about 8 lines of code and a string constant table entry.
I'll happily wait to see if anyone else has any thoughts on this. The
votes seem roughly 2 vs 2 at the moment.
David
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-28 23:47 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 01:02 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-29 01:54 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 06:39 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-09-17 23:36 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-09-18 00:00 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-09-21 04:58 ` Peter Geoghegan <pg@bowt.ie>
2023-09-21 07:06 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Peter Geoghegan @ 2023-09-21 04:58 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On Sun, Sep 17, 2023 at 5:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Rowley <dgrowleyml@gmail.com> writes:
> > 2. In master only, remove the special case added in #1 and allow any
> > non-integer constants to be treated as expressions.
>
> > I think #2 is a good move for the following reasons:
>
> FTR, I still think this is a bad idea. It will add more confusion
> than it removes, and I don't buy that it will confer any advantages,
> because nobody asked for it previously.
I agree. Maintaining bug compatibility doesn't seem worth it in this instance.
David Micallef mentioned that only one query was affected. You have to
draw the line somewhere.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-28 23:47 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 01:02 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-08-29 01:54 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-08-29 06:39 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-09-17 23:36 ` Re: group by true now errors with non-integer constant in GROUP BY David Rowley <dgrowleyml@gmail.com>
2023-09-18 00:00 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-09-21 04:58 ` Re: group by true now errors with non-integer constant in GROUP BY Peter Geoghegan <pg@bowt.ie>
@ 2023-09-21 07:06 ` David Rowley <dgrowleyml@gmail.com>
0 siblings, 0 replies; 20+ messages in thread
From: David Rowley @ 2023-09-21 07:06 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On Thu, 21 Sept 2023 at 16:58, Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Sun, Sep 17, 2023 at 5:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > FTR, I still think this is a bad idea. It will add more confusion
> > than it removes, and I don't buy that it will confer any advantages,
> > because nobody asked for it previously.
>
> I agree. Maintaining bug compatibility doesn't seem worth it in this instance.
Thanks for chipping in.
Just for the record, I'm more keen on removing the special case that
disallows this. For me it's more about POLA. I expect that anywhere
I can write an expression in SQL, that I can put a Const in its place
and not receive an error to say constants are disallowed.
I mentioned this to Andres in a meeting yesterday and it seems to be
against allowing Consts, so I seem to be losing this one.
David
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-10-19 12:07 ` Dennis Brouwer <dennis.brouwer@kizitos.com>
2023-10-19 13:18 ` Re: group by true now errors with non-integer constant in GROUP BY Laurenz Albe <laurenz.albe@cybertec.at>
2 siblings, 1 reply; 20+ messages in thread
From: Dennis Brouwer @ 2023-10-19 12:07 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
Dear all,
I would like to add something to the question: "why grouping on a constant
would be necessary", even if it is plain wrong.
Background info: I stumbled upon this thread while upgrading our database
from postgresql-14 to postgresql-16 making use of a Java application using
of JPA and Hibernate. Hibernate is a widely used framework and this library
will compose queries (under certain conditions (still unknown to me))
with *GROUP
BY coulmn1, column2, true* *<-- *
Hibernate has been doing this quircky thing for many many years and even in
the latest release does so. So, potentionally many Java Enterprise
applications will be tied to postgresql-14 if there is no compatibility
switch possible.
In our case a tiny compatability switch would be a livesaver. Of courcse, I
will try to convince the Hibernate community to have this unnecessary
constant removed but that still leaves all legacy code to not work with
postgresql-15+ databases which would be pitiful!
Kind regards,
Dennis Brouwer
On Thu, 19 Oct 2023 at 13:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> John Naylor <john.naylor@enterprisedb.com> writes:
> > On Mon, Aug 28, 2023 at 1:28 PM David Micallef <
> david.j.micallef@gmail.com>
> > wrote:
> >> ERROR: non-integer constant in GROUP BY
>
> > I can confirm the reproducer script fails with commit 941460fcf731a ("Add
> > Boolean node"). (So actually all 15.x were affected.)
>
> Yeah, same result from bisecting here. I had had the idea that this
> was an intentional semantics change to reduce the probability of error,
> but that commit didn't mention any such thing, so that's a tough claim
> to make as far as the historical intent goes.
>
> Having said that ... it seems to me that it was pure coincidence that
> "group by true" worked before, and I'm not sure we should install a
> grotty hack to make it work again. In particular, why should we allow
> Boolean Consts but not other non-integer Consts? (And if we do, don't
> we need to change that error message?)
>
> The bigger picture here is: what is the use-case for grouping by a
> constant at all? Assuming that it is an error seems like a good
> foolproofing restriction. The reason we felt we could keep the
> "group by N" SQL92-ism after SQL99 redefined GROUP BY arguments is
> exactly that there's no obvious use-case for grouping by a constant.
> As soon as you allow it, "group by N" becomes hopelessly ambiguous.
>
> So my druthers would be to reject this as a non-bug. But if we accept
> it as something to fix, we need to revisit exactly which conditions
> are errors here. Perhaps rather than "reject all non-integer cases",
> we should only reject the Float case, and let others fall through to
> the SQL99 code. (I would not be happy allowing Float, because that'd
> mean that "group by 4" and "group by 4.0" mean fundamentally different
> things.)
>
> regards, tom lane
>
>
>
>
>
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-10-19 12:07 ` Re: group by true now errors with non-integer constant in GROUP BY Dennis Brouwer <dennis.brouwer@kizitos.com>
@ 2023-10-19 13:18 ` Laurenz Albe <laurenz.albe@cybertec.at>
2023-10-19 13:33 ` Re: group by true now errors with non-integer constant in GROUP BY David G. Johnston <david.g.johnston@gmail.com>
2023-10-20 05:44 ` Re: group by true now errors with non-integer constant in GROUP BY Vik Fearing <vik@postgresfriends.org>
0 siblings, 2 replies; 20+ messages in thread
From: Laurenz Albe @ 2023-10-19 13:18 UTC (permalink / raw)
To: Dennis Brouwer <dennis.brouwer@kizitos.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On Thu, 2023-10-19 at 14:07 +0200, Dennis Brouwer wrote:
> Hibernate is a widely used framework and this library will compose queries
> (under certain conditions (still unknown to me))
> with GROUP BY coulmn1, column2, true <--
>
> Hibernate has been doing this quircky thing for many many years and even
> in the latest release does so. So, potentionally many Java Enterprise
> applications will be tied to postgresql-14 if there is no compatibility
> switch possible.
>
> In our case a tiny compatability switch would be a livesaver. Of courcse,
> I will try to convince the Hibernate community to have this unnecessary
> constant removed but that still leaves all legacy code to not work with
> postgresql-15+ databases which would be pitiful!
I understand your pain.
But according to my reading of the SQL standard, it only allows for
regular column references in GROUP BY. Unless I got something wrong,
that would mean the the ball is in Hibernate's court. It ought to
produce correct SQL.
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-10-19 12:07 ` Re: group by true now errors with non-integer constant in GROUP BY Dennis Brouwer <dennis.brouwer@kizitos.com>
2023-10-19 13:18 ` Re: group by true now errors with non-integer constant in GROUP BY Laurenz Albe <laurenz.albe@cybertec.at>
@ 2023-10-19 13:33 ` David G. Johnston <david.g.johnston@gmail.com>
1 sibling, 0 replies; 20+ messages in thread
From: David G. Johnston @ 2023-10-19 13:33 UTC (permalink / raw)
To: Laurenz Albe <laurenz.albe@cybertec.at>; +Cc: Dennis Brouwer <dennis.brouwer@kizitos.com>; Tom Lane <tgl@sss.pgh.pa.us>; John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On Thursday, October 19, 2023, Laurenz Albe <laurenz.albe@cybertec.at>
wrote:
> On Thu, 2023-10-19 at 14:07 +0200, Dennis Brouwer wrote:
> > Hibernate is a widely used framework and this library will compose
> queries
> > (under certain conditions (still unknown to me))
> > with GROUP BY coulmn1, column2, true <--
> >
> > Hibernate has been doing this quircky thing for many many years and even
> > in the latest release does so. So, potentionally many Java Enterprise
> > applications will be tied to postgresql-14 if there is no compatibility
> > switch possible.
> >
> > In our case a tiny compatability switch would be a livesaver. Of courcse,
> > I will try to convince the Hibernate community to have this unnecessary
> > constant removed but that still leaves all legacy code to not work with
> > postgresql-15+ databases which would be pitiful!
>
> I understand your pain.
>
> But according to my reading of the SQL standard, it only allows for
> regular column references in GROUP BY. Unless I got something wrong,
> that would mean the the ball is in Hibernate's court. It ought to
> produce correct SQL.
>
My takeaway is that if Hibernate is able to produce this and get away with
it in the various jdbc drivers it needs to work with then our existing
choice to extend beyond the standard seems justified as others must be
doing it as well. Introducing a regression now on the basis of standard
conformance is just going to harm our reputation with minimal benefit, all
seemingly accrued by developers, not users.
David J.
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-10-19 12:07 ` Re: group by true now errors with non-integer constant in GROUP BY Dennis Brouwer <dennis.brouwer@kizitos.com>
2023-10-19 13:18 ` Re: group by true now errors with non-integer constant in GROUP BY Laurenz Albe <laurenz.albe@cybertec.at>
@ 2023-10-20 05:44 ` Vik Fearing <vik@postgresfriends.org>
2023-10-20 06:20 ` Re: group by true now errors with non-integer constant in GROUP BY Vik Fearing <vik@postgresfriends.org>
2023-10-20 06:32 ` Re: group by true now errors with non-integer constant in GROUP BY Laurenz Albe <laurenz.albe@cybertec.at>
1 sibling, 2 replies; 20+ messages in thread
From: Vik Fearing @ 2023-10-20 05:44 UTC (permalink / raw)
To: Laurenz Albe <laurenz.albe@cybertec.at>; Dennis Brouwer <dennis.brouwer@kizitos.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On 10/19/23 15:18, Laurenz Albe wrote:
> On Thu, 2023-10-19 at 14:07 +0200, Dennis Brouwer wrote:
>> Hibernate is a widely used framework and this library will compose queries
>> (under certain conditions (still unknown to me))
>> with GROUP BY coulmn1, column2, true <--
>>
>> Hibernate has been doing this quircky thing for many many years and even
>> in the latest release does so. So, potentionally many Java Enterprise
>> applications will be tied to postgresql-14 if there is no compatibility
>> switch possible.
>>
>> In our case a tiny compatability switch would be a livesaver. Of courcse,
>> I will try to convince the Hibernate community to have this unnecessary
>> constant removed but that still leaves all legacy code to not work with
>> postgresql-15+ databases which would be pitiful!
>
> I understand your pain.
>
> But according to my reading of the SQL standard, it only allows for
> regular column references in GROUP BY. Unless I got something wrong,
> that would mean the the ball is in Hibernate's court. It ought to
> produce correct SQL.
The answer is not as easy as that. It is true that the standard
requires a <column reference> for each element of the GROUP BY clause,
but it is also true that PostgreSQL allows arbitrary expressions.
Why is this non-standard query allowed:
vik=# select true group by true or true;
?column?
----------
t
(1 row)
but not this one?
vik=# select true group by true;
ERROR: non-integer constant in GROUP BY
LINE 1: select true group by true;
^
I may have oversimplified this example, but as long as the value is
present in the SELECT, logic would dictate that we can group by it.
The correct thing to do would be to *at least* get rid of the horrible
monstrosity that is "GROUP BY 1", but that is probably never going to
happen. This syntax was never part of the standard, and the "ORDER BY
1" syntax it was calqued upon was ripped out in SQL-99.
--
Vik Fearing
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-10-19 12:07 ` Re: group by true now errors with non-integer constant in GROUP BY Dennis Brouwer <dennis.brouwer@kizitos.com>
2023-10-19 13:18 ` Re: group by true now errors with non-integer constant in GROUP BY Laurenz Albe <laurenz.albe@cybertec.at>
2023-10-20 05:44 ` Re: group by true now errors with non-integer constant in GROUP BY Vik Fearing <vik@postgresfriends.org>
@ 2023-10-20 06:20 ` Vik Fearing <vik@postgresfriends.org>
1 sibling, 0 replies; 20+ messages in thread
From: Vik Fearing @ 2023-10-20 06:20 UTC (permalink / raw)
To: Laurenz Albe <laurenz.albe@cybertec.at>; Dennis Brouwer <dennis.brouwer@kizitos.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On 10/20/23 07:44, Vik Fearing wrote:
> I may have oversimplified this example, but as long as the value is
> present in the SELECT, logic would dictate that we can group by it.
Oops, I have this backwards: the projection comes after the grouping.
--
Vik Fearing
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-10-19 12:07 ` Re: group by true now errors with non-integer constant in GROUP BY Dennis Brouwer <dennis.brouwer@kizitos.com>
2023-10-19 13:18 ` Re: group by true now errors with non-integer constant in GROUP BY Laurenz Albe <laurenz.albe@cybertec.at>
2023-10-20 05:44 ` Re: group by true now errors with non-integer constant in GROUP BY Vik Fearing <vik@postgresfriends.org>
@ 2023-10-20 06:32 ` Laurenz Albe <laurenz.albe@cybertec.at>
2023-10-20 19:03 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 20+ messages in thread
From: Laurenz Albe @ 2023-10-20 06:32 UTC (permalink / raw)
To: Vik Fearing <vik@postgresfriends.org>; Dennis Brouwer <dennis.brouwer@kizitos.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
On Fri, 2023-10-20 at 07:44 +0200, Vik Fearing wrote:
> > But according to my reading of the SQL standard, it only allows for
> > regular column references in GROUP BY. Unless I got something wrong,
> > that would mean the the ball is in Hibernate's court. It ought to
> > produce correct SQL.
>
> The answer is not as easy as that. It is true that the standard
> requires a <column reference> for each element of the GROUP BY clause,
> but it is also true that PostgreSQL allows arbitrary expressions.
>
> Why is this non-standard query allowed:
>
> vik=# select true group by true or true;
> ?column?
> ----------
> t
> (1 row)
>
>
> but not this one?
>
> vik=# select true group by true;
> ERROR: non-integer constant in GROUP BY
> LINE 1: select true group by true;
> ^
>
> I may have oversimplified this example, but as long as the value is
> present in the SELECT, logic would dictate that we can group by it.
I agree that it is desirable to fix this regression.
Still, we shouldn't exonerate Hibernate from fixing the junk SQL
it produces.
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: group by true now errors with non-integer constant in GROUP BY
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` Re: group by true now errors with non-integer constant in GROUP BY John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Re: group by true now errors with non-integer constant in GROUP BY Tom Lane <tgl@sss.pgh.pa.us>
2023-10-19 12:07 ` Re: group by true now errors with non-integer constant in GROUP BY Dennis Brouwer <dennis.brouwer@kizitos.com>
2023-10-19 13:18 ` Re: group by true now errors with non-integer constant in GROUP BY Laurenz Albe <laurenz.albe@cybertec.at>
2023-10-20 05:44 ` Re: group by true now errors with non-integer constant in GROUP BY Vik Fearing <vik@postgresfriends.org>
2023-10-20 06:32 ` Re: group by true now errors with non-integer constant in GROUP BY Laurenz Albe <laurenz.albe@cybertec.at>
@ 2023-10-20 19:03 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 20+ messages in thread
From: Tom Lane @ 2023-10-20 19:03 UTC (permalink / raw)
To: Laurenz Albe <laurenz.albe@cybertec.at>; +Cc: Vik Fearing <vik@postgresfriends.org>; Dennis Brouwer <dennis.brouwer@kizitos.com>; John Naylor <john.naylor@enterprisedb.com>; David Micallef <david.j.micallef@gmail.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; pgsql-bugs@lists.postgresql.org
Laurenz Albe <laurenz.albe@cybertec.at> writes:
> I agree that it is desirable to fix this regression.
It's not a regression. If anything, it's a bug fix, because the
code is now doing what it intended to do all along: reject all
non-integral constants. The previous behavior was exposing an
implementation detail, namely that "true" wasn't a simple literal
constant according to the older grammar. But all of these were
and still are rejected:
GROUP BY 'true';
GROUP BY 1.0;
GROUP BY null;
I'm not really satisfied with concluding that we need to be
bug-compatible (literally) with an old implementation wart forever.
> Still, we shouldn't exonerate Hibernate from fixing the junk SQL
> it produces.
I'm curious as to how this incompatibility escaped notice for a full
year. Does Hibernate emit this SQL only in narrow corner cases?
regards, tom lane
^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2023-10-20 19:03 UTC | newest]
Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-08-28 06:27 group by true now errors with non-integer constant in GROUP BY David Micallef <david.j.micallef@gmail.com>
2023-08-28 09:24 ` John Naylor <john.naylor@enterprisedb.com>
2023-08-28 19:56 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-08-28 21:20 ` Andrew Dunstan <andrew@dunslane.net>
2023-08-28 23:47 ` David Rowley <dgrowleyml@gmail.com>
2023-08-29 01:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-08-29 01:54 ` David Rowley <dgrowleyml@gmail.com>
2023-08-29 06:39 ` John Naylor <john.naylor@enterprisedb.com>
2023-09-17 23:36 ` David Rowley <dgrowleyml@gmail.com>
2023-09-18 00:00 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-09-18 00:22 ` David Rowley <dgrowleyml@gmail.com>
2023-09-21 04:58 ` Peter Geoghegan <pg@bowt.ie>
2023-09-21 07:06 ` David Rowley <dgrowleyml@gmail.com>
2023-10-19 12:07 ` Dennis Brouwer <dennis.brouwer@kizitos.com>
2023-10-19 13:18 ` Laurenz Albe <laurenz.albe@cybertec.at>
2023-10-19 13:33 ` David G. Johnston <david.g.johnston@gmail.com>
2023-10-20 05:44 ` Vik Fearing <vik@postgresfriends.org>
2023-10-20 06:20 ` Vik Fearing <vik@postgresfriends.org>
2023-10-20 06:32 ` Laurenz Albe <laurenz.albe@cybertec.at>
2023-10-20 19:03 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox