Date: Tue, 31 Mar 2020 23:53:40 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: James Coleman <jtc331@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
	Michael Paquier <michael@paquier.xyz>,
	Rafia Sabih <rafia.pghackers@gmail.com>,
	Peter Geoghegan <pg@bowt.ie>, Simon Riggs <simon@2ndquadrant.com>,
	Shaun Thomas <shaun.thomas@2ndquadrant.com>,
	Dmitry Dolgov <9erthalion6@gmail.com>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>,
	Andreas Karlsson <andreas@proxel.se>
Subject: Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Message-ID: <20200331215340.bbbrccbehmjb7dbx@development>
References: 
 <CAAaqYe_P9sJ4ACuaHRU-+pq0HAg76ei052BsUfuWjA9EwSkGMw@mail.gmail.com>
 <20200329031437.yvhogo25wdijwpqd@development>
 <CAAaqYe-rLqNBeCFq0Ruw4RUs5h1xbO2B1BGnjLq0k5Sw3a2gTg@mail.gmail.com>
 <CAAaqYe-M==BwbC47LeMM2HcUGHqCdynNMMyncrBa7t0AGzZGWQ@mail.gmail.com>
 <20200330014442.chqnwfmelm3ejsgr@development>
 <CAAaqYe__vp0QQHgUfiVe+NrcMpECfQgZO8iu2CEwVOcBgA+S0Q@mail.gmail.com>
 <20200330122426.wjdfpqiatv4ybui7@development>
 <CAAaqYe8hK3FpBRkAtTn1L_sETMUivoR=knoLf5by6Q7yLZnzTg@mail.gmail.com>
 <20200331011407.vwz3ektatgqyi6vy@development>
 <CAAaqYe8M-r4esHOg_46w3ojeUEB7KGZ4j2GAi6rLv0B9bHoiPg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAAaqYe8M-r4esHOg_46w3ojeUEB7KGZ4j2GAi6rLv0B9bHoiPg@mail.gmail.com>
Precedence: bulk

On Tue, Mar 31, 2020 at 02:23:15PM -0400, James Coleman wrote:
>On Mon, Mar 30, 2020 at 9:14 PM Tomas Vondra
><tomas.vondra@2ndquadrant.com> wrote:
>>
>> The main thing I've been working on today is benchmarking how this
>> affects planning. And I'm seeing a regression that worries me a bit,
>> unfortunately.
>>
>> The test I'm doing is pretty simple - build a small table with a bunch
>> of columns:
>>
>>    create table t (a int, b int, c int, d int, e int, f int, g int);
>>
>>    insert into t select 100*random(), 100*random(), 100*random(),
>>      100*random(), 100*random(), 100*random(), 100*random()
>>    from generate_series(1,100000) s(i);
>>
>> and then a number of indexes on subsets of up to 3 columns, as generated
>> using the attached build-indexes.py script. And then run a bunch of
>> explains (so no actual execution) sorting the data by at least 4 columns
>> (to trigger incremental sort paths), measuring timing of the script.
>>
>> I did a bunch of runs on current master and v46 with incremental sort
>> disabled and enabled, and the results look like this:
>>
>>      master       off        on
>>      --------------------------
>>      34.609    37.463    37.729
>>
>> which means about 8-9% regression with incremental sort. Of course, this
>> is only for planning time, for execution the impact is going to be much
>> smaller. But it's still a bit annoying.
>>
>> I've suspected this might be either due to the add_partial_path changes
>> or the patch adding incremental sort to additional places, so I tested
>> those parts individually and the answer is no - add_partial_path changes
>> have very small impact (~1%, which might be noise). The regression comes
>> mostly from the 0002 part that adds incremental sort. At least in this
>> particular test - different tests might behave differently, of course.
>>
>> The annoying bit is that the overhead does not disappear after disabling
>> incremental sort. That suggests this is not merely due to considering
>> and creating higher number of paths, but due to something that happens
>> before we even look at the GUC ...
>>
>> I think I've found one such place - if you look at compare_pathkeys, it
>> has this check right before the forboth() loop:
>>
>>      if (keys1 == keys2)
>>          return PATHKEYS_EQUAL;
>>
>> But with incremental sort we don't call pathkeys_contained_in, we call
>> pathkeys_common_contained_in instead. And that does not call
>> compare_pathkeys and does not have the simple equality check. Adding
>> the following check seems to cut the overhead in half, which is nice:
>>
>>      if (keys1 == keys2)
>>      {
>>          *n_common = list_length(keys1);
>>         return true;
>>      }
>>
>> Not sure where the rest of the regression comes from yet.
>
>I noticed in the other function we also optimize by checking if either
>keys list is NIL, so I tried adding that, and it might have made a
>minor difference, but it's hard to tell as it was under 1%.
>

Which other function? I don't see this optimization in compare_pathkeys,
that only checks key1/key2 after the forboth loop, but that's something
different.

I may be wrong, but my guess would be that this won't save much, because
the loop should terminate immediately. The whole point is not to loop
over possibly many pathkeys when we know it's exactly the same pathkeys
list (same pointer). When one of the lists is NIL the loop should
terminate right away ...

>I also ran perf with a slightly modified version of your test that
>uses psql, and after the above changes was seeing something like a
>3.5% delta between master and this patch series. Nothing obvious in
>the perf report though.
>

Yeah, I don't think there's anything obviously more expensive.

>This test is intended to be somewhat worst case, no? At what point do
>we consider the trade-off worth it (given that it's not plausible to
>have zero impact)?
>

Yes, more or less. It was definitely designed to do that - it merely
plans the query (no execution), with many applicable indexes etc. It's
definitely true that once the exection starts to take more time, the
overhead will get negligible. Same for reducing number of indexes.

And of course, for queries that can benefit from incremental sort, the
speedup is likely way larger than this.

In general, I think it'd be naive that we can make planner smarter with
no extra overhead spent on planning, and we can never accept patches
adding even tiny overhead. With that approach we'd probably end up with
a trivial planner that generates just a single query plan, because
that's going to be the fastest planner. A realistic approach needs to
consider both the planning and execution phase, and benefits of this
patch seem to be clear - if you have queries that do benefit from it.

Let's try to minimize the overhead a bit more, and then we'll see.


>> Also, while looking at pathkeys_common_contained_in(), I've been a bit
>> puzzled why does is this correct:
>>
>>      return (key1 == NULL);
>>
>> It's easy to not notice it's key1 and not keys1, so I suggest we add a
>> comment 'key1 == NULL' means we've processed whole keys1 list.
>
>Done.
>
>I've included fixes for Alvaro's comments in this patch series also.
>

OK, thanks.

regards

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