Hi, Alexander! On Mon, 25 Dec 2023 at 02:51, Alexander Korotkov <aekorot...@gmail.com> wrote:
> On Tue, Dec 12, 2023 at 3:22 PM Alexander Korotkov <aekorot...@gmail.com> > wrote: > > > > On Mon, Dec 11, 2023 at 6:16 PM Peter Geoghegan <p...@bowt.ie> wrote: > > > Will you be in Prague this week? If not this might have to wait. > > > > Sorry, I wouldn't be in Prague this week. Due to my current > > immigration status, I can't travel. > > I wish you to have a lovely time in Prague. I'm OK to wait, review > > once you can. I will probably provide a more polished version > > meanwhile. > > Please find the revised patchset attached. It comes with revised > comments and commit messages. Besides bug fixing the second patch > makes optimization easier to understand. Now the flag used for > skipping checks of same direction required keys is named > continuescanPrechecked and means exactly that *continuescan flag is > known to be true for the last item on the page. > > Any objections to pushing these two patches? > I've reviewed both patches: 0001 - is a pure refactoring replacing argument transfer from via struct member to transfer explicitly as a function argument. It's justified by the fact firstPage is localized only to several places. The patch looks simple and good enough. 0002: continuescanPrechecked is semantically much better than previous requiredMatchedByPrecheck which confused me earlier. Thanks! >From the new comments, it looks a little bit hard to understand who does what. Semantics "if caller told" in comments looks more clear to me. Could you especially give attention to the comments: "If they wouldn't be matched, then the *continuescan flag would be set for the current item and the last item on the page accordingly." "If the key is required for the opposite direction scan, we need to know there was already at least one matching item on the page. For those keys." > Prechecking the value of the continuescan flag for the last item on the >+ * page (according to the scan direction). Maybe, in this case, it would be more clear like: "...(for backwards scan it will be the first item on a page)" Otherwise the patch 0002 looks like a good fix for the bug to be pushed. Kind regards, Pavel Borisov
