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-9222 Include reordered fields when calculating mlog buffer size #5321

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

VarunNagaraju
Copy link
Contributor

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

Problem

Upstream commit e6e13a8 tries to fix an issue that doesn't log the fields that changes orders when using ALGORITHM=instant, by checking if the fields is also reordered, then adding the columns into the list. However when calculating the size of the buffer this fix doesn't take account the extra fields that may be logged, and causing the assertion on the buffer size failed eventually.

Solution

To calculate the buffer size correctly, we move the logic of finding reordered fiedls before buffer size calculation, then count the number of fields with the same logic when deciding if a field needs to be logged.

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)

storage/innobase/mtr/mtr0log.cc Outdated Show resolved Hide resolved
storage/innobase/mtr/mtr0log.cc Outdated Show resolved Hide resolved
storage/innobase/mtr/mtr0log.cc Outdated Show resolved Hide resolved
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)

storage/innobase/mtr/mtr0log.cc Outdated Show resolved Hide resolved
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)

storage/innobase/mtr/mtr0log.cc Outdated Show resolved Hide resolved
storage/innobase/mtr/mtr0log.cc Outdated Show resolved Hide resolved
storage/innobase/mtr/mtr0log.cc Show resolved Hide resolved
@VarunNagaraju
Copy link
Contributor Author

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)

storage/innobase/mtr/mtr0log.cc Show resolved Hide resolved
@VarunNagaraju VarunNagaraju force-pushed the PS-9222-8.0 branch 2 times, most recently from 7d42d75 to 63a614f Compare June 28, 2024 10:46
Copy link
Contributor

@satya-bodapati satya-bodapati left a comment

Choose a reason for hiding this comment

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

LGTM!

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

Problem
=======
When writing to the redo log, an issue of column order change not
being recorded with INSTANT DDL was fixed by creating an array
with size equal to the number of fields in the index which kept
track of whether the original position of the field was changed
or not. Later, that array would be used to make a decision on
logging the field.
But, this solution didn't take into account the fact that
there could be column prefixes because of the primary key. This
resulted in inaccurate entries being filled in the
fields_with_changed_order[] array.

Solution
========
It is fixed by using the method, get_col_phy_pos() which takes
into account the existence of column prefix instead of get_phy_pos()
while generating fields_with_changed_order[] array.
https://perconadev.atlassian.net/browse/PS-9222

Problem
=======
When writing to the redo log, an issue of column order change not
being recorded with INSTANT DDL was fixed by checking if the fields
are also reordered, then adding the columns into the list.
However when calculating the size of the buffer this fix doesn't take
account the extra fields that may be logged, and causing the assertion
on the buffer size failed eventually.

Solution
========
To calculate the buffer size correctly, we move the logic of finding
reordered fiedls before buffer size calculation, then count the number
of fields with the same logic when deciding if a field needs to be logged.
@VarunNagaraju VarunNagaraju merged commit 86bde10 into percona:release-8.0.37-29 Jun 28, 2024
8 of 28 checks passed
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)

static void log_index_get_size_needed(
const dict_index_t *index, size_t size, uint16_t n, bool is_comp,
bool is_versioned, bool is_instant,
const std::unique_ptr<bool[]> &changed_order, size_t &size_needed) {

Choose a reason for hiding this comment

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

⚠️ misc-unused-parameters ⚠️
parameter changed_order is unused

Suggested change
const std::unique_ptr<bool[]> &changed_order, size_t &size_needed) {
size_t &size_needed) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants