Re: Index Skip Scan

> On Fri, Mar 15, 2019 at 4:55 AM Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> I have some comments on the latest v11 patch.

Thank you!

> L619:
> > +    indexstate->ioss_NumDistinctKeys = node->distinctPrefix;
>
> The number of distinct prefix keys has various names in this
> patch.  They should be unified as far as possible.

Good point, I've renamed everything to skipPrefixSize, it seems for me that
this name should be self explanatory enough.

> L:728
> > +                    root->distinct_pathkeys > 0)
>
> It is not an integer, but a list.

Thanks for noticing, fixed (via compare with NIL, since we just need to know if
this list is empty or not).

> L730:
> > +                    Path *subpath = (Path *)
> > +                        create_skipscan_unique_path(root,
>
> The name "subpath" here is not a subpath, but it can be removed
> by directly calling create_skipscan_unique_path in add_path.
>
>
> L:758
> > +create_skipscan_unique_path(PlannerInfo *root,
> > +                            RelOptInfo *rel,
> > +                            Path *subpath,
> > +                            int numCols,
>
> The "subpath" is not a subpath. How about basepath, or orgpath?
> The "numCols" doesn't makes clear sense. unique_prefix_keys?

I agree, suggested names sound good.

> L764:
> > +    IndexPath *pathnode = makeNode(IndexPath);
> > +
> > +    Assert(IsA(subpath, IndexPath));
> > +
> > +    /* We don't want to modify subpath, so make a copy. */
> > +    memcpy(pathnode, subpath, sizeof(IndexPath));
>
> Why don't you just use copyObject()?

Maybe I'm missing something, but I don't see that copyObject works with path
nodes, does it? I've tried it with subpath directly and got `unrecognized node
type`.

> L773:
> > +    Assert(numCols > 0);
>
> Maybe Assert(numCols > 0 && numCols <= list_length(path->pathkeys)); ?

Yeah, makes sense.

> L586:
> > +     * Check if we need to skip to the next key prefix, because we've been
> > +     * asked to implement DISTINCT.
> > +     */
> > +    if (node->ioss_NumDistinctKeys > 0 && node->ioss_FirstTupleEmitted)
> > +    {
> > +        if (!index_skip(scandesc, direction, node->ioss_NumDistinctKeys))
> > +        {
> > +            /* Reached end of index. At this point currPos is invalidated,
>
> I thought a while on this bit. It seems that the lower layer must
> know whether it has emitted the first tuple. So I think that this
> code can be reduced as the follows.
>
> >   if (node->ioss_NumDistinctKeys &&
> >       !index_skip(scandesc, direction, node->ioss_NumDistinctKeys))
> >      return ExecClearTupler(slot);
>
> Then, the index_skip returns true with doing nothing if the
> scandesc is in the initial state. (Of course other index AMs can
> do something in the first call.)  ioss_FirstTupleEmitted and the
> comment can be removed.

I'm not sure then, how to figure out when scandesc is in the initial state from
the inside index_skip without passing the node as an argument? E.g. in the
case, describe in the commentary, when we do fetch forward/fetch backward/fetch
forward again.

> L1032:
> > + Index Only Scan using tenk1_four on public.tenk1
> > +   Output: four
> > +   Scan mode: Skip scan
>
> The "Scan mode" has only one value and it is shown only for
> "Index Only Scan" case. It seems to me that "Index Skip Scan"
> implies Index Only Scan. How about just "Index Skip Scan"?

Do you mean, show "Index Only Scan", and then "Index Skip Scan" in details,
instead of "Scan mode", right?