Date: Sat, 11 Jul 2020 18:12:58 +0200
From: Dmitry Dolgov <9erthalion6@gmail.com>
To: Peter Geoghegan <pg@bowt.ie>
Cc: 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: <20200711161258.5modwkdeszhf55ux@localhost>
References: <20200609102247.jdlatmfyeecg52fi@localhost>
 <CAH2-WznUY=0_yYek3ciNv4-ObotrWokAZbGnWjEsqxZ4oMWCGg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAH2-WznUY=0_yYek3ciNv4-ObotrWokAZbGnWjEsqxZ4oMWCGg@mail.gmail.com>
Precedence: bulk

> On Wed, Jul 08, 2020 at 03:44:26PM -0700, Peter Geoghegan wrote:
>
> On Tue, Jun 9, 2020 at 3:20 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> > * Btree-implementation contains btree specific code to implement amskip,
> >   introduced in the previous patch.
>
> The way that you're dealing with B-Tree tuples here needs to account
> for posting list tuples:
>
> > +               currItem = &so->currPos.items[so->currPos.lastItem];
> > +               itup = (IndexTuple) (so->currTuples + currItem->tupleOffset);
> > +               nextOffset = ItemPointerGetOffsetNumber(&itup->t_tid);

Do you mean this last part with t_tid, which could also have a tid array
in case of posting tuple format?

> > +               /*
> > +                * To check if we returned the same tuple, try to find a
> > +                * startItup on the current page. For that we need to update
> > +                * scankey to match the whole tuple and set nextkey to return
> > +                * an exact tuple, not the next one. If the nextOffset is the
> > +                * same as before, it means we are in the loop, return offnum
> > +                * to the original position and jump further
> > +                */
>
> Why does it make sense to use the offset number like this? It isn't
> stable or reliable. The patch goes on to do this:
>
> > +                   startOffset = _bt_binsrch(scan->indexRelation,
> > +                                             so->skipScanKey,
> > +                                             so->currPos.buf);
> > +
> > +                   page = BufferGetPage(so->currPos.buf);
> > +                   maxoff = PageGetMaxOffsetNumber(page);
> > +
> > +                   if (nextOffset <= startOffset)
> > +                   {
>
> Why compare a heap TID's offset number (an offset number for a heap
> page) to another offset number for a B-Tree leaf page? They're
> fundamentally different things.

Well, it's obviously wrong, thanks for noticing. What is necessary is to
compare two index tuples, the start and the next one, to test if they're
the same (in which case if I'm not mistaken probably we can compare item
pointers). I've got this question when I was about to post a new version
with changes to address feedback from Andy, now I'll combine them and
send a cumulative patch.