[MDEV-13359] Enable online ALTER TABLE for compressed columns Created: 2017-07-20  Updated: 2017-09-13  Resolved: 2017-09-01

Status: Closed
Project: MariaDB Server
Component/s: Storage Engine - InnoDB
Affects Version/s: 10.3
Fix Version/s: 10.3.2

Type: Bug Priority: Major
Reporter: Sergey Vojtovich Assignee: Marko Mäkelä
Resolution: Fixed Votes: 0
Labels: None

Issue Links:
Relates
relates to MDEV-11371 Big column compressed Closed
relates to MDEV-12586 ALTER TABLE…ALGORITHM=INPLACE fails w... Closed
relates to MDEV-13625 Merge InnoDB test cases from MySQL 5.6 Closed
relates to MDEV-13795 ALTER TABLE…DROP PRIMARY KEY, ADD PRI... Closed

 Description   

!!! PLEASE DO NOT CHANGE OBJECTIVE OF THIS BUG AND DON'T CLOSE IT UNTIL DESCRIBED PROBLEM IS SOLVED !!!

Apparently InnoDB doesn't copy blob data during online ALTER TABLE. In a meanwhile it calls innobase_rec_reset(), which may reallocate memory for blobs.

It wasn't a problem before column compression was introduced, since in most (if not all) cases memory reallocation was not needed. With compressed columns reallocation is always there.

To reproduce this issue grep for MDEV-13359 in source code for relevant markers. Re-enable online ALTER TABLE and run main.column_compression test.

Objective of this bug is to enable online ALTER TABLE for compressed columns.

 Comments   
Comment by Marko Mäkelä [ 2017-08-07 ]

InnoDB does copy BLOB data ALTER TABLE…ALGORITHM=INPLACE, LOCK=NONE.

For ADD INDEX (adding secondary indexes that include column prefixes of BLOB columns), the BLOB prefixes are copied already when creating the secondary index entries in row0merge.cc.

For table-rebuilding online ALTER, entire BLOBs are copied in two places: when initially building the new clustered index in row_merge_insert_index_tuples(), or later in row_log_table_apply_convert_mrec(). The latter operation is protected by index->online_log->blobs, which keeps track of freed BLOB objects, so that if purge or rollback is freeing BLOBs during the log apply, we will avoid dereferencing dangling blob pointers.

svoj, what exactly do you mean by saying that blob data is not being copied? Please note that innobase_rec_reset() and innobase_row_to_mysql() are only used for reporting duplicate key errors.

Comment by Sergey Vojtovich [ 2017-08-25 ]

I confirm the problem is still there, in 10.3 with MDEV-12586 fixed. Unfortunately I lost original test and relevant code was changed substantially, so it is not easily repeatable anymore.

Rebased version of column compression is currently in bb-10.3-MDEV-11371.

Test case:

--source include/have_innodb.inc
 
 
 
CREATE TABLE t1(a INT) ENGINE=InnoDB;
 
INSERT INTO t1 VALUES(1);
 
ALTER TABLE t1 ADD COLUMN b BLOB COMPRESSED DEFAULT "must be visible";
 
SELECT * FROM t1;
 
DROP TABLE t1;

It should work pretty well, now apply the following patch:

diff --git a/sql/field.cc b/sql/field.cc
 
index cc9d856..f23b69c 100644
 
--- a/sql/field.cc
 
+++ b/sql/field.cc
 
@@ -8441,7 +8441,8 @@ int Field_blob_compressed::store(const char *from, uint length,
 
   uint to_length= MY_MIN(max_data_length(), field_charset->mbmaxlen * length + 1);
 
   int rc;
 
 
 
-  if (value.alloc(to_length))
 
+  /* Increase reallocation likelihood */
 
+  if (value.alloc(value.alloced_length() * 10 + to_length))
 
   {
 
     set_ptr((uint32) 0, NULL);
 
     return -1;

Works fine still, now revert InnoDB workaround:

diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc
 
index 98987e3..ceaa07b 100644
 
--- a/storage/innobase/handler/handler0alter.cc
 
+++ b/storage/innobase/handler/handler0alter.cc
 
@@ -719,13 +719,6 @@ ha_innobase::check_if_supported_inplace_alter(
 
 
 
                        DBUG_RETURN(HA_ALTER_INPLACE_NOT_SUPPORTED);
 
                }
 
-
 
-               /* Disable online ALTER TABLE for compressed columns until
 
-               MDEV-13359 - "Online ALTER TABLE will be disabled for compressed columns"
 
-               is fixed. */
 
-               if (field->compression_method()) {
 
-                       DBUG_RETURN(HA_ALTER_INPLACE_NOT_SUPPORTED);
 
-               }
 
        }
 
 
 
        ulint n_indexes = UT_LIST_GET_LEN((m_prebuilt->table)->indexes);
 
@@ -1024,12 +1017,6 @@ ha_innobase::check_if_supported_inplace_alter(
 
                            || (ha_alter_info->handler_flags
 
                                & Alter_inplace_info::ADD_COLUMN));
 
 
 
-               /* Disable online ALTER TABLE for compressed columns until
 
-               MDEV-13359 - "Online ALTER TABLE will be disabled for compressed columns"
 
-               is fixed. */
 
-               if (cf->compression_method()) {
 
-                       DBUG_RETURN(HA_ALTER_INPLACE_NOT_SUPPORTED);
 
-               }
 
                if (const Field* f = cf->field) {
 
                        /* This could be changing an existing column
 
                        from NULL to NOT NULL. */
 
diff --git a/storage/innobase/row/row0merge.cc b/storage/innobase/row/row0merge.cc
 
index 26eaf49..de1f35a 100644
 
--- a/storage/innobase/row/row0merge.cc
 
+++ b/storage/innobase/row/row0merge.cc
 
@@ -4786,9 +4786,7 @@ row_merge_build_indexes(
 
        }
 
 
 
        /* Reset the MySQL row buffer that is used when reporting
 
-       duplicate keys.
 
-
 
-       This is likely reason for a problem described in MDEV-13359. */
 
+       duplicate keys. */
 
        innobase_rec_reset(table);
 
 
 
        if (global_system_variables.log_warnings > 2) {

Now it either fails with "1259: ZLIB: Input data corrupted" or crashes with:

Thread 1 (Thread 0x7fae156ad700 (LWP 32549)):
 
#0  __pthread_kill (threadid=<optimized out>, signo=6) at ../sysdeps/unix/sysv/linux/pthread_kill.c:62
 
#1  0x000056010b8f6de0 in my_write_core (sig=6) at /home/svoj/devel/maria/mariadb/mysys/stacktrace.c:477
 
#2  0x000056010b182f79 in handle_fatal_signal (sig=6) at /home/svoj/devel/maria/mariadb/sql/signal_handler.cc:299
 
#3  <signal handler called>
 
#4  0x00007fae1bce7428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
 
#5  0x00007fae1bce902a in __GI_abort () at abort.c:89
 
#6  0x00007fae1bcdfbd7 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x56010babe70b "0", file=file@entry=0x56010babe6e0 "/home/svoj/devel/maria/mariadb/sql/field.h", line=line@entry=3288, function=function@entry=0x56010babe820 <_ZZL14read_bigendianPKhjE19__PRETTY_FUNCTION__> "longlong read_bigendian(const uchar*, uint)") at assert.c:92
 
#7  0x00007fae1bcdfc82 in __GI___assert_fail (assertion=0x56010babe70b "0", file=0x56010babe6e0 "/home/svoj/devel/maria/mariadb/sql/field.h", line=3288, function=0x56010babe820 <_ZZL14read_bigendianPKhjE19__PRETTY_FUNCTION__> "longlong read_bigendian(const uchar*, uint)") at assert.c:101
 
#8  0x000056010b17bf62 in read_bigendian (from=0x7fadc803a319 "", bytes=0) at /home/svoj/devel/maria/mariadb/sql/field.h:3288
 
#9  0x000056010b17c1b6 in uncompress_zlib (to=0x7fae156aa4b0, from=0x7fadc803a319 "", from_length=15, field_length=65535) at /home/svoj/devel/maria/mariadb/sql/field_comp.cc:78
 
#10 0x000056010b16699a in Field_longstr::uncompress (this=0x7fadc800a498, val_buffer=0x7fae156aa4b0, val_ptr=0x7fae156aa4b0, from=0x7fadc803a318 "\210", from_length=16) at /home/svoj/devel/maria/mariadb/sql/field.cc:7807
 
#11 0x000056010b16909d in Field_blob_compressed::val_str (this=0x7fadc800a498, val_buffer=0x7fae156aa4b0, val_ptr=0x7fae156aa4b0) at /home/svoj/devel/maria/mariadb/sql/field.cc:8460
 
#12 0x000056010adfc389 in Field::val_str (this=0x7fadc800a498, str=0x7fae156aa4b0) at /home/svoj/devel/maria/mariadb/sql/field.h:829
 
#13 0x000056010adfa82b in Protocol_text::store (this=0x7fadc8001098, field=0x7fadc800a498) at /home/svoj/devel/maria/mariadb/sql/protocol.cc:1245
 
#14 0x000056010b1aa657 in Item_field::send (this=0x7fadc8015878, protocol=0x7fadc8001098, buffer=0x7fae156aa890) at /home/svoj/devel/maria/mariadb/sql/item.cc:7009
 
#15 0x000056010adf9911 in Protocol::send_result_set_row (this=0x7fadc8001098, row_items=0x7fadc8004f38) at /home/svoj/devel/maria/mariadb/sql/protocol.cc:985
 
#16 0x000056010ae7d650 in select_send::send_data (this=0x7fadc80151d0, items=...) at /home/svoj/devel/maria/mariadb/sql/sql_class.cc:2823
 
#17 0x000056010af4204e in end_send (join=0x7fadc80151f0, join_tab=0x7fadc80167f0, end_of_records=false) at /home/svoj/devel/maria/mariadb/sql/sql_select.cc:20021
 
#18 0x000056010af3fbca in evaluate_join_record (join=0x7fadc80151f0, join_tab=0x7fadc8016440, error=0) at /home/svoj/devel/maria/mariadb/sql/sql_select.cc:19078
 
#19 0x000056010af3f4c1 in sub_select (join=0x7fadc80151f0, join_tab=0x7fadc8016440, end_of_records=false) at /home/svoj/devel/maria/mariadb/sql/sql_select.cc:18858
 
#20 0x000056010af3ea5c in do_select (join=0x7fadc80151f0, procedure=0x0) at /home/svoj/devel/maria/mariadb/sql/sql_select.cc:18398
 
#21 0x000056010af1834b in JOIN::exec_inner (this=0x7fadc80151f0) at /home/svoj/devel/maria/mariadb/sql/sql_select.cc:3540
 
#22 0x000056010af177fa in JOIN::exec (this=0x7fadc80151f0) at /home/svoj/devel/maria/mariadb/sql/sql_select.cc:3335
 
#23 0x000056010af189bc in mysql_select (thd=0x7fadc8000b00, tables=0x7fadc8014af0, wild_num=1, fields=..., conds=0x0, og_num=0, order=0x0, group=0x0, having=0x0, proc_param=0x0, select_options=2147748608, result=0x7fadc80151d0, unit=0x7fadc80046d0, select_lex=0x7fadc8004e10) at /home/svoj/devel/maria/mariadb/sql/sql_select.cc:3735
 
#24 0x000056010af0d188 in handle_select (thd=0x7fadc8000b00, lex=0x7fadc8004608, result=0x7fadc80151d0, setup_tables_done_option=0) at /home/svoj/devel/maria/mariadb/sql/sql_select.cc:376
 
#25 0x000056010aed927e in execute_sqlcom_select (thd=0x7fadc8000b00, all_tables=0x7fadc8014af0) at /home/svoj/devel/maria/mariadb/sql/sql_parse.cc:6441
 
#26 0x000056010aecf9da in mysql_execute_command (thd=0x7fadc8000b00) at /home/svoj/devel/maria/mariadb/sql/sql_parse.cc:3694
 
#27 0x000056010aedccd1 in mysql_parse (thd=0x7fadc8000b00, rawbuf=0x7fadc8014908 "SELECT * FROM t1", length=16, parser_state=0x7fae156ac080, is_com_multi=false, is_next_command=false) at /home/svoj/devel/maria/mariadb/sql/sql_parse.cc:7896
 
#28 0x000056010aeca80e in dispatch_command (command=COM_QUERY, thd=0x7fadc8000b00, packet=0x7fadc808f8f1 "", packet_length=16, is_com_multi=false, is_next_command=false) at /home/svoj/devel/maria/mariadb/sql/sql_parse.cc:1814
 
#29 0x000056010aec90d9 in do_command (thd=0x7fadc8000b00) at /home/svoj/devel/maria/mariadb/sql/sql_parse.cc:1372
 
#30 0x000056010b01914c in do_handle_one_connection (connect=0x56010e69f970) at /home/svoj/devel/maria/mariadb/sql/sql_connect.cc:1354
 
#31 0x000056010b018e9f in handle_one_connection (arg=0x56010e69f970) at /home/svoj/devel/maria/mariadb/sql/sql_connect.cc:1260
 
#32 0x000056010b3a7c6e in pfs_spawn_thread (arg=0x56010e6a7f90) at /home/svoj/devel/maria/mariadb/storage/perfschema/pfs.cc:1862
 
#33 0x00007fae1c9246ba in start_thread (arg=0x7fae156ad700) at pthread_create.c:333
 
#34 0x00007fae1bdb93dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Comment by Marko Mäkelä [ 2017-08-28 ]

InnoDB only calls the innobase_rec_reset() to ensure that when a duplicate key error is reported, the error message will not contain bogus values for columns whose values are unknown.
The equivalent call (to reset *handler::table->record) could be moved to the SQL layer, and the function innobase_rec_reset() can be removed.
While that could fix the scenario at hand, I would like you to clarify the following:

  1. What would happen if a duplicate key error is reported during ALTER TABLE…ALGORITHM=INPLACE?
  2. Where is the data ownership of handler::table->record[] documented?
  3. What if InnoDB started to support column type changes in ALGORITHM=INPLACE, and it used TABLE::record[] to let the SQL layer convert the values?
  4. Is there any conflict with the 10.2 MDEV-5800 ALTER TABLE…ADD INDEX(virtual_column)? I believe that TABLE::record[] could be used there for computing the indexed virtual column values.
Comment by Sergey Vojtovich [ 2017-08-28 ]

1. What would happen if a duplicate key error is reported during ALTER TABLE…ALGORITHM=INPLACE?

I haven't tested it.

2. Where is the data ownership of handler::table->record[] documented?

I don't understand this question. Did you mean life time? Value is there until it is updated, in this case it gets updated by innobase_rec_reset().

3. What if InnoDB started to support column type changes in ALGORITHM=INPLACE, and it used TABLE::record[] to let the SQL layer convert the values?

Fine with me, assuming it doesn't use same record[] for in/out.

4. Is there any conflict with the 10.2 MDEV-5800 ALTER TABLE…ADD INDEX(virtual_column)? I believe that TABLE::record[] could be used there for computing the indexed virtual column values.

Conflict with what?

Comment by Sergey Vojtovich [ 2017-08-28 ]

Note that I can be very wrong about innobase_rec_reset() being culprit. Last time I debugged it was about half year ago. Tencent also came across this issue in their implementation and they had to disable it. Probably for different reason though.

Comment by Marko Mäkelä [ 2017-09-01 ]

The fix is to simply remove the function innobase_rec_reset().

I had introduced this function in the InnoDB Plugin for MySQL 5.1, which later evolved into MySQL 5.5. There used to be a bug that ADD UNIQUE INDEX would not always correctly report the duplicate key value of the secondary index. This function ensured that instead of reporting totally garbage values, InnoDB would report column default values.

It looks like I made the function unnecessary in MySQL 5.6.6 and adjusted the test.

Now that the InnoDB ALTER TABLE tests were imported as part of MDEV-13625, I was able to confirm that it is safe to remove the function.

The unnecessary function did not do any harm before MDEV-11371 introduced compressed columns.

One question remains: What if we needed to report a duplicate key value for a compressed column? The simple answer is that the test main.column_compression demonstrates that no indexes can be defined on compressed columns.

Generated at Thu Feb 08 08:04:59 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.