Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PS-9121 Innodb fails to update spatial index #5319

Merged

Conversation

VarunNagaraju
Copy link
Contributor

https://perconadev.atlassian.net/browse/PS-9121

Problem

Errors about InnoDB unable "to purge non-delete-marked record in index" occurred in error log when geometry column with a spatial index on it was updated in a such a way that MBRs for old value and new value were close enough (for example, when difference was smaller than 1.0e-16 for values below 1).
Moreover server might abort due to assertion failure if later the updated row was deleted and then its old version was re-inserted again.

Analysis

The problem occurs due to combination of several factors:

  • MySQL uses double floating-point type to store coordinates in Geometry values and do related calculations.
  • Update code in InnoDB tries to avoid updating spatial index value if MBR of new value doesn't differ from the old one. To compare MBRs for old and new values it uses mbr_equal_cmp() function, which nowadays uses Boost GIS to do this. The latter uses Boost math::equals() method for this comparison which does approximate epsilon comparison for doubles.
  • Other code in InnoDB, specifically purge code uses exact comparison of MBRs for lookups in spatial index.

As result of the above discrepancy we get problem with purge described above (purge does not associate old MBR in spatial index with new row version/value and tries to delete along with old row version) and later assertion failures.

Note that before the above optimization to update code was introduced, the method mbr_equal_cmp() was doing exact MBR comparison, so the problem didn't exist. This has been changed when support for geographic coordinate systems were added.

Solution

This problem is solved by providing another MBR comparison method which does exact comparison and using it in the Update code (and thus effectively revert to old, pre-bug behavior as result).

@VarunNagaraju
Copy link
Contributor Author

@VarunNagaraju VarunNagaraju requested a review from dlenev June 17, 2024 10:27
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

///
/// @retval true The two MBRs are equal.
/// @retval false The two MBRs aren't equal.
bool mbr_equal_precise_cmp(rtr_mbr_t *a, rtr_mbr_t *b);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-length ⚠️
parameter name a is too short, expected at least 2 characters

///
/// @retval true The two MBRs are equal.
/// @retval false The two MBRs aren't equal.
bool mbr_equal_precise_cmp(rtr_mbr_t *a, rtr_mbr_t *b);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-length ⚠️
parameter name b is too short, expected at least 2 characters

@@ -143,6 +143,14 @@ bool mbr_equal_cmp(const dd::Spatial_reference_system *srs, rtr_mbr_t *a,
return result;
}

bool mbr_equal_precise_cmp(rtr_mbr_t *a, rtr_mbr_t *b) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-length ⚠️
parameter name a is too short, expected at least 2 characters

@@ -143,6 +143,14 @@
return result;
}

bool mbr_equal_precise_cmp(rtr_mbr_t *a, rtr_mbr_t *b) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-length ⚠️
parameter name b is too short, expected at least 2 characters

Copy link
Contributor

@dlenev dlenev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is OK to push the patch after fixing code formatting issue detected by clang-format.

@VarunNagaraju VarunNagaraju force-pushed the PS-9121-8.0.37-29 branch 2 times, most recently from bb9a0af to f78cf53 Compare June 17, 2024 20:13
https://perconadev.atlassian.net/browse/PS-9121

Problem
=======
Errors about InnoDB unable "to purge non-delete-marked record in index"
occurred in error log when geometry column with a spatial index on it
was updated in a such a way that MBRs for old value and new value were
close enough (for example, when difference was smaller than 1.0e-16
for values below 1).
Moreover server might abort due to assertion failure if later the
updated row was deleted and then its old version was re-inserted again.

Analysis
========
The problem occurs due to combination of several factors:

* MySQL uses double floating-point type to store coordinates in
  Geometry values and do related calculations.
* Update code in InnoDB tries to avoid updating spatial index value
  if MBR of new value doesn't differ from the old one. To compare
  MBRs for old and new values it uses mbr_equal_cmp() function,
  which nowadays uses Boost GIS to do this.
  The latter uses Boost math::equals() method for this comparison
  which does approximate epsilon comparison for doubles.
* Other code in InnoDB, specifically purge code uses exact comparison
  of MBRs for lookups in spatial index.

As result of the above discrepancy we get problem with purge described
above (purge does not associate old MBR in spatial index with new
row version/value and tries to delete along with old row version)
and later assertion failures.

Note that before the above optimization to update code was introduced,
the method mbr_equal_cmp() was doing exact MBR comparison, so the
problem didn't exist. This has been changed when support for geographic
coordinate systems were added.

Solution
========
This problem is solved by providing another MBR comparison method
which does exact comparison and using it in the Update code
(and thus effectively revert to old, pre-bug behavior as result).
@VarunNagaraju VarunNagaraju merged commit 2d41d23 into percona:release-8.0.37-29 Jun 18, 2024
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants