Date: Sat, 8 Feb 2020 15:56:35 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, florisvannee@optiver.com,
	pg@bowt.ie, jesper.pedersen@redhat.com,
	david.rowley@2ndquadrant.com, alvherre@2ndquadrant.com,
	thomas.munro@gmail.com, jtc331@gmail.com, rafia.pghackers@gmail.com,
	jeff.janes@gmail.com, bhushan.uparkar@gmail.com,
	pgsql-hackers@postgresql.org, a.korotkov@postgrespro.ru
Subject: Re: Index Skip Scan
Message-ID: <20200208145635.5cgmvgz2gsyvqpgj@development>
References: <20200205163730.eqvoxh6eb3do52r5@localhost>
 <20200206.102450.1514793516830965023.horikyota.ntt@gmail.com>
 <20200206105707.fuidmdsi6oapieoz@localhost>
 <20200206.212220.642002310227620136.horikyota.ntt@gmail.com>
 <20200207162543.zlj2tvcwy7jku6vu@localhost>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20200207162543.zlj2tvcwy7jku6vu@localhost>
Precedence: bulk

OK,

A couple more comments based on quick review of the patch, particularly
the part related to planning:

1) create_skipscan_unique_path has one assert commented out. Either it's
something we want to enforce, or we should remove it.

   /*Assert(distinctPrefixKeys <= list_length(pathnode->path.pathkeys));*/


2) I wonder if the current costing model is overly optimistic. We simply
copy the startup cost from the IndexPath, which seems fine. But for
total cost we do this:

   pathnode->path.total_cost = basepath->startup_cost * numGroups;

which seems a bit too simplistic. The startup cost is pretty much just
the cost to find the first item in the index, but surely we need to do
more to find the next group - we need to do comparisons to skip some of
the items, etc. If we think that's unnecessary, we need to explain it in
a comment or somthing.


3) I don't think we should make planning dependent on hasDeclareCursor.


4) I'm not quite sure how sensible it's to create a new IndexPath in
create_skipscan_unique_path. On the one hand it works, but I don't think
any other path is constructed like this so I wonder if we're missing
something. Perhaps it'd be better to just add a new path node on top of
the IndexPath, and then handle this in create_plan. We already do
something similar for Bitmap Index Scans, where we create a different
executor node from IndexPath depending on the parent node.


regards

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