From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrew Gierth <andrew@tao11.riddles.org.uk>
cc: pgsql-hackers@postgresql.org
Subject: Re: parseCheckAggregates vs. assign_query_collations
In-reply-to: <87muo0k0c7.fsf@news-spur.riddles.org.uk>
References: <87muo0k0c7.fsf@news-spur.riddles.org.uk>
Comments: In-reply-to Andrew Gierth <andrew@tao11.riddles.org.uk>
	message dated "Wed, 16 Jan 2019 17:02:07 +0000"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <16968.1547660970.1@sss.pgh.pa.us>
Date: Wed, 16 Jan 2019 12:49:30 -0500
Message-ID: <16969.1547660970@sss.pgh.pa.us>
Precedence: bulk

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