Date: Tue, 16 Aug 2022 20:57:55 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Richard Guo <guofenglinux@gmail.com>,
	Ronan Dunklau <ronan.dunklau@aiven.io>,
	pgsql-hackers@lists.postgresql.org,
	Ranier Vilela <ranier.vf@gmail.com>
Subject: Re: Add proper planner support for ORDER BY / DISTINCT aggregates
Message-ID: <20220817015755.GB26426@telsasoft.com>
References: <3163474.aeNJFYEL58@aivenronan>
 <CAApHDvqS-p57j++gLMyQXmttk_MM3u243dWQ54FyW-7=4GP+1Q@mail.gmail.com>
 <CAMbWs49hcB-Mj3MJTOesL5=y4oU+md=zEvDYdCBrOSaDvXLRGQ@mail.gmail.com>
 <CAApHDvr3JQv4_H7vfu5Djtw0ubKwGDwvNtPuphtaQY4utgj7bQ@mail.gmail.com>
 <CAMbWs4_hAK6+0Gk=vZX+ikSFBx=6981iFLGkgLObDkvvGpjogg@mail.gmail.com>
 <CAApHDvrai=tCuUhH=YUvDnysivzbabz4pGPGz_3SytysWmyNdg@mail.gmail.com>
 <CAMbWs49Xw1Sb4KOZx6CMGBjiCNvGCcAyXkwWRGNZivyYv2fGMg@mail.gmail.com>
 <CAApHDvqtmK7AprGs=uh2+BNyew6L3OpC7hPSe43uD-7m1Dtx7w@mail.gmail.com>
 <1138716.1659282591@sss.pgh.pa.us>
 <CAApHDvpQ+LmokunMogxc1Ba+AOptP=sHqruCWeC8Xgv2XDbiDg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAApHDvpQ+LmokunMogxc1Ba+AOptP=sHqruCWeC8Xgv2XDbiDg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220817015755.GB26426%40telsasoft.com>
Precedence: bulk

On Tue, Aug 02, 2022 at 11:21:04PM +1200, David Rowley wrote:
> I've now pushed the patch.

I've not studied the patch at all.

But in a few places, it removes the locally-computed group_pathkeys:

-                       List       *group_pathkeys = root->group_pathkeys;

However it doesn't do that here:

                /*
                 * Instead of operating directly on the input relation, we can
                 * consider finalizing a partially aggregated path.
                 */
                if (partially_grouped_rel != NULL)
                {
                        foreach(lc, partially_grouped_rel->pathlist)
                        {
                                ListCell   *lc2;
                                Path       *path = (Path *) lfirst(lc);
                                Path       *path_original = path;
 
                                List       *pathkey_orderings = NIL;
 
                                List       *group_pathkeys = root->group_pathkeys;

I noticed because that creates a new shadow variable, which seems accidental.

make src/backend/optimizer/plan/planner.o COPT=-Wshadow=compatible-local

src/backend/optimizer/plan/planner.c:6642:14: warning: declaration of ‘group_pathkeys’ shadows a previous local [-Wshadow=compatible-local]
 6642 |     List    *group_pathkeys = root->group_pathkeys;
      |              ^~~~~~~~~~~~~~
src/backend/optimizer/plan/planner.c:6438:12: note: shadowed declaration is here
 6438 |   List    *group_pathkeys;

-- 
Justin