> On Fri, Mar 15, 2019 at 4:55 AM Kyotaro HORIGUCHI > <horiguchi.kyot...@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?
v12-0001-Index-skip-scan.patch
Description: Binary data
