Skip to content
/ server Public

MDEV-38079: Crash Recovery Fails After ALTER TABLE…PAGE_COMPRESSED=1 #4586

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.

Sign up for GitHub

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

Open
mariadb-TafzeelShams wants to merge 1 commit into
base: 10.6
Choose a base branch
from
+27 −10
Open

MDEV-38079: Crash Recovery Fails After ALTER TABLE…PAGE_COMPRESSED=1 #4586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions storage/innobase/fil/fil0fil.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1066,8 +1066,8 @@ bool fil_space_t::read_page0(const byte *dpage, bool no_lsn) noexcept
return ok;
}

void fil_space_set_recv_size_and_flags(ulint id, uint32_t size, uint32_t flags)
noexcept
void fil_space_set_recv_size_and_flags(ulint id, uint32_t size,
uint32_t flags, lsn_t lsn) noexcept
{
ut_ad(id < SRV_SPACE_ID_UPPER_BOUND);
mysql_mutex_assert_owner(&recv_sys.mutex);
Expand All @@ -1079,7 +1079,7 @@ void fil_space_set_recv_size_and_flags(ulint id, uint32_t size, uint32_t flags)
if (size)
space->recv_size= size;
if (flags != FSP_FLAGS_FCRC32_MASK_MARKER)
space->flags= flags;
recv_sys.update_space_flags(space, flags, lsn);
}
mysql_mutex_unlock(&fil_system.mutex);
}
Expand Down
5 changes: 3 additions & 2 deletions storage/innobase/include/fil0fil.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1672,9 +1672,10 @@ fil_space_free(
/** Set the recovered size of a tablespace in pages.
@param id tablespace ID
@param size recovered size in pages
@param flags tablespace flags */
@param flags tablespace flags
@param lsn lsn of the redo log */
void fil_space_set_recv_size_and_flags(ulint id, uint32_t size,
uint32_t flags) noexcept;
uint32_t flags, lsn_t lsn) noexcept;
Comment on lines 1672 to +1678
Copy link
Contributor

@dr-m dr-m Jan 28, 2026

Choose a reason for hiding this comment

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

The @param lsn comment is not adding any value. Which specific LSN does it refer to? Also the size and flags could be described more accurately. Let us use spaces instead of TAB also in the comment:

/** Set the recovered size and flags of a tablespace.
@param id         tablespace ID
@param size       recovered FSP_SIZE, or a less-than-equal value if no change
@param flags      recovered FSP_SPACE_FLAGS if applicable
@param flags_lsn  start LSN of modifying flags, or 0 */

This comment would also answer the question what would happen or how we would identify the situation when a mini-transaction is modifying only one of FSP_SIZE or FSP_SPACE_FLAGS.


/*******************************************************************//**
Sets the max tablespace id counter if the given number is bigger than the
Expand Down
4 changes: 4 additions & 0 deletions storage/innobase/include/log0recv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,8 @@ struct recv_sys_t
/** set when an inconsistency with the file system contents is detected
during log scan or apply */
bool found_corrupt_fs;
/** Last applied LSN for space->flags updates (used to prevent stale updates) */
std::map<uint32_t, lsn_t> space_flags_lsn;
Comment on lines +271 to +272
Copy link
Contributor

@dr-m dr-m Jan 28, 2026

Choose a reason for hiding this comment

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

This could significantly increase the memory footprint. Do we really need an ordered map? We already store similar information in file_name_t linked from recv_spaces.

Could we slightly repurpose fil_space_t::create_lsn for storing this information? I think that the name fil_space_t::format_lsn would also cover the meaning of create_lsn, since the format is usually determined at creation.

If the information is being read before a fil_space_t object is available, then it would seem possible to add file_name_t::format_lsn for storing this.

public:
/** whether we are applying redo log records during crash recovery.
This is protected by recv_sys.mutex */
Expand Down Expand Up @@ -351,6 +353,8 @@ struct recv_sys_t
if (pages_it != pages.end() && pages_it->first.space() == space_id)
pages_it= pages.end();
}
/** Update space->flags only if lsn >= last recorded space-flags LSN */
void update_space_flags(fil_space_t *space, uint32_t flags, lsn_t lsn);
Comment on lines +356 to +357
Copy link
Contributor

@dr-m dr-m Jan 28, 2026

Choose a reason for hiding this comment

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

Which LSN? This comment should also somehow refer to fil_space_set_recv_size_and_flags().


private:
/** Attempt to initialize a page based on redo log records.
Expand Down
22 changes: 17 additions & 5 deletions storage/innobase/log/log0recv.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1093,6 +1093,17 @@ class mlog_init_t

static mlog_init_t mlog_init;

/** Update space->flags only if lsn >= last recorded space-flags LSN */
void recv_sys_t::update_space_flags(fil_space_t *space, uint32_t flags, lsn_t lsn)
{
auto it= space_flags_lsn.find(space->id);
if (it == space_flags_lsn.end() || lsn >= it->second)
{
space->flags= flags;
space_flags_lsn[space->id]= lsn;
}
Comment on lines +1099 to +1104
Copy link
Contributor

@dr-m dr-m Jan 28, 2026

Choose a reason for hiding this comment

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

Why not it->lsn= lsn;?

We are missing mysql_mutex_assert_owner(&recv_sys.mutex) here. The access to a shared data structure needs to be protected somehow.

}

/** Try to recover a tablespace that was not readable earlier
@param p iterator to the page
@param name tablespace file name
Expand Down Expand Up @@ -1334,7 +1345,7 @@ static void fil_name_process(const char *name, ulint len, uint32_t space_id,
if (f.size
|| f.flags != f.initial_flags) {
fil_space_set_recv_size_and_flags(
space->id, f.size, f.flags);
space->id, f.size, f.flags, lsn);
}
Comment on lines 1345 to 1349
Copy link
Contributor

@dr-m dr-m Jan 28, 2026

Choose a reason for hiding this comment

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

This is passing the start LSN of the mini-transaction of a FILE_ record, instead of the start LSN of the mini-transaction that actually modified FSP_SPACE_FLAGS.


f.space = space;
Expand Down Expand Up @@ -2723,7 +2734,7 @@ bool recv_sys_t::parse(lsn_t checkpoint_lsn, store_t *store, bool apply)
if (has_flags)
it->second.flags= flags;
}
fil_space_set_recv_size_and_flags(space_id, size, flags);
fil_space_set_recv_size_and_flags(space_id, size, flags, start_lsn);
}
}
last_offset+= rlen;
Expand Down Expand Up @@ -3081,9 +3092,10 @@ static buf_block_t *recv_recover_page(buf_block_t *block, mtr_t &mtr,
: fil_space_t::get(block->page.id().space())) {
switch (a) {
case log_phys_t::APPLIED_TO_FSP_HEADER:
s->flags = mach_read_from_4(
FSP_HEADER_OFFSET
+ FSP_SPACE_FLAGS + frame);
recv_sys.update_space_flags(
s, mach_read_from_4(FSP_HEADER_OFFSET +
FSP_SPACE_FLAGS + frame),
l->start_lsn);
Comment on lines 3094 to +3098
Copy link
Contributor

@dr-m dr-m Jan 28, 2026

Choose a reason for hiding this comment

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

Could this code be moved to log_phys_t::apply() and the special return value APPLIED_TO_FSP_HEADER removed? That return value does not currently distinguish changes to FSP_SPACE_FLAGS and FSP_SIZE. That is, if a mini-transaction only modifies FSP_SIZE but not FSP_SPACE_FLAGS, we would execute some unnecessary code here.

s->size_in_header = mach_read_from_4(
FSP_HEADER_OFFSET + FSP_SIZE
+ frame);
Expand Down