On Fri, 10 May 2024, 22:42 Pavel Borisov, <pashkin.e...@gmail.com> wrote:
> Hi, Mark! > > > On Fri, 10 May 2024, 21:35 Mark Dilger, <mark.dil...@enterprisedb.com> > wrote: > >> >> >> > On May 10, 2024, at 5:10 AM, Pavel Borisov <pashkin.e...@gmail.com> >> wrote: >> > >> > Hi, Alexander! >> > >> > On Fri, 10 May 2024 at 12:39, Alexander Korotkov <aekorot...@gmail.com> >> wrote: >> > On Fri, May 10, 2024 at 3:43 AM Tom Lane <t...@sss.pgh.pa.us> wrote: >> > > Alexander Korotkov <aekorot...@gmail.com> writes: >> > > > The revised patchset is attached. I applied cosmetical changes. >> I'm >> > > > going to push it if no objections. >> > > >> > > Is this really suitable material to be pushing post-feature-freeze? >> > > It doesn't look like it's fixing any new-in-v17 issues. >> > >> > These are code improvements to the 5ae2087202, which answer critics in >> > the thread. 0001 comprises an optimization, but it's rather small and >> > simple. 0002 and 0003 contain refactoring. 0004 contains better >> > error reporting. For me this looks like pretty similar to what others >> > commit post-FF, isn't it? >> > I've re-checked patches v2. Differences from v1 are in improving >> naming/pgindent's/commit messages. >> > In 0002 order of variables in struct BtreeLastVisibleEntry changed. >> This doesn't change code behavior. >> > >> > Patch v2-0003 doesn't contain credits and a discussion link. All other >> patches do. >> > >> > Overall, patches contain small performance optimization (0001), code >> refactoring and error reporting changes. IMO they could be pushed post-FF. >> >> v2-0001's commit message itself says, "This commit implements skipping >> keys". I take no position on the correctness or value of the improvement, >> but it seems out of scope post feature freeze. The patch seems to postpone >> uniqueness checking until later in the scan than what the prior version >> did, and that kind of change could require more analysis than we have time >> for at this point in the release cycle. >> >> >> v2-0002 does appear to just be refactoring. I don't care for a small >> portion of that patch, but I doubt it violates the post feature freeze >> rules. In particular: >> >> + BtreeLastVisibleEntry lVis = {InvalidBlockNumber, >> InvalidOffsetNumber, -1, NULL}; >> >> >> v2-0003 may be an improvement in some way, but it compounds some >> preexisting confusion also. There is already a member of the >> BtreeCheckState called "target" and a memory context in that struct called >> "targetcontext". That context is used to allocate pages "state->target", >> "rightpage", "child" and "page", but not "metapage". Perhaps >> "targetcontext" is a poor choice of name? "notmetacontext" is a terrible >> name, but closer to describing the purpose of the memory context. Care to >> propose something sensible? >> >> Prior to applying v2-0003, the rightpage was stored in state->target, and >> continued to be in state->target later when entering >> >> /* >> * * Downlink check * >> * >> * Additional check of child items iff this is an internal page >> and >> * caller holds a ShareLock. This happens for every downlink >> (item) >> * in target excluding the negative-infinity downlink (again, >> this is >> * because it has no useful value to compare). >> */ >> if (!P_ISLEAF(topaque) && state->readonly) >> bt_child_check(state, skey, offset); >> >> and thereafter. Now, the rightpage of state->target is created, checked, >> and free'd, and then the old state->target gets processed in the downlink >> check and thereafter. This is either introducing a bug, or fixing one, but >> the commit message is totally ambiguous about whether this is a bugfix or a >> code cleanup or something else? I think this kind of patch should have a >> super clear commit message about what it thinks it is doing. >> >> >> v2-0004 guards against a real threat, and is reasonable post feature >> freeze >> >> >> — >> Mark Dilger >> EnterpriseDB: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > IMO 0003 doesn't introduce nor fixes a bug. It loads rightpage into a > local variable, rather that to a BtreeCheckState that can have another > users of state->target afterb uniqueness check in the future, but don't > have now. So the original patch is correct, and the goal of this > refactoring is to untie rightpage fron state structure as it's used only > transiently for cross-page unuque check. It's the same style as already > used bt_right_page_check_scankey() that loads rightpage into a local > variable. > > For 0002 I doubt I understand your actual positiob. Could you explain what > it violates or doesn't violate? > Please forgive many typos in the previous message, I wrote from phone. Pavel. >