Date: Sat, 5 Dec 2020 18:55:42 +0100
From: Dmitry Dolgov <9erthalion6@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Peter Geoghegan <pg@bowt.ie>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Jesper Pedersen <jesper.pedersen@redhat.com>,
	David Rowley <dgrowleyml@gmail.com>,
	Floris Van Nee <florisvannee@optiver.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Thomas Munro <thomas.munro@gmail.com>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	Andy Fan <zhihui.fan1213@gmail.com>,
	Dilip Kumar <dilipbalaut@gmail.com>
Subject: Re: Index Skip Scan (new UniqueKeys)
Message-ID: <20201205175542.fqrhbhp3co7cgvkj@localhost>
References: <20200711161258.5modwkdeszhf55ux@localhost>
 <CAH2-Wzkyx9A28bb5yNiN+6iRDJpGS0sQdAhyWEAA_yjrsh98-Q@mail.gmail.com>
 <20200727102431.mjspwec4yhzeyny2@localhost>
 <20200815141240.wuyc6ijkmnywywtn@localhost>
 <CAH2-Wz=z8yXENbUWgP+3PhfS7HfB9bj01XuuSY8bj4jasQj_3A@mail.gmail.com>
 <20201006152039.xs2ul7il52sowhlu@localhost>
 <20201024164553.tel5uynf2dzyh4az@localhost>
 <598ff988-5ebb-f6f7-0c9d-82208dff3bbd@iki.fi>
 <20201201202119.jcsr7z6cdiffuops@localhost>
 <6020ad2b-357f-da90-de7b-60b79452fc66@iki.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6020ad2b-357f-da90-de7b-60b79452fc66@iki.fi>
Precedence: bulk

> On Tue, Dec 01, 2020 at 10:59:22PM +0200, Heikki Linnakangas wrote:
>
> > > - Does this optimization apply to bitmap index scans?
> >
> > No, from what I understand it doesn't.
>
> Would it be hard to add? Don't need to solve everything in the first
> version of this, but I think in principle you could do the same
> optimization for bitmap index scans, so if the current API can't do it,
> that's maybe an indication that the API isn't quite right.

I would expect it should not be hard as at the moment all parts seems
relatively generic. But of course I need to check, while it seems no one
had bitmap index scans in mind while developing this patch.

> > > I'm actually a bit confused why we need this condition. The IndexScan
> > > executor node should call amskip() only after checking the additional quals,
> > > no?
> >
> > This part I don't quite get, what exactly you mean by checking the
> > additional quals in the executor node? But at the end of the day this
> > condition was implemented exactly to address the described issue, which
> > was found later and added to the tests.
>
> As I understand this, the executor logic goes like this:
>
> query: SELECT DISTINCT ON (a, b)  a, b FROM foo where c like '%y%' and a
> like 'a%' and b = 'b';
>
> 1. Call index_beginscan, keys: a >= 'a', b = 'b'
>
> 2. Call index_getnext, which returns first row to the Index Scan node
>
> 3. Evaluates the qual "c like '%y%'" on the tuple. If it's false, goto step
> 2 to get next tuple.
>
> 4. Return tuple to parent node
>
> 5. index_amskip(), to the next tuple with a > 'a'. Goto 2.
>
> The logic should work fine, even if there are quals that are not indexable,
> like "c like '%y'" in the above example. So why doesn't it work? What am I
> missing?

To remind myself how it works I went through this sequence, and from
what I understand the qual "c like '%y%'" is evaluated in this case in
ExecQual, not after index_getnext_tid (and values returned after
skipping are reported as filtered out). So when it comes to index_skip
only quals on a & b were evaluated. Or did you mean something else?

Another small detail is that in the current implementation there is no
goto 2 in the last step. Originally it was like that, but since skipping
return an exact position that we need there was something like "find a
value, then do one step back so that index_getnext will find it".
Unfortunately this stepping back part turns out to be a source of
troubles, and getting rid of it even allowed to make code somewhat more
concise. But of course I'm open for suggestions about improvements.