Date: Wed, 27 May 2020 11:07:04 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Jeff Davis <pgsql@j-davis.com>, Thomas Munro <thomas.munro@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
	HeikkiLinnakangas <hlinnaka@iki.fi>
Subject: Re: Trouble with hashagg spill I/O pattern and costing
Message-ID: <20200527090704.onkjogxeg34ib5cn@development>
References: <20200525021045.dilgcsmgiu4l5jpa@development>
 <20200525121722.6wzcelweefyuwg54@development>
 <2df2e0728d48f498b9d6954b5f9080a34535c385.camel@j-davis.com>
 <20200525225909.aifxjm53hw4xyzxd@development>
 <CA+hUKG+q3Ma+sTKn2316Ofof6u21hh0cGJHcJ-fAnLxw=LXevw@mail.gmail.com>
 <20200526141524.ohtvrp23vxosgivl@development>
 <e913544f6c236d37e8b0597cabd086570b5806f2.camel@j-davis.com>
 <20200526191511.d7pwz4awrvdm4p6j@development>
 <0589f869969de2f3e9713689159c7c4fbe1fcfa5.camel@j-davis.com>
 <CAAKRu_YNGQ-zw9rpXGtYhURMqdPk+HnMa+Wj-tnNx9cwENgDxg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAAKRu_YNGQ-zw9rpXGtYhURMqdPk+HnMa+Wj-tnNx9cwENgDxg@mail.gmail.com>
Precedence: bulk

On Tue, May 26, 2020 at 06:42:50PM -0700, Melanie Plageman wrote:
>On Tue, May 26, 2020 at 5:40 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
>> On Tue, 2020-05-26 at 21:15 +0200, Tomas Vondra wrote:
>> >
>> > As for the tlist fix, I think that's mostly ready too - the one thing
>> > we
>> > should do is probably only doing it for AGG_HASHED. For AGG_SORTED
>> > it's
>> > not really necessary.
>>
>> Melanie previously posted a patch to avoid spilling unneeded columns,
>> but it introduced more code:
>>
>>
>>
>> https://www.postgresql.org/message-id/CAAKRu_aefEsv+UkQWqu+ioEnoiL2LJu9Diuh9BR8MbyXuZ0j4A@mail.gmail.com
>>
>> and it seems that Heikki also looked at it. Perhaps we should get an
>> acknowledgement from one of them that your one-line change is the right
>> approach?
>>
>>
>I spent some time looking at it today, and, it turns out I was wrong.
>
>I thought that there was a case I had found where CP_SMALL_TLIST did not
>eliminate as many columns as could be eliminated for the purposes of
>spilling, but, that turned out not to be the case.
>
>I changed CP_LABEL_TLIST to CP_SMALL_TLIST in
>create_groupingsets_plan(), create_agg_plan(), etc and tried a bunch of
>different queries and this 2-3 line change worked for all the cases I
>tried. Is that where you made the change?

I've only made the change in create_agg_plan, because that's what was in
the query plan I was investigating. You may be right that the same fix
is needed in additional places, though.

>And then are you proposing to set it based on the aggstrategy to either
>CP_LABEL_TLIST or CP_SMALL_TLIST here?
>

Yes, something like that. The patch I shared on on 5/21 just changed
that, but I'm wondering if that could add overhead for sorted
aggregation, which already does the projection thanks to the sort.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services