> On May 10, 2024, at 11:42 AM, Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>
> IMO 0003 doesn't introduce nor fixes a bug. It loads rightpage into a local variable, rather that to a
BtreeCheckStatethat can have another users of state->target afterb uniqueness check in the future, but don't have now.
Sothe original patch is correct, and the goal of this refactoring is to untie rightpage fron state structure as it's
usedonly transiently for cross-page unuque check. It's the same style as already used bt_right_page_check_scankey()
thatloads rightpage into a local variable.
Well, you can put an Assert(false) dead in the middle of the code we're discussing and all the regression tests still
pass,so I'd argue the change is getting zero test coverage.
This patch introduces a change that stores a new page into variable "rightpage" rather than overwriting
"state->target",which the old implementation most certainly did. That means that after returning from
bt_target_page_check()into the calling function bt_check_level_from_leftmost() the value in state->target is not what
itwould have been prior to this patch. Now, that'd be irrelevant if nobody goes on to consult that value, but just 44
linesfurther down in bt_check_level_from_leftmost() state->target is clearly used. So the behavior at that point is
changingbetween the old and new versions of the code, and I think I'm within reason to ask if it was wrong before the
patch,wrong after the patch, or something else? Is this a bug being introduced, being fixed, or ... ?
Having a regression test that actually touches this code would go a fair way towards helping the analysis.
> For 0002 I doubt I understand your actual positiob. Could you explain what it violates or doesn't violate?
v2-0002 is does not violate the post feature freeze restriction on new features so far as I can tell, but I just don't
carefor the variable initialization because it doesn't name the fields. If anybody refactored the struct they might
notnotice that the need to reorder this initialization, and depending on various factors including compiler flags they
mightnot get an error.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
