Skip to content

MDEV-16417 MDEV-20865 Prerequisites and cleanups - #5586

Open
midenok wants to merge 8 commits into
mainfrom
bb-main-midenok-MDEV-16417-prereq
Open

MDEV-16417 MDEV-20865 Prerequisites and cleanups#5586
midenok wants to merge 8 commits into
mainfrom
bb-main-midenok-MDEV-16417-prereq

Conversation

@midenok

@midenok midenok commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Preparatory refactorings and cleanups that precede the main "Store foreign
key metadata in TABLE_SHARE" work (MDEV-16417 / MDEV-20865). On their own they
introduce no user-visible feature change — they reshape the FRM/data-dictionary
code, prelocking, tests and build tooling so the FK-in-TABLE_SHARE commits apply
cleanly.

FRM / data dictionary

  • extra2 structures moved to datadict.h — relocate the extra2 helper types
    next to the new/refactored datadict routines (unireg.cc stays for the legacy
    FRM code); they feed the upcoming Extra2_info.
  • build_frm_image() readability cleanup — clearer names and named constants.
  • extra2_write_len() fix — always write a correct 2-byte length (the old code
    skipped a leading zero); tighten the related DBUG_ASSERT.
  • extra2_fields refactored to Extra2_info classread_extra2() becomes
    Extra2_info::read() with size-consistency assertions; Extra2_info::write()
    is used by later MDEV-20865 work.

Prelocking

  • Prelocking_strategy refactoring — move has_prelocking_list and
    extend_table_list() into the base Prelocking_strategy, initialize
    has_prelocking_list in reset() (subclasses chain to base), and make
    maybe_need_prelocking() virtual so it can vary per strategy.

Tests & system scripts

  • Fix tests and system scripts to foreign key requirements — since storing FK
    metadata in TABLE_SHARE makes opening a foreign table preopen its referenced
    tables, referenced tables must be created before (and dropped after) the foreign
    ones. Reorder CREATE/DROP in the help-table scripts
    (mariadb_system_tables{,_fix}.sql) and several tests; some also index a
    referenced column or add a missing referenced table.

Build / tooling

  • Deprecated warning fix — emit the legacy-mysqld-symlink and removed-option
    compatibility warnings in release builds only, so they don't pollute debug/test
    output.
  • gcov fix — add -fprofile-update=atomic so parallel/forked test runs don't
    corrupt .gcda coverage data.

@midenok
midenok requested a balanced review from Copilot August 24, 2026 09:45
@CLAassistant

CLAassistant commented Aug 24, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Refactors FRM metadata and prelocking infrastructure in preparation for foreign-key metadata storage.

Changes:

  • Centralizes and refactors extra2 FRM serialization.
  • Updates prelocking state handling and FK-dependent tests/scripts.
  • Limits compatibility warnings and improves parallel gcov reliability.

Reviewed changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
sql/unireg.h Removes relocated FRM definitions.
sql/unireg.cc Cleans up FRM construction and extra2 writing.
sql/table.h Removes relocated visibility constant.
sql/table.cc Uses Extra2_info for FRM parsing.
sql/sql_update.cc Chains multi-update prelocking reset.
sql/sql_delete.cc Chains multi-delete prelocking reset.
sql/sql_base.h Moves prelocking state into the base strategy.
sql/sql_base.cc Refactors prelocking extension calls.
sql/mysqld.cc Restricts removed-option warnings.
sql/datadict.h Defines extra2 structures and helpers.
sql/datadict.cc Implements Extra2_info reading and writing.
scripts/mariadb_system_tables.sql Reorders help-table creation.
scripts/mariadb_system_tables_fix.sql Reorders help-table alteration.
mysys/my_init.c Restricts deprecated-name warnings.
mysql-test/suite/innodb/t/instant_alter_index_rename.test Adds a valid referenced table.
mysql-test/suite/innodb/r/instant_alter_index_rename.result Updates expected output.
mysql-test/main/union.test Creates and drops FK references correctly.
mysql-test/main/union.result Updates expected output.
mysql-test/main/system_mysql_db_fix50568.test Reorders FK-related help tables.
mysql-test/main/system_mysql_db_fix50568.result Updates expected output.
mysql-test/main/system_mysql_db_fix50117.test Reorders FK-related help tables.
mysql-test/main/system_mysql_db_fix50117.result Updates expected output.
mysql-test/main/system_mysql_db_fix50030.test Reorders FK-related help tables.
mysql-test/main/system_mysql_db_fix50030.result Updates expected output.
mysql-test/main/system_mysql_db_fix40123.test Reorders FK-related help tables.
mysql-test/main/system_mysql_db_fix40123.result Updates expected output.
mysql-test/main/opt_context_store_ddls.test Corrects FK drop order.
mysql-test/main/opt_context_store_ddls.result Includes FK metadata and new order.
mysql-test/main/insert_notembedded.test Corrects FK target and lifecycle order.
mysql-test/main/insert_notembedded.result Updates expected output.
mysql-test/main/fetch_first.test Adds referenced index and fixes drop order.
mysql-test/main/fetch_first.result Updates expected output.
CMakeLists.txt Enables atomic gcov profile updates.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread sql/datadict.cc Outdated
Comment thread sql/sql_base.cc

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.

Comment thread mysql-test/main/opt_context_store_ddls.result Outdated
Comment thread sql/datadict.cc Outdated
Comment thread mysys/my_init.c Outdated
@midenok
midenok force-pushed the bb-main-midenok-MDEV-16417-prereq branch from 7f90efe to 48292c9 Compare August 24, 2026 12:55
@midenok
midenok requested a balanced review from Copilot August 24, 2026 12:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

sql/datadict.cc:298

  • Remove the extra “in” so this TODO reads grammatically.
  TODO: now this is used in by MDEV-20865 but this can be also used by

Comment thread sql/datadict.h Outdated
Comment thread sql/datadict.cc
@midenok
midenok force-pushed the bb-main-midenok-MDEV-16417-prereq branch from 5fdfb2a to b58d55b Compare August 24, 2026 14:08
@midenok
midenok requested a balanced review from Copilot August 24, 2026 14:08

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.

Comment thread sql/datadict.cc Outdated
Comment thread sql/unireg.cc
Two compatibility warnings are meant for end users but only add noise to
debug/test builds: mysqld is commonly started through a legacy "mysqld"
symlink, and test .cnf files still reference long-removed options. Emit
them in release builds only.

- mysys/my_init.c: the deprecated-invocation-name warning
  (EE_NAME_DEPRECATED), shown when the program is run via a non-"mariadb"
  symlink to a "mariadb*" binary -- now compiled under #ifdef NDEBUG.

- sql/mysqld.cc: the "'<opt>' was removed ... exists only for
  compatibility" warning for OPT_REMOVED_OPTION -- now under #ifdef DBUG_OFF.
Fix coverage data corruption by enabling atomic profile updates

When compiling with coverage instrumentation, concurrent or forked test executions
can cause corruption of coverage data files (.gcda), resulting in negative hit counts
and invalid coverage reports.

Adding the compiler flag `-fprofile-update=atomic` ensures that updates to
coverage counters are performed atomically, preventing race conditions during
profile data writes.

This change eliminates warnings about unexpected negative hit counts and
improves the accuracy and reliability of coverage measurements in parallel
or multithreaded test environments.
- Moved has_prelocking_list into Prelocking_strategy (removed it from
  Multiupdate_prelocking_strategy). It is now initialized in
  Prelocking_strategy::reset(), so reset() overrides
  (Multiupdate_prelocking_strategy, Multidelete_prelocking_strategy)
  chain to the base implementation to set it.
- Moved extend_table_list() into Prelocking_strategy;
- Made maybe_need_prelocking() virtual method.

Now maybe_need_prelocking() can be different per prelocking strategy.
datadict.h is appropriate place for such types. These data types are
used by Extra2_info, which is added to datadict.h later.

unireg.cc is for older FRM routines. datadict.cc accepts new and
refactored routines.
@midenok
midenok force-pushed the bb-main-midenok-MDEV-16417-prereq branch from 42e3a6d to e796b86 Compare August 24, 2026 14:52
@midenok
midenok requested a balanced review from Copilot August 24, 2026 14:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.

Comment thread sql/datadict.h
Cleanups:

- build_frm_image(): clearer names and named constants
- engine_table_options_frm_image(): mark in/out argument directions

Improvements:

- build_frm_image(): guard against overflow of the 2-byte key-info offset
- extra2_read_len(): validate the length against the remaining bytes

build_frm_image(): the key-information offset is stored in a 2-byte field,
yet only each individual extra2 value was length-checked. A definition with
enough extra2 content could push frm.length past 0xFFFF, silently truncating
the offset in int2store() and yielding an unreadable frm. Reject such a
definition before the store.

extra2_read_len(): the short-form (1-byte) length was returned without
verifying that its payload fits before the segment end, unlike the long-form
encoding. A truncated or corrupt frm could produce an out-of-range span that
callers consume before any later boundary check. Both encodings are now
checked against the remaining bytes.
Current implementation of extra2_write_len() does not guarantee of
writing correct 2-byte values as it skips writing zero at the
beginning.

Refined DBUG_ASSERT() to more truthful limit.
read_extra2() is now Extra2_info::read()
Additional assertions for checking size consistency.

Extra2_info::write() is used by further MDEV-20865 development.
Storing foreign key metadata in TABLE_SHARE makes opening a foreign table
preopen its referenced tables, so a referenced table must be created
before the foreign one and dropped after it. Rearrange the order of
CREATE/DROP commands in tests and system scripts accordingly.

Affected: the help tables in mariadb_system_tables{,_fix}.sql (with
system_mysql_db_fix* results), fetch_first, union, insert_notembedded,
instant_alter_index_rename and opt_context_store_ddls; some also index a
referenced column or add a missing referenced table, and
opt_context_store_ddls re-records the now-shown MyISAM foreign key.
@midenok
midenok force-pushed the bb-main-midenok-MDEV-16417-prereq branch from dcb6474 to 6c125ea Compare August 24, 2026 15:16
@midenok
midenok requested a balanced review from Copilot August 24, 2026 15:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants