pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedparseCheckAggregates vs. assign_query_collations
3+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2019-01-17 e74d8c5085 409230a721 624046abe2 91448e7dcc 174fab9934 d16d453870 Postpone aggregate checks until after collation is assigned.
* parseCheckAggregates vs. assign_query_collations
@ 2019-01-16 17:02 Andrew Gierth <andrew@tao11.riddles.org.uk>
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Gierth @ 2019-01-16 17:02 UTC (permalink / raw)
To: pgsql-hackers
Looking into a bug report on the -general list about grouping sets,
which turns out to be an issue of collation assignment: if the query has
CASE GROUPING(expr) WHEN 1 ...
then the expression is rejected as not matching the one in the GROUP BY
clause, because CASE already assigned collations to the expression (as a
special case in its transform function) while the rest of the query
hasn't yet had them assigned, because parseCheckAggregates gets run
before assign_query_collations.
I'll be looking into this in detail later, but right now, cam anyone
think of any reason why parseCheckAggregates couldn't be moved to after
assign_query_collations?
--
Andrew (irc:RhodiumToad)
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: parseCheckAggregates vs. assign_query_collations
@ 2019-01-16 17:49 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andrew Gierth <andrew@tao11.riddles.org.uk>
0 siblings, 1 reply; 3+ messages in thread
From: Tom Lane @ 2019-01-16 17:49 UTC (permalink / raw)
To: Andrew Gierth <andrew@tao11.riddles.org.uk>; +Cc: pgsql-hackers
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> Looking into a bug report on the -general list about grouping sets,
> which turns out to be an issue of collation assignment: if the query has
> CASE GROUPING(expr) WHEN 1 ...
> then the expression is rejected as not matching the one in the GROUP BY
> clause, because CASE already assigned collations to the expression (as a
> special case in its transform function) while the rest of the query
> hasn't yet had them assigned, because parseCheckAggregates gets run
> before assign_query_collations.
Bleah.
> I'll be looking into this in detail later, but right now, cam anyone
> think of any reason why parseCheckAggregates couldn't be moved to after
> assign_query_collations?
I never particularly liked assign_query_collations, as a matter of overall
system design. I'd prefer to nuke that and instead require collation
assignment to be done per-expression, ie at the end of transformExpr and
similar places. Now that we've seen this example, it's fairly clear why
collation assignment really should be considered an integral part of
expression parsing. Who's to say there aren't more gotchas of this sort
waiting to bite us? Also, if it were integrated into transformExpr as
it should have been to begin with, we would not have the need for quite
so many random calls to assign_expr_collations, with consequent bugs of
omission, cf 7a28e9aa0.
regards, tom lane
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: parseCheckAggregates vs. assign_query_collations
@ 2019-01-17 02:48 Andrew Gierth <andrew@tao11.riddles.org.uk>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 3+ messages in thread
From: Andrew Gierth @ 2019-01-17 02:48 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>> I'll be looking into this in detail later, but right now, cam anyone
>> think of any reason why parseCheckAggregates couldn't be moved to
>> after assign_query_collations?
Tom> I never particularly liked assign_query_collations, as a matter of
Tom> overall system design. I'd prefer to nuke that and instead require
Tom> collation assignment to be done per-expression, ie at the end of
Tom> transformExpr and similar places. Now that we've seen this
Tom> example, it's fairly clear why collation assignment really should
Tom> be considered an integral part of expression parsing. Who's to say
Tom> there aren't more gotchas of this sort waiting to bite us? Also,
Tom> if it were integrated into transformExpr as it should have been to
Tom> begin with, we would not have the need for quite so many random
Tom> calls to assign_expr_collations, with consequent bugs of omission,
Tom> cf 7a28e9aa0.
Sure, this might be the right approach going forward. But right now we
need a back-patchable fix, and the above sounds a bit too intrusive for
that.
Turns out the issue can be reproduced without grouping sets too:
select case a||'' when '1' then 0 else 1 end
from (select (select random()::text) as a) s group by a||'';
ERROR: column "s.a" must appear in the GROUP BY clause or be used in an aggregate function
select case when a||'' = '1' then 0 else 1 end
from (select (select random()::text) as a) s group by a||''; -- works
--
Andrew (irc:RhodiumToad)
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2019-01-17 02:48 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-01-16 17:02 parseCheckAggregates vs. assign_query_collations Andrew Gierth <andrew@tao11.riddles.org.uk>
2019-01-16 17:49 ` Tom Lane <tgl@sss.pgh.pa.us>
2019-01-17 02:48 ` Andrew Gierth <andrew@tao11.riddles.org.uk>
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