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