From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Peter Geoghegan <pg@bowt.ie>
Cc: James Coleman <jtc331@gmail.com>
Cc: Simon Riggs <simon@2ndquadrant.com>
Cc: Rafia Sabih <rafia.pghackers@gmail.com>
Cc: Shaun Thomas <shaun.thomas@2ndquadrant.com>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Date: Tue, 25 Jun 2019 22:32:07 +0200
Message-ID: <20190625203207.lvmmat7d2ilnr64t@development> (raw)
In-Reply-To: <CAH2-Wz=L8dZNttnvSXvH6NTMWw2T5VsBBC4oU4tLTMV9CoLRvg@mail.gmail.com>
References: <CAAaqYe_PXXHnOavS_X+C+yJnWesQzWeCv80jsp33SW4osGRUWg@mail.gmail.com>
<CANP8+jJaGHCQb8aBuywkz8Ev9WJwSZrMkmtr2WciCFazhtsgcw@mail.gmail.com>
<CAAaqYe9N=BZDJ2U9FsRX1-foJu-a8u1t1KGYtvf=zJdUAO9JdA@mail.gmail.com>
<20190624201601.iqy3ddig72l5lvbz@development>
<CAAaqYe9pzNZnF328bFZfzR8zJK9Mvft3i6TN0NjxjCkWeVm1iQ@mail.gmail.com>
<20190625160235.6ctryjzvw5je7cet@development>
<CAAaqYe_JMQh0FeSSynr_j66ZNNiX3BjKoLiMgMF4zna1m0tcVg@mail.gmail.com>
<CAH2-Wz=ptCW_dnRi3z4n8eUCzc82Y2qsd9ReWLr0-nxfhh=Hsg@mail.gmail.com>
<CAAaqYe_pPWgcgjM5OVkwwgDPn9e63ZjFbv6ychiuHF_22B-FjQ@mail.gmail.com>
<CAH2-Wz=L8dZNttnvSXvH6NTMWw2T5VsBBC4oU4tLTMV9CoLRvg@mail.gmail.com>
On Tue, Jun 25, 2019 at 12:13:01PM -0700, Peter Geoghegan wrote:
>On Tue, Jun 25, 2019 at 11:03 AM James Coleman <jtc331@gmail.com> wrote:
>> No, I haven't confirmed that it's called less frequently, and I'd be
>> extremely surprised if it were given the diff doesn't suggest any
>> changes to that at all.
>
>I must have misunderstood, then. I thought that you were suggesting
>that that might have happened.
>
>> If you think it's important enough to do so, I can instrument it to
>> confirm, but I was mostly wanting to know if there were any other
>> plausible explanations, and I think you've provided one: there *are*
>> changes in the patch to memory contexts in tuplesort.c, so if memory
>> fragmentation is a real concern this patch could definitely notice
>> changes in that regard.
>
>Sounds like it's probably fragmentation. That's generally hard to measure.
>
I'm not sure I'm really conviced this explains the difference, because
the changes in tuplesort.c are actually fairly small - we do split the
tuplesort context into two, but vast majority of the stuff is allocated
in one of the contexts (essentially just the tuplesort state gets moved
to a new context). I wouldn't expect this to have such strong impact on
locality/fragmentation.
But maybe it does - in that case it seems it might be worthwile to do it
separately, irrespectedly of the incremental sort patch. I wonder if
perf would show that as cache hits/misses, or something?
It shouldn't be that difficult to separate this change into a separate
patch, and benchmark it on it's own, though.
FWIW while looking at the tuplesort.c changes, I've noticed some
inaccurate comments in tuplesort_free. Firstly, the top-level comment
says:
/*
* tuplesort_free
*
* Internal routine for freeing resources of tuplesort.
*/
without mentioning which resources it actually releases, so it kinda
suggests it releases everything. But that's not true - AFAICS it only
releases the per-sort resources. IMO this is a poor function name, and
people will easily keep resources longer than they think - we should
rename it to something like tuplesort_free_batch().
And then at the end tuplesort_free() does this:
/*
* Free the per-sort memory context, thereby releasing all working memory,
* including the Tuplesortstate struct itself.
*/
MemoryContextReset(state->sortcontext);
But that's clearly not true, because the tuplesortstate is allocated in
the maincontext, not sortcontext.
In general, the comments seem to be a bit confused by what 'sort' means.
Sometimes it means the whole sort operation, sometimes it means one of
the batches, etc. And the fact that the per-batch context is called
sortcontext does not really improve the situation.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, pg@bowt.ie, jtc331@gmail.com, simon@2ndquadrant.com, rafia.pghackers@gmail.com, shaun.thomas@2ndquadrant.com, 9erthalion6@gmail.com, a.korotkov@postgrespro.ru, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: [PATCH] Incremental sort (was: PoC: Partial sort)
In-Reply-To: <20190625203207.lvmmat7d2ilnr64t@development>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox