Date: Sun, 23 Jun 2019 15:10:31 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: David Rowley <david.rowley@2ndquadrant.com>,
	Jesper Pedersen <jesper.pedersen@redhat.com>,
	Floris Van Nee <florisvannee@optiver.com>,
	Rafia Sabih <rafia.pghackers@gmail.com>,
	Jeff Janes <jeff.janes@gmail.com>,
	Thomas Munro <thomas.munro@gmail.com>,
	Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru>,
	Peter Geoghegan <pg@bowt.ie>,
	Thomas Munro <thomas.munro@enterprisedb.com>,
	Bhushan Uparkar <bhushan.uparkar@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>,
	James Coleman <jtc331@gmail.com>
Subject: Re: Index Skip Scan
Message-ID: <20190623131031.65zmz2m4waew6paa@development>
References: <1559362223126.18661@Optiver.com>
 <CA+FpmFeKh9BAhDTAf-ZWQ=pvVaRy+Lnahb_8=OOFqcVXJDBWWQ@mail.gmail.com>
 <988b7763-5c55-1be1-b7b6-f3a68eff0d40@redhat.com>
 <CA+q6zcVnRRqktw4cg2Vddy3S+VY3Tsmit0kausYtctvsTum7tA@mail.gmail.com>
 <1559763453006.85615@Optiver.com>
 <1559763570329.85227@Optiver.com>
 <4d469bf2-49ff-bc3d-a428-6c7b5d87d526@redhat.com>
 <CAKJS1f86FgODuUnHiQ25RKeuES4qTqeNxm1QbqJWrBoZxVGLiQ@mail.gmail.com>
 <CA+q6zcXoYjCtyU-jFU_=p2DL_K3BHbBEEbJHaJTxdvRX3cU-Sg@mail.gmail.com>
 <CA+q6zcVVigO_1KRBxvzKfnR593CEE7FKR-bwP-XcH6AVHOo4+g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CA+q6zcVVigO_1KRBxvzKfnR593CEE7FKR-bwP-XcH6AVHOo4+g@mail.gmail.com>
User-Agent: NeoMutt/20180716-1444-295967
Precedence: bulk

Hi,

I've done some initial review on v20 - just reading through the code, no
tests at this point. Here are my comments:


1) config.sgml

I'm not sure why the enable_indexskipscan section says

    This parameter requires that <varname>enable_indexonlyscan</varname>
    is <literal>on</literal>.

I suppose it's the same thing as for enable_indexscan, and we don't say
anything like that for that GUC.


2) indices.sgml

The new section is somewhat unclear and difficult to understand, I think
it'd deserve a rewording. Also, I wonder if we really should link to the
wiki page about FSM problems. We have a couple of wiki links in the sgml
docs, but those seem more generic while this seems as a development page
that might disapper. But more importantly, that wiki page does not say
anything about "Loose Index scans" so is it even the right wiki page?


3) nbtsearch.c

  _bt_skip - comments are formatted incorrectly
  _bt_update_skip_scankeys - missing comment
  _bt_scankey_within_page - missing comment

4) explain.c

There are duplicate blocks of code for IndexScan and IndexOnlyScan:

    if (indexscan->skipPrefixSize > 0)
    {
        if (es->format != EXPLAIN_FORMAT_TEXT)
            ExplainPropertyInteger("Distinct Prefix", NULL,
                                   indexscan->skipPrefixSize,
                                   es);
    }

I suggest we wrap this into a function ExplainIndexSkipScanKeys() or
something like that.

Also, there's this:

    if (((IndexScan *) plan)->skipPrefixSize > 0)
    {
        ExplainPropertyText("Scan mode", "Skip scan", es);
    }

That does not make much sense - there's just a single 'scan mode' value.
So I suggest we do the same thing as for unique joins, i.e. 

    ExplainPropertyBool("Skip Scan",
                        (((IndexScan *) plan)->skipPrefixSize > 0),
                        es);


5) nodeIndexOnlyScan.c

In ExecInitIndexOnlyScan, we should initialize the ioss_ fields a bit
later, with the existing ones. This is just cosmetic issue, though.


6) nodeIndexScan.c

I wonder why we even add and initialize the ioss_ fields for IndexScan
nodes, when the skip scans require index-only scans?


7) pathnode.c

I wonder how much was the costing discussed. It seems to me the logic is
fairly similar to ideas discussed in the incremental sort patch, and
we've been discussing some weak points there. I'm not sure how much we
need to consider those issues here.


8) execnodes.h

The comment before IndexScanState mentions new field NumDistinctKeys,
but there's no such field added to the struct.


9) pathnodes.h

I don't understand what the uniq_distinct_pathkeys comment says :-(


10) plannodes.h

The naming of the new field (skipPrefixSize) added to IndexScan and
IndexOnlyScan is clearly inconsistent with the naming convention of the
existing fields.


regards

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