From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: David Rowley <david.rowley@2ndquadrant.com>
Cc: Jesper Pedersen <jesper.pedersen@redhat.com>
Cc: Floris Van Nee <florisvannee@optiver.com>
Cc: Rafia Sabih <rafia.pghackers@gmail.com>
Cc: Jeff Janes <jeff.janes@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: Bhushan Uparkar <bhushan.uparkar@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: James Coleman <jtc331@gmail.com>
Subject: Re: Index Skip Scan
Date: Sun, 23 Jun 2019 15:10:31 +0200
Message-ID: <20190623131031.65zmz2m4waew6paa@development> (raw)
In-Reply-To: <CA+q6zcVVigO_1KRBxvzKfnR593CEE7FKR-bwP-XcH6AVHOo4+g@mail.gmail.com>
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>
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
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, 9erthalion6@gmail.com, david.rowley@2ndquadrant.com, jesper.pedersen@redhat.com, florisvannee@optiver.com, rafia.pghackers@gmail.com, jeff.janes@gmail.com, thomas.munro@gmail.com, a.kuzmenkov@postgrespro.ru, pg@bowt.ie, thomas.munro@enterprisedb.com, bhushan.uparkar@gmail.com, a.korotkov@postgrespro.ru, jtc331@gmail.com
Subject: Re: Index Skip Scan
In-Reply-To: <20190623131031.65zmz2m4waew6paa@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