Re: Index Skip Scan

Hello.

At Thu, 14 Mar 2019 14:32:49 +0100, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote in <CA+q6zcUSuFBhGVFZN_AVSxRbt5wr_4_YEYwv8PcQB=m6J6Zpvg(at)mail(dot)gmail(dot)com>
> > On Tue, Mar 5, 2019 at 4:05 PM Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
> >
> > Although there are still some rough edges, e.g. going forth, back and forth
> > again leads to a sutiation, when `_bt_first` is not applied anymore and the
> > first element is wrongly skipped. I'll try to fix it with the next version of
> > patch.
>
> It turns out that `_bt_skip` was unnecessary applied every time when scan was
> restarted from the beginning. Here is the fixed version of patch.

I have some comments on the latest v11 patch.

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.

L:728
> + root->distinct_pathkeys > 0)

It is not an integer, but a list.

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?

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()?

L773:
> + Assert(numCols > 0);

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

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.

By the way this patch seems to still be forgetting about the
explicit rescan case but just doing this makes such consideration
not required.

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"?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center