Re: Tid scan improvements

Hi, my apologies for the delay.

I've finished rebasing and rewriting it for Tom's changes to tidpath.c and
his recommendations for tid range scans, but I then found a bug with cursor
interaction. Specifically, FETCH LAST scans through the whole range, and
then proceeds to scan backwards to get the last row. It worked in both my
very first draft, and in the most recent draft before the changes to
tidpath, but I haven't got it working yet for the new version.

I'm hoping to get that fixed in the next 24 hours, and I'll then post the
new patch.

Edmund

On Sun, 3 Feb 2019 at 23:34, Andres Freund <andres(at)anarazel(dot)de> wrote:

> Hi,
>
> On 2019-01-19 17:04:13 +1300, Edmund Horner wrote:
> > On Sat, 19 Jan 2019 at 05:35, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > > Edmund Horner <ejrh00(at)gmail(dot)com> writes:
> > > > My patch uses the same path type and executor for all extractable
> > > tidquals.
> > >
> > > > This worked pretty well, but I am finding it difficult to
> reimplement it
> > > in
> > > > the new tidpath.c code.
> > >
> > > I didn't like that approach to begin with, and would suggest that you
> go
> > > over to using a separate path type and executor node. I don't think
> the
> > > amount of commonality for the two cases is all that large, and doing it
> > > as you had it required some ugly ad-hoc conventions about the semantics
> > > of the tidquals list. Where I think this should go is that the
> tidquals
> > > list still has OR semantics in the existing path type, but you use AND
> > > semantics in the new path type, so that "ctid > ? AND ctid < ?" is just
> > > represented as an implicit-AND list of two simple RestrictInfos.
> > >
> >
> > Thanks for the advice. This approach resembles my first draft, which
> had a
> > separate executor type. However, it did have a combined path type, with
> an
> > enum TidPathMethod to determine how tidquals was interpreted. At this
> > point, I think a different path type is clearer, though generation of
> both
> > types can live in tidpath.c (just as indxpath.c generates different index
> > path types).
> >
> >
> > > Now admittedly, this wouldn't give us an efficient way to execute
> > > queries with conditions like "WHERE ctid = X OR (ctid > Y AND ctid <
> Z)",
> > > but I find myself quite unable to get excited about supporting that.
> > > I see no reason for the new code to worry about any cases more complex
> > > than one or two TID inequalities at top level of the restriction list.
> > >
> >
> > I'm a bit sad to see support for multiple ranges go, though I never saw
> > such queries as ever being particularly common. (And there was always a
> > nagging feeling that tidpath.c was beginning to perform feats of boolean
> > acrobatics out of proportion to its importance. Perhaps in some distant
> > future, TID quals will become another way of supplying TIDs to a bitmap
> > heap scan, which would enable complicated boolean queries using both
> > indexes and TID scans. But that's just musing, not a proposal.)
> >
> > > In the query information given to the path generator, there is no
> existing
> > > > RestrictInfo relating to the whole expression "ctid > ? AND ctid <
> ?". I
> > > > am still learning about RestrictInfos, but my understanding is it
> doesn't
> > > > make sense to have a RestrictInfo for an AND clause, anyway; you're
> > > > supposed to have them for the sub-expressions of it.
> > >
> > > FWIW, the actual data structure for cases like that is that there's
> > > a RestrictInfo for the whole clause ctid = X OR (ctid > Y AND ctid <
> Z),
> > > and if you look into its "orclause" field, you will find RestrictInfos
> > > attached to the primitive clauses ctid = X, ctid > Y, ctid < Z. (The
> > > old code in tidpath.c didn't know that, because it'd never been
> rewritten
> > > since RestrictInfos were invented.) However, I think this new code
> should
> > > not worry about OR cases at all, but just pull out top-level TID
> > > comparison clauses.
> > >
> >
> > Thanks for the explanation.
> >
> > > And it doesn't seem a good idea to try to create new RestrictInfos in
> the
> > > > path generation just to pass the tidquals back to plan creation.
> > >
> > > No, you should avoid that. There are places that assume there's only
> > > one RestrictInfo for any given original clause (or sub-clause).
>
>
> The commitfest has ended, and you've not updated the patch to address
> the feedback yet. Are you planning to do so soon? Otherwise I think we
> ought to mark the patch as returned with feedback?
>
> Greetings,
>
> Andres Freund
>