Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lM4ww-0002t5-Ng for pgsql-hackers@arkaria.postgresql.org; Tue, 16 Mar 2021 08:21:11 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1lM4wv-0003y1-LM for pgsql-hackers@arkaria.postgresql.org; Tue, 16 Mar 2021 08:21:09 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lM4wv-0003xq-CD for pgsql-hackers@lists.postgresql.org; Tue, 16 Mar 2021 08:21:09 +0000 Received: from relay12.mail.gandi.net ([217.70.178.232]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lM4ws-0007QS-GW for pgsql-hackers@lists.postgresql.org; Tue, 16 Mar 2021 08:21:08 +0000 Received: from [172.16.255.146] (14.149.88.79.rev.sfr.net [79.88.149.14]) (Authenticated sender: vik@postgresfriends.org) by relay12.mail.gandi.net (Postfix) with ESMTPSA id C9B4B200017; Tue, 16 Mar 2021 08:21:03 +0000 (UTC) Subject: Re: GROUP BY DISTINCT To: Tomas Vondra , Georgios Kokolatos , pgsql-hackers@lists.postgresql.org Cc: Erik Rijkers References:

<161469758801.29967.4455067072614569957.pgcf@coridan.postgresql.org> <4294e82f-a134-5dbb-d9d6-03d72dce2e1e@postgresfriends.org> From: Vik Fearing Message-ID: <3817d5ec-f3e4-e4b9-d169-271ae5d3cbe4@postgresfriends.org> Date: Tue, 16 Mar 2021 09:21:03 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------B710932658C9224E4CB6159D" Content-Language: en-US List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This is a multi-part message in MIME format. --------------B710932658C9224E4CB6159D Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 3/13/21 12:33 AM, Tomas Vondra wrote: > Hi Vik, > > The patch seems quite ready, I have just two comments. Thanks for taking a look. > 1) Shouldn't this add another for DISTINCT, somewhere in the > documentation? Now the index points just to the SELECT DISTINCT part. Good idea; I never think about the index. > 2) The part in gram.y that wraps/unwraps the boolean flag as an integer, > in order to stash it in the group lists is rather ugly, IMHO. It forces > all the places handling the list to be aware of this (there are not > many, but still ...). And there are no other places doing (bool) intVal > so it's not like there's a precedent for this. There is kind of a precedent for it, I was copying off of TriggerEvents and func_alias_clause. > I think the clean solution is to make group_clause produce a struct with > two fields, and just use that. Not sure how invasive that will be > outside gram.y, though. I didn't want to create a whole new parse node for it, but Andrew Gierth pointed me towards SelectLimit so I did it like that and I agree it is much cleaner. > Also, the all_or_distinct vs. distinct_or_all seems a bit error-prone. I > wonder if we can come up with some clearer names, describing the context > of those types. I turned this into an enum for ALL/DISTINCT/default and the caller can choose what it wants to do with default. I think that's a lot cleaner, too. Maybe DISTINCT ON should be changed to fit in that? I left it alone for now. I also snuck in something that all of us overlooked which is outputting the DISTINCT in ruleutils.c. I didn't add a test for it but that would have been an unfortunate bug. New patch attached, rebased on 15639d5e8f. -- Vik Fearing --------------B710932658C9224E4CB6159D Content-Type: text/x-patch; charset=UTF-8; name="0001-implement-GROUP-BY-DISTINCT.v04.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-implement-GROUP-BY-DISTINCT.v04.patch"