Date: Thu, 12 Mar 2020 18:40:15 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: James Coleman <jtc331@gmail.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>, pgsql-hackers@lists.postgresql.org,
	Alvaro Herrera <alvherre@2ndquadrant.com>
Subject: Re: [PATCH] Incremental sort (was: PoC: Partial sort)
Message-ID: <20200312234014.GI29065@telsasoft.com>
References: <20190909215538.42hrv6j4z5ztcft4@development>
 <CAAaqYe-L4a2mj=AqguVjdkf65LZ772C-GbKgAj7CQro8rftXBg@mail.gmail.com>
 <20190928230049.k4l2mayun7jomhxy@development>
 <20191129060146.GH2505@paquier.xyz>
 <20191201004716.cenkc33em2lnrrdl@development>
 <20200121142529.wpickvfefp7lok4p@development>
 <CAAaqYe-=w0RaadtaTxfwuVa7GK+8QS7D51YzmfJLX9YZGfqc4A@mail.gmail.com>
 <CAAaqYe_ctGqQsauuYS5StPULkES7=t8vNwvEPyzXQdbjAuZ6vA@mail.gmail.com>
 <CAAaqYe-BV=kuMmwHCnOfdAG_mKjCo=41MeFrdtBjB+iXd6-FJg@mail.gmail.com>
 <20200311024355.5h2v56jpgzvd7yox@development>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200311024355.5h2v56jpgzvd7yox@development>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

Thanks for working on this.  I have some minor comments.

In 0005:

+				/* Restore the input path (we might have addes Sort on top). */

=> added?  There's at least two more of the same typo.

+				/* also ignore already sorted paths */

=> You say that in a couple places, but I don't think "also" makes sense since
there's nothing preceding it ?

In 0004:

+			 * end up resorting the entire data set.  So, unless we can push

=> re-sorting

+ * Unlike generate_gather_paths, this does not look just as pathkeys of the

=> look just AT ?

+			/* now we know is_sorted == false */

=> I would just spell that "Assert", as I think you already do elsewhere.

+				/* continue */

=> Please consider saying "fall through", since "continue" means exactly the
opposite.


+generate_useful_gather_paths(PlannerInfo *root, RelOptInfo *rel, bool override_rows)
...
+			/* finally, consider incremental sort */
...
+				/* Also consider incremental sort. */

=> I think it's more confusing than useful with two comments - one is adequate.

In 0002:

+ * If it's EXPLAIN ANALYZE, show tuplesort stats for a incremental sort node
...
+ * make_incrementalsort --- basic routine to build a IncrementalSort plan node

=> AN incremental

+ * Initial size of memtuples array.  We're trying to select this size so that
+ * array don't exceed ALLOCSET_SEPARATE_THRESHOLD and overhead of allocation
+ * be possible less.  However, we don't cosider array sizes less than 1024

Four typos (?)
that array DOESN'T
and THE overhead
CONSIDER
I'm not sure, but "be possible less" should maybe say "possibly be less" ?

+	bool		maxSpaceOnDisk;	/* true when maxSpace is value for on-disk

I suggest to call it IsMaxSpaceDisk

+	MemoryContext maincontext;	/* memory context for tuple sort metadata
+					   that persist across multiple batches */

persists

+ *	a new sort.  It allows evade recreation of tuple sort (and save resources)
+ *	when sorting multiple small batches.

allows to avoid?  Or allows avoiding?

+ *	 When performing sorting by multiple keys input dataset could be already
+ *	 presorted by some prefix of these keys.  We call them "presorted keys".

"already presorted" sounds redundant

+	int64		fullsort_group_count;	/* number of groups with equal presorted keys */
+	int64		prefixsort_group_count;	/* number of groups with equal presorted keys */

I guess these should have different comments

-- 
Justin