On Sat, Jan 22, 2022 at 1:32 PM Dmitry Dolgov <9erthali...@gmail.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-...@mail.gmail.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

Reply via email to