> On May 10, 2024, at 11:42 AM, Pavel Borisov <pashkin.e...@gmail.com> wrote:
>
> 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.
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 it would have been prior to this patch. Now, that'd
be irrelevant if nobody goes on to consult that value, but just 44 lines
further down in bt_check_level_from_leftmost() state->target is clearly used.
So the behavior at that point is changing between 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 care for the variable initialization
because it doesn't name the fields. If anybody refactored the struct they
might not notice that the need to reorder this initialization, and depending on
various factors including compiler flags they might not get an error.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
