Re: Index Skip Scan (new UniqueKeys)

On Sat, Jan 22, 2022 at 1:32 PM Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:

> > On Fri, May 21, 2021 at 05:31:38PM +0200, Dmitry Dolgov wrote:
> > Hi,
> >
> > Here is another take on the patch with a couple of changes:
> >
> > * I've removed for now UniqueKeys parts. The interaction of skip scan &
> > unique keys patch was actually not that big, so the main difference is
> > that now the structure itself went away, a list of unique expressions
> > is used instead. All the suggestions about how this feature should
> > look like from the planning perspective are still there. On the one
> > hand it will allow to develop both patches independently and avoid
> > confusion for reviewers, on the other UniqueKeys could be easily
> > incorporated back when needed.
> >
> > * Support for skipping in case of moving backward on demand (scroll
> > cursor) is moved into a separate patch. This is implemented via
> > returning false from IndexSupportsBackwardScan in case if it's a skip
> > scan node, which in turn adds Materialize node on top when needed. The
> > name SupportsBackwardScan was a bit confusing for me, but it seems
> > it's only being used with a cursorOptions check for CURSOR_OPT_SCROLL.
> > Eventually those cases when BackwardScanDirection is used are still
> > handled by amskip. This change didn't affect the test coverage, all
> > the test cases supported in previous patch versions are still there.
> >
> > About Materialize node, I guess it could be less performant than a
> > "native" support, but it simplifies the implementation significantly
> > to the point that most parts, which were causing questions before, are
> > now located in the isolated patch. My idea here is to concentrate
> > efforts on the first three patches in this series, and consider the
> > rest of them as an experiment field.
> >
> > * IndexScan support was also relocated into a separate patch, the first
> > three patches are now only about IndexOnlyScan.
> >
> > * Last bits of reviews were incorporated and rebased.
>
> While the patch is still waiting for a review, I was motivated by the
> thread [1] to think about it from the interface point of view. Consider
> an index skip scan being just like a normal index scan with a set of
> underspecified leading search keys. It makes sense to have the same
> structure "begin scan" -> "get the next tuple" -> "end scan" (now I'm
> not sure if amskip is a good name to represent that, don't have anything
> better yet). But the "underspecified" part is currently indeed
> interpreted in a limited way -- as "missing" keys -- and is expressed
> only via the prefix size. Another option would be e.g. leading keys
> constrained by a range of values, so generally speaking it makes sense
> to extend amount of the information provided for skipping.
>
> As a naive approach I've added a new patch into the series, containing
> the extra data structure (ScanLooseKeys, doesn't have much meaning yet
> except somehow representing keys for skipping) used for index skip scan.
> Any thoughts about it?
>
> Besides that the new patch version contains some cleaning up and
> addresses commentaries around leaf page pinning from [1]. The idea
> behind the series structure is still the same: the first three patches
> contains the essence of the implementation (hoping to help concentrate
> review), the rest are more "experimental".
>
> [1]:
> https://www.postgresql.org/message-id/flat/CAH2-WzmUscvoxVkokHxP=uPTDjSi0tJkFpUPD-CeA35dvn-CMw(at)mail(dot)gmail(dot)com

Hi,

+ /* If same EC already is already in the list, then not unique */

The word already is duplicated.

+ * make_pathkeys_for_uniquekeyclauses

The func name in the comment is different from the actual func name.

+ * Portions Copyright (c) 2020, PostgreSQL Global Development Group

The year should be 2022 :-)

make_pathkeys_for_uniquekeys() is called by build_uniquekeys(). Should
make_pathkeys_for_uniquekeys() be moved into uniquekeys.c ?

+query_has_uniquekeys_for(PlannerInfo *root, List *path_uniquekeys,
+ bool allow_multinulls)

It seems allow_multinulls is not checked in the func. Can the parameter be
removed ?

+ Path *newpath;
+
+ newpath = (Path *) create_projection_path(root, rel, subpath,
+ scanjoin_target);

You can remove variable newpath and assign to lfirst(lc) directly.

+add_path(RelOptInfo *parent_rel, Path *new_path)
+add_unique_path(RelOptInfo *parent_rel, Path *new_path)

It seems the above two func's can be combined into one func which
takes parent_rel->pathlist / parent_rel->unique_pathlist as third parameter.

Cheers