MDEV-39261 MariaDB crash on startup in presence of indexed virtual columns#4914
MDEV-39261 MariaDB crash on startup in presence of indexed virtual columns#4914Thirunarayanan wants to merge 1 commit into10.6from
Conversation
|
|
dr-m
left a comment
dr-m
left a comment
There was a problem hiding this comment.
Thank you, great work in reproducing the error. I found some ways to simplify this logic further. I think that we should invoke open_purge_table() for each dict_table_t::id only once per purge batch, in trx_purge_table_acquire(), provided that indexed virtual columns exist. In that way, when we are purging individual undo log records, we are guaranteed to have a TABLE* handle available.
Thirunarayanan
force-pushed
the
MDEV-39261
branch
6 times, most recently
from
60d1668 to
227427a
Compare
storage/innobase/include/trx0purge.h
Outdated
| trx_purge() after all workers complete. */ | ||
| THD* coordinator_thd= nullptr; |
There was a problem hiding this comment.
This is mixing TAB and space indentation.
I wonder if we truly need this field. Can we get the necessary information via MDL_ticket::get_ctx()?
There was a problem hiding this comment.
When innodb_purge_threads=1, the coordinator acts as both coordinator and
worker, calling srv_purge_worker_task_low() directly. In this case, it
processes purge_node_t entries and would normally call
innobase_reset_background_thd() in purge_node_t::end(), which would
prematurely close all open TABLE* objects via close_thread_tables().
This would break the batch processing since tables need to remain open
until all purge_node_t entries are processed.
Worker threads (when innodb_purge_threads > 1) have their own THD
lifecycle managed by acquire_thd()/release_thd() and must call
innobase_reset_background_thd() in purge_node_t::end() to clean up
their thread-local resources.
By comparing the current THD against coordinator_thd, purge_node_t::end()
can skip the premature cleanup for the coordinator while still allowing
workers to properly clean up their resources. The coordinator's cleanup
happens centrally in trx_purge() after all nodes complete
| if (thd != purge_sys.coordinator_thd) | ||
| innobase_reset_background_thd(thd); |
There was a problem hiding this comment.
Do we really need this call here, or could we avoid adding purge_sys.coordinator_thd and invoke this only in trx_purge_close_tables()? Note: The comment of that function incorrectly hints that it could be called by purge workers. That needs to be corrected; it is only called by the purge coordinator task.
There was a problem hiding this comment.
Yes, we need this here. mentioned the reason in the above comment.
| if (!table) | ||
| return nullptr; | ||
| /* At this point, the freshly loaded table may already have been evicted. | ||
| We must look it up again while holding a shared dict_sys.latch. We keep | ||
| trying this until the table is found in the cache or it cannot be found | ||
| in the dictionary (because the table has been dropped or rebuilt). */ |
There was a problem hiding this comment.
It is a little hard to review this change, because some code was shuffled around. Can you make sure that a minimal diff will be displayed for the function trx_purge_table_open()? I can see that we removed the comment and added a dereferencing of table after it. I will post a separate comment on the moved snippet where this comment had been omitted.
There was a problem hiding this comment.
I realized that the comment is at the end of a for (;;) loop body. So, the next iteration of the loop should look up the freshly loaded table.
| dict_sys.lock(SRW_LOCK_CALL); | ||
| table= dict_load_table_on_id(table_id, DICT_ERR_IGNORE_FK_NOKEY); | ||
| dict_sys.unlock(); | ||
| if (!table) | ||
| return nullptr; | ||
| } | ||
|
|
||
| if (!table->is_readable() || table->corrupted) |
There was a problem hiding this comment.
In this function trx_purge_table_open(), which the patch is moving elsewhere, so that all of the code will be indicated as new here, there used to be a comment after the !table check, saying that the freshly loaded table could already be evicted at that point, and therefore it is not safe to access the table without looking it up first. Now the comment is gone, and we are dereferencing table in an unsafe way.
Can dict_load_table_on_id() ever return an unreadable or corrupted table, or would it return nullptr in that case? I wonder if the condition could be relocated.
However, for the rest of the function, the same problem exists. Apparently, the subsequent code is assuming that we are holding dict_sys.freeze(). That is not the case if we had invoked dict_load_table_on_id().
There was a problem hiding this comment.
I realized that the table would be latched in the next iteration of this for (;;) loop. So, there is no correctness issue. Only the comment was being removed, making it harder to understand the logic.
…lumns Problem: ======== A single InnoDB purge worker thread can process undo logs from different tables within the same batch. But get_purge_table(), open_purge_table() incorrectly assumes that a 1:1 relationship between a purge worker thread and a table within a single batch. Based on this wrong assumtion, InnoDB attempts to reuse TABLE objects cached in thd->open_tables for virtual column computation. 1) Purge worker opens Table A and caches the TABLE pointer in thd->open_tables. 2) Same purge worker moves to Table B in the same batch, get_purge_table() retrieves the cached pointer for Table A instead of opening Table B. 3) Because innobase::open() is ignored for Table B, the virtual column template is never initialized. 4) virtual column computation for Table B aborts the server Solution: ======== The purge coordinator thread now opens both InnoDB dict_table_t* and MariaDB TABLE* handles during batch preparation in trx_purge_attach_undo_recs(). purge_node_t::tables now stores only std::pair<dict_table_t*, TABLE*>. When worker thread needs TABLE* for virtual column computation, it fetches the TABLE* from purge_node_t->tables. trx_purge_table_open(): Modified to open TABLE* using the coordinator's MDL ticket open_purge_table(): Accept and use the MDL ticket from coordinator thread purge_sys_t::coordinator_thd: To track the coordinator thread in purge subsystem purge_node_t::end(): To prevent premature table closure when the coordinator acts as a worker and skips innobase_reset_background_thd() purge_sys_t::close_and_reopen(): Properly handles retry logic by clearing all table state and reopening the table. Uses mdl_map for maintaining mdl_tickets for each table id trx_purge_close_tables(): Now accepts mdl_map parameter to release MDL tickets from the coordinator's map after closing tables trx_purge(): MDL tickets are now stored in a local mdl_map instead of purge_node_t. Closes the coordinator's TABLE* objects after all workers are completed Declared open_purge_table() and close_thread_tables() in trx0purge.cc
Thirunarayanan
force-pushed
the
MDEV-39261
branch
from
227427a to
c69ba83
Compare
3 participants
Problem:
A single InnoDB purge worker thread can process undo logs from different tables within the same batch. But get_purge_table(), open_purge_table() incorrectly assumes that a 1:1 relationship between a purge worker thread and a table within a single batch. Based on this wrong assumtion, InnoDB attempts to reuse TABLE objects cached in thd->open_tables for virtual column computation.
Solution:
get_purge_table(): Accept the specific db_name and table_name associated with the current undo log record. Compare the db_name and table_name against the existing cached TABLE objects. If it is match then return the cached table object.