PS-5874 : Upgrade to 8.0.16-7 breaks on innodb_dynamic_metadata with encrypted #3390
Conversation
satya-bodapati
changed the title
satya-bodapati
changed the title
| @@ -165,6 +165,7 @@ bool update_system_tables(THD *thd) { | |||
| std::unique_ptr<Properties> table_def_properties( | |||
| Properties::parse_properties(def)); | |||
| table_def->set_actual_table_definition(*table_def_properties); | |||
| table_def->set_actual_encrypted(); | |||
There was a problem hiding this comment.
Why is this done unconditionally?
Side note: since robert is on vacation, I am debugging this to find the answer
There was a problem hiding this comment.
Few more questions:
Why is the property change done in-memory only and not registerd in DD properties?
Why do we do the "CREATE TABLE mysql.innodb_dynamic_metadata" (the actual table vs target table) when the table already existed in SE. Does the mysql_execute_command() or mysql_create_table() really try to create table in SE?
There was a problem hiding this comment.
Answers from debugging
- (on the unconditionally question) : No answer yet
- property change (adding encryption='y') to DD properties is not just in-memory change. It is written to disk as well ( dd::update_properties()) is called
- the CREATE TABLE innodb_dynamic_metadata doesn't result in table creation in SE. (we already have the table in SE). This step is only to create TABLE*/TABLE_SHARE* for core tables.
| @@ -1352,7 +1353,9 @@ bool initialize_dd_properties(THD *thd) { | |||
| */ | |||
| if (!bootstrap::DD_bootstrap_ctx::instance().is_initialize() && | |||
| bootstrap::DD_bootstrap_ctx::instance().is_dd_upgrade() && | |||
| update_system_tables(thd)) { | |||
| update_system_tables(thd) && | |||
| update_properties(thd, nullptr, nullptr, | |||
There was a problem hiding this comment.
By using this update_properties() the DD_properties of the dictionary tables are stored on disk? Next restart shouldn't depend on in-memory flag?
Also why do we need this unconditionally?
There was a problem hiding this comment.
Am guessing that restart would keep using this in-memory flag and add double "encryption=y" again? Have to debug and find out
There was a problem hiding this comment.
DD properties after update_properties is like this:
tbl_props->raw_string()
$12 = "col0=table_id\=2\;;col1=table_id\=2\;;col2=table_id\=2\;;col3=table_id\=2\;;col4=table_id\=2\;;data=;def=fields\=elem0\\\=def\\\\\\\=metadata BLOB NOT NULL\\\\\\\;lbl\
\\\\\=metadata\\\\\\\;pos\\\\\\\=2\\\\\\\;\\\;elem1\\\=def\\\\\\\=table_id BIGINT UNSIGNED NOT NULL\\\\\\\;lbl\\\\\\\=table_id\\\\\\\;pos\\\
\\\\=0\\\\\\\;\\\;elem2\\\=def\\\\\\\=version BIGINT UNSIGNED NOT NULL\\\\\\\;lbl\\\\\\\=version\\\\\\\;pos\\\\\\\=1\\\\\\\;\\\;\;foreign_key
s\=\;indexes\=elem0\\\=def\\\\\\\=PRIMARY KEY (table_id)\\\\\\\;lbl\\\\\\\=index_pk\\\\\\\;pos\\\\\\\=0\\\\\\\;\\\;\;name\=innodb_dynamic_metadata\;op
tions\=elem0\\\=def\\\\\\\=DEFAULT CHARSET", '\' <repeats 15 times>, "=utf8\\\\\\\;lbl\\\\\\\=CHARSET\\\\\\\;pos\\\\\\\=1\\\\\\\;\\\;elem1\\\=def\\\
\\\\=COLLATE", '\' <repeats 15 times>, "=utf8_bin\\\\\\\;lbl\\\\\\\=COLLATION\\\\\\\;pos\\\\\\\=2\\\\\\\;\\\;elem2\\\=def\\\\\\\=ENCRYPTION", '\' <rep
eats 15 times>, "='Y'\\\\\\\;lbl\\\\\\\=ENCRYPTION\\\\\\\;pos\\\\\\\=6\\\\\\\;\\\;elem3\\\=def\\\\\\\=ENGINE", '\' <repeats 15 times>, "=INNODB\\\\\\
\;lbl\\\\\\\=ENGINE\\\\\\\;pos\\\\\\\=0\\\\\\\;\\\;elem4\\\=def\\\\\\\=ROW_FORMAT", '\' <repeats 15 times>, "=DYNAMIC\\\\\\\;lbl\\\\\\\=ROW_FORMAT
\\\\\\;pos\\\\\\\=3\\\\\\\;\\\;elem5\\\=def\\\\\\\=STATS_PERSISTENT", '\' <repeats 15 times>, "=0\\\\\\\;lbl\\\\\\\=STATS_PERSISTENT\\\\\\\;pos\\
\\\\=4\\\\\\\;\\\;elem6\\\=def\\\\\\\=TABLESPACE", '\' <repeats 15 times>, "=mysql\\\\\\\;lbl\\\\\\\=TABLESPACE\\\\\\\;pos\\\\\\\=5\\\\\\\;\\
\;\;;id=2;idx0=id\=2\;root\=5\;space_id\=4294967294\;table_id\=2\;trx_id\=0\;;space_id=1;"
We can see that ENCRYPTION="Y". So this goes to disk. Next I have to check if next restart also adds the same
| @@ -58,8 +60,19 @@ Object_table_impl::Object_table_impl() | |||
| } | |||
|
|
|||
| void Object_table_impl::set_encrypted() { | |||
There was a problem hiding this comment.
I observed that this is used only for DD properties table and for the rest the constructor does this.
| @@ -35,7 +35,9 @@ Object_table_impl::Object_table_impl() | |||
| m_target_def(), | |||
| m_actual_present(false), | |||
| m_actual_def(), | |||
| m_hidden(true) { | |||
| m_hidden(true), | |||
| was_target_encryption_set{false}, | |||
There was a problem hiding this comment.
Better names are m_target_encryption_set and m_actual_encryption_set
Why do you always use "was_"?
| @@ -125,6 +127,8 @@ class Object_table_impl : virtual public Object_table { | |||
|
|
|||
| virtual void set_encrypted(); | |||
There was a problem hiding this comment.
may be rename this set_target_encrypted() instead of generic set_encrypted(). We have sibling set_actual_encrypted().
| @@ -125,6 +127,8 @@ class Object_table_impl : virtual public Object_table { | |||
|
|
|||
| virtual void set_encrypted(); | |||
There was a problem hiding this comment.
Although the person who reviews this code should understand what actual vs target table means, it is better if there is atleast 'some' comment on function (along with description in commit message)
There was a problem hiding this comment.
I tried to reproduce the above crash in 8.0.15 and I couldn't. May be specific to patch or specific to 8.0.16. Will check
There is new bug with 8.0.16. default_table_encryption=ON at bootstrap doesn't encrypt mysql.ibd. I fixed this (wrote patch and I have to submit PR)
8.0.16 doesn't have the above issue. So this crash is specific to upgrade of encrypted mysql.ibd from 8.0.15 to 8.0.16
There was a problem hiding this comment.
Fixed as part of PS-5925 and PS-5926 (the blockers for this PR)
| @@ -58,8 +60,19 @@ Object_table_impl::Object_table_impl() | |||
| } | |||
|
|
|||
| void Object_table_impl::set_encrypted() { | |||
There was a problem hiding this comment.
And in the constructor the member variable is not set. This is a bug
I would suggest to call this set_encrypted() method from constructor
|
After applying patch: Crash after upgrade and restart Y-010116] [Server] /home/satya/WORK/PS-8.0/bld/runtime_output_directory/mysqld (mysqld 8.0.16-6-debug) starting as process 15893 Thread 2 "mysqld" received signal SIGABRT, Aborted. |
|
I tried to reproduce the above crash in 8.0.15 and I couldn't. May be specific to patch or specific to 8.0.16. Will check |
|
the above crash is easy to reproduce. Just add restart in the test |
|
Another thing to find out how the upstream does "ALTER TABLESPACE mysql ENCRYPTION='Y'". Does it add encryption attribute in dd_properties of all tables? |
|
Found the reason for crash. it is not related to this patch. I could reproduce the same crash in 8.0.15 using same datadir |
The answer is NO. Upstream does kind of "cheating". It doesn't treat mysql tablespace as encrypted during the bootstrap phase. |
Fixed as PS-5925 and PS-5926 |
|
My fear on double encryption is indeed true. I just bumped up DD version to simulate upgrade and it crashed. #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51 |
|
So on restart, dd::Object_table_impl::Object_table_impl() sets encryption property..
To get this crash: Apply this PR, get datadir, and then apply this patch Startup on this datadir by 8017 (upcoming merge too may be) will cause failure |
|
Very happy to find a problem that will occur in future 😊 |
|
Next onto fixing this issue |
A temporary patch that solves the double encryption issue. |
|
I have to check if we can survive without the on-disk change |
No, even if I remove dd::update_properties line from this PR, there is another place where dd::update_properties is called. So it goes to disk by default on upgrades. So two options infront of us
|
Its kind of pick the poison scenario
|
|
Other issues with deviating from upgrade on-disk format is that 8.0 provides ability to downgrade until as specific version (like two minor versions?). So such on-disk deviation will prevent downgrades |
DD properties are not updated but the table options of DD tables in mysql.tables.options are updated. |
Not sure if this is useful or not .. Because when DD is booted, the tables are created from DD properties? This is because mysql.tables is not available during bootstrap |
robgolebiowski
force-pushed
the
8_0_enc_dd_upgr_from_15
branch
from
1bb35f6 to
3b2a33a
Compare
Please elaborate the problem clearly and the document the fix |
robgolebiowski
force-pushed
the
8_0_enc_dd_upgr_from_15
branch
from
3b2a33a to
b7aefc2
Compare
robgolebiowski
force-pushed
the
8_0_enc_dd_upgr_from_15
branch
from
b7aefc2 to
d54858a
Compare
tablespace During upgrade both actual and target table definitions need to have ENCRYPTION='Y' set when we upgrading from encrypted mysql.ibd. When upgrading from version previous to 8.0.16 where mysql.ibd can be encrypted in PS we need to add ENCRYPTION='Y' to all the system target tables to be created in mysql tablespace. We also need to add ENCRYPTION='Y' to actual tables definitions, although those tables already exist. This step is only to create TABLE*/TABLE_SHARE* for core tables. Upstream does mysql.ibd encryption after bootstrap with ALTER statement. Thus encryption of mysql.ibd does not update SYSTEM_TABLES properties in dd_properties table. To be compliant we do not update dd_properties SYSTEM_TABLES with ENCRYPTION='Y' (per every SYSTEM TABLE) also.
robgolebiowski
force-pushed
the
8_0_enc_dd_upgr_from_15
branch
from
d54858a to
07203c4
Compare
tablespace
Upgrade creates tables in mysql tablespaces in two phases:
taken from dd_properties table, option SYSTEM_TABLES.
We add here in-memory encryption flag to CREATE statement
if the mysql tablespace is encrypted.
flag to CREATE statement if mysql tablespace is encrypted and update
SYSTEM_TABLES option in dd_properties with encryption flag.