maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #10999
Re: review of MDEV-10177 Hidden columns
Hi Serg!
Thanks for the review.
Please review the updated patch , at bb-hidden-test branch
I am currently working on this , only 2 things are left.
Changes apart from comments.
Rebased to latest 10.3.
Completely_invisible, or HA_INVISIBLE_INDEX is only deleted when same
name column/
index is created(with tests).
On Thu, Sep 28, 2017 at 8:15 PM, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> Hi, Sachin!
>
> Here's a review of all changes between 474f51711b1 and f0e2cf3e6c8:
>
>> diff --git a/include/my_base.h b/include/my_base.h
>> index b93300d7562..68c828d5f74 100644
>> --- a/include/my_base.h
>> +++ b/include/my_base.h
>> @@ -277,7 +277,8 @@ enum ha_base_keytype {
>> This flag can be calculated -- it's based on key lengths comparison.
>> */
>> #define HA_KEY_HAS_PART_KEY_SEG 65536
>> -
>> +/* Internal Flag Can be calcaluted */
>> +#define HA_INVISIBLE_SYSTEM_KEY 2<<18 /* Is it a *Invisible* key */
>
> "an invisible key"
>
> and I'd just use HA_INVISIBLE_KEY
>
erledigt.
>> /* Automatic bits in key-flag */
>>
>> #define HA_SPACE_PACK_USED 4 /* Test for if SPACE_PACK used */
>> diff --git a/mysql-test/r/features.result b/mysql-test/r/features.result
>> index c6d1a6b0bac..3acd7436ad9 100644
>> --- a/mysql-test/r/features.result
>> +++ b/mysql-test/r/features.result
>> @@ -8,6 +8,7 @@ Feature_delay_key_write 0
>> Feature_dynamic_columns 0
>> Feature_fulltext 0
>> Feature_gis 0
>> +Feature_hidden_columns 0
>
> You forgot to rename the variable.
> and let's rename test files too, it's a bit confusing now.
>
Done. I guess in all test files I have replaced hidden with invisible.
Apart from this I forgot this change.
>> Feature_locale 0
>> Feature_subquery 0
>> Feature_timezone 0
>> diff --git a/mysql-test/r/hidden_field.result b/mysql-test/r/hidden_field.result
>> new file mode 100644
>> index 00000000000..7ff6f617eef
>> --- /dev/null
>> +++ b/mysql-test/r/hidden_field.result
>> @@ -0,0 +1,692 @@
>> +FLUSH STATUS;
>> +create table t1(abc int primary key, xyz int invisible);
>> +SHOW STATUS LIKE 'Feature_invisible_columns';
>> +Variable_name Value
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +abc int(11) NO PRI NULL
>> +xyz int(11) YES NULL INVISIBLE
>> +show create table t1;
>> +Table Create Table
>> +t1 CREATE TABLE `t1` (
>> + `abc` int(11) NOT NULL,
>> + `xyz` int(11) INVISIBLE DEFAULT NULL,
>> + PRIMARY KEY (`abc`)
>> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
>> +select * from INFORMATION_SCHEMA.COLUMNS where TABLE_SCHEMA='test' and TABLE_NAME='t1';
>> +TABLE_CATALOG TABLE_SCHEMA TABLE_NAME COLUMN_NAME ORDINAL_POSITION COLUMN_DEFAULT IS_NULLABLE DATA_TYPE CHARACTER_MAXIMUM_LENGTH CHARACTER_OCTET_LENGTH NUMERIC_PRECISION NUMERIC_SCALE DATETIME_PRECISION CHARACTER_SET_NAME COLLATION_NAME COLUMN_TYPE COLUMN_KEY EXTRA PRIVILEGES COLUMN_COMMENT IS_GENERATED GENERATION_EXPRESSION
>> +def test t1 abc 1 NULL NO int NULL NULL 10 0 NULL NULL NULL int(11) PRI select,insert,update,references NEVER NULL
>> +def test t1 xyz 2 NULL YES int NULL NULL 10 0 NULL NULL NULL int(11) INVISIBLE select,insert,update,references NEVER NULL
>> +drop table t1;
>> +create table t1(a1 int invisible);
>> +ERROR 42000: A table must have at least 1 column
>> +create table t1(a1 blob,invisible(a1));
>> +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '(a1))' at line 1
>> +create table t1(a1 int primary key invisible ,a2 int unique invisible , a3 blob,a4 int not null invisible unique);
>> +ERROR HY000: Hidden column 'a1' must either have a default value or allow null values
>> +create table t1(abc int not null invisible);
>> +ERROR HY000: Hidden column 'abc' must either have a default value or allow null values
>> +create table t1(a int invisible, b int);
>> +insert into t1 values(1);
>> +insert into t1(a) values(2);
>> +insert into t1(b) values(3);
>> +insert into t1(a,b) values(5,5);
>> +select * from t1;
>> +b
>> +1
>> +NULL
>> +3
>> +5
>> +select a,b from t1;
>> +a b
>> +NULL 1
>> +2 NULL
>> +NULL 3
>> +5 5
>> +delete from t1;
>> +insert into t1 values(1),(2),(3),(4);
>> +select * from t1;
>> +b
>> +1
>> +2
>> +3
>> +4
>> +select a from t1;
>> +a
>> +NULL
>> +NULL
>> +NULL
>> +NULL
>> +drop table t1;
>> +#more complex case of invisible
>> +create table t1(a int , b int invisible , c int invisible auto_increment unique, d blob , e int unique, f int);
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +b int(11) YES NULL INVISIBLE
>
> remove a space character before "INVISIBLE"
Okay.
>
>> +c int(11) NO PRI NULL auto_increment, INVISIBLE
>> +d blob YES NULL
>> +e int(11) YES UNI NULL
>> +f int(11) YES NULL
>> +insert into t1 values(1,'d blob',1,1),(1,'d blob',11,1),(1,'d blob',2,1),(1,'d blob',3,1),(1,'d blob',41,1);
>> +select * from t1;
>> +a d e f
>> +1 d blob 1 1
>> +1 d blob 11 1
>> +1 d blob 2 1
>> +1 d blob 3 1
>> +1 d blob 41 1
>
> would be nice to see `select a,b,c,d,e,f` here
>
Done.
>> +drop table t1;
>> +create table sdsdsd(a int , b int, invisible(a,b));
>> +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '(a,b))' at line 1
>> +create table t1(a int,abc int as (a mod 3) virtual invisible);
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +abc int(11) YES NULL VIRTUAL GENERATED, INVISIBLE
>> +insert into t1 values(1,default);
>> +ERROR 21S01: Column count doesn't match value count at row 1
>> +insert into t1 values(1),(22),(233);
>> +select * from t1;
>> +a
>> +1
>> +22
>> +233
>> +select a,abc from t1;
>> +a abc
>> +1 1
>> +22 1
>> +233 2
>> +drop table t1;
>> +create table t1(abc int primary key invisible auto_increment, a int);
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +abc int(11) NO PRI NULL auto_increment, INVISIBLE
>> +a int(11) YES NULL
>> +show create table t1;
>> +Table Create Table
>> +t1 CREATE TABLE `t1` (
>> + `abc` int(11) NOT NULL INVISIBLE AUTO_INCREMENT,
>> + `a` int(11) DEFAULT NULL,
>> + PRIMARY KEY (`abc`)
>> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
>> +insert into t1 values(1);
>> +insert into t1 values(2);
>> +insert into t1 values(3);
>> +select * from t1;
>> +a
>> +1
>> +2
>> +3
>> +select abc,a from t1;
>> +abc a
>> +1 1
>> +2 2
>> +3 3
>> +delete from t1;
>> +insert into t1 values(1),(2),(3),(4),(6);
>> +select abc,a from t1;
>> +abc a
>> +4 1
>> +5 2
>> +6 3
>> +7 4
>> +8 6
>> +drop table t1;
>> +create table t1(abc int);
>> +alter table t1 change abc ss int invisible;
>> +ERROR 42000: A table must have at least 1 column
>> +alter table t1 add column xyz int;
>> +alter table t1 modify column abc int ;
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +abc int(11) YES NULL
>> +xyz int(11) YES NULL
>> +insert into t1 values(22);
>> +ERROR 21S01: Column count doesn't match value count at row 1
>> +alter table t1 modify column abc int invisible;
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +abc int(11) YES NULL INVISIBLE
>> +xyz int(11) YES NULL
>> +insert into t1 values(12);
>> +drop table t1;
>> +#some test on copy table structure with table data;
>> +#table with invisible fields and unique keys;
>> +create table t1(a int , b int invisible , c int invisible auto_increment unique, d blob , e int unique, f int);
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +b int(11) YES NULL INVISIBLE
>> +c int(11) NO PRI NULL auto_increment, INVISIBLE
>> +d blob YES NULL
>> +e int(11) YES UNI NULL
>> +f int(11) YES NULL
>> +insert into t1 values(1,'d blob',1,1),(1,'d blob',11,1),(1,'d blob',2,1),(1,'d blob',3,1),(1,'d blob',41,1);
>> +select * from t1;
>> +a d e f
>> +1 d blob 1 1
>> +1 d blob 11 1
>> +1 d blob 2 1
>> +1 d blob 3 1
>> +1 d blob 41 1
>> +select a,b,c,d,e,f from t1;
>> +a b c d e f
>> +1 NULL 1 d blob 1 1
>> +1 NULL 2 d blob 11 1
>> +1 NULL 3 d blob 2 1
>> +1 NULL 4 d blob 3 1
>> +1 NULL 5 d blob 41 1
>> +#this wont copy invisible fields and keys;
>
> won't
>
Done
>> +create table t2 as select * from t1;
>> +desc t2;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +d blob YES NULL
>> +e int(11) YES NULL
>> +f int(11) YES NULL
>> +select * from t2;
>> +a d e f
>> +1 d blob 1 1
>> +1 d blob 11 1
>> +1 d blob 2 1
>> +1 d blob 3 1
>> +1 d blob 41 1
>> +select a,b,c,d,e,f from t2;
>> +ERROR 42S22: Unknown column 'b' in 'field list'
>> +drop table t2;
>> +#now this will copy invisible fields
>> +create table t2 as select a,b,c,d,e,f from t1;
>> +desc t2;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +b int(11) YES NULL
>> +c int(11) NO 0
>> +d blob YES NULL
>> +e int(11) YES NULL
>> +f int(11) YES NULL
>> +select * from t2;
>> +a b c d e f
>> +1 NULL 1 d blob 1 1
>> +1 NULL 2 d blob 11 1
>> +1 NULL 3 d blob 2 1
>> +1 NULL 4 d blob 3 1
>> +1 NULL 5 d blob 41 1
>> +select a,b,c,d,e,f from t2;
>> +a b c d e f
>> +1 NULL 1 d blob 1 1
>> +1 NULL 2 d blob 11 1
>> +1 NULL 3 d blob 2 1
>> +1 NULL 4 d blob 3 1
>> +1 NULL 5 d blob 41 1
>> +drop table t2,t1;
>> +#some test related to copy of data from one table to another;
>> +create table t1(a int , b int invisible , c int invisible auto_increment unique, d blob , e int unique, f int);
>> +insert into t1 values(1,'d blob',1,1),(1,'d blob',11,1),(1,'d blob',2,1),(1,'d blob',3,1),(1,'d blob',41,1);
>> +select a,b,c,d,e,f from t1;
>> +a b c d e f
>> +1 NULL 1 d blob 1 1
>> +1 NULL 2 d blob 11 1
>> +1 NULL 3 d blob 2 1
>> +1 NULL 4 d blob 3 1
>> +1 NULL 5 d blob 41 1
>> +create table t2(a int , b int invisible , c int invisible , d blob , e int unique, f int);
>> +insert into t2 select * from t1;
>> +select a,b,c,d,e,f from t2;
>> +a b c d e f
>> +1 NULL NULL d blob 1 1
>> +1 NULL NULL d blob 11 1
>> +1 NULL NULL d blob 2 1
>> +1 NULL NULL d blob 3 1
>> +1 NULL NULL d blob 41 1
>> +truncate t2;
>> +insert into t2 (a,b,c,d,e,f) select a,b,c,d,e,f from t1;
>> +select a,b,c,d,e,f from t2;
>> +a b c d e f
>> +1 NULL 1 d blob 1 1
>> +1 NULL 2 d blob 11 1
>> +1 NULL 3 d blob 2 1
>> +1 NULL 4 d blob 3 1
>> +1 NULL 5 d blob 41 1
>> +truncate t2;
>> +drop table t1,t2;
>> +#some test related to creating view on table with invisible column;
>> +create table t1(a int , b int invisible , c int invisible auto_increment unique, d blob , e int unique, f int);
>> +insert into t1 values(1,'d blob',1,1),(1,'d blob',11,1),(1,'d blob',2,1),(1,'d blob',3,1),(1,'d blob',41,1);
>> +create view v as select * from t1;
>> +desc v;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +d blob YES NULL
>> +e int(11) YES NULL
>> +f int(11) YES NULL
>> +select * from v;
>> +a d e f
>> +1 d blob 1 1
>> +1 d blob 11 1
>> +1 d blob 2 1
>> +1 d blob 3 1
>> +1 d blob 41 1
>> +#v does not have invisible column;
>> +select a,b,c,d,e,f from v;
>> +ERROR 42S22: Unknown column 'b' in 'field list'
>> +insert into v values(1,21,32,4);
>> +select * from v;
>> +a d e f
>> +1 d blob 1 1
>> +1 d blob 11 1
>> +1 d blob 2 1
>> +1 d blob 3 1
>> +1 d blob 41 1
>> +1 21 32 4
>> +insert into v(a,b,c,d,e,f) values(1,12,3,4,5,6);
>> +ERROR 42S22: Unknown column 'b' in 'field list'
>
> good tests, with views. this behavior is not obvious
>
right.
>> +drop view v;
>> +create view v as select a,b,c,d,e,f from t1;
>> +desc v;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +b int(11) YES NULL
>> +c int(11) NO 0
>> +d blob YES NULL
>> +e int(11) YES NULL
>> +f int(11) YES NULL
>> +select * from v;
>> +a b c d e f
>> +1 NULL 1 d blob 1 1
>> +1 NULL 2 d blob 11 1
>> +1 NULL 3 d blob 2 1
>> +1 NULL 4 d blob 3 1
>> +1 NULL 5 d blob 41 1
>> +1 NULL 6 21 32 4
>> +#v does have invisible column but they aren't invisible anymore.
>> +select a,b,c,d,e,f from v;
>> +a b c d e f
>> +1 NULL 1 d blob 1 1
>> +1 NULL 2 d blob 11 1
>> +1 NULL 3 d blob 2 1
>> +1 NULL 4 d blob 3 1
>> +1 NULL 5 d blob 41 1
>> +1 NULL 6 21 32 4
>> +insert into v values(1,26,33,4,45,66);
>> +select a,b,c,d,e,f from v;
>> +a b c d e f
>> +1 NULL 1 d blob 1 1
>> +1 NULL 2 d blob 11 1
>> +1 NULL 3 d blob 2 1
>> +1 NULL 4 d blob 3 1
>> +1 NULL 5 d blob 41 1
>> +1 NULL 6 21 32 4
>> +1 26 33 4 45 66
>> +insert into v(a,b,c,d,e,f) values(1,32,31,41,5,6);
>> +select a,b,c,d,e,f from v;
>> +a b c d e f
>> +1 NULL 1 d blob 1 1
>> +1 NULL 2 d blob 11 1
>> +1 NULL 3 d blob 2 1
>> +1 NULL 4 d blob 3 1
>> +1 NULL 5 d blob 41 1
>> +1 NULL 6 21 32 4
>> +1 26 33 4 45 66
>> +1 32 31 41 5 6
>> +drop view v;
>> +drop table t1;
>> +#now invisible column in where and some join query
>> +create table t1 (a int unique , b int invisible unique, c int unique invisible);
>> +insert into t1(a,b,c) values(1,1,1);
>> +insert into t1(a,b,c) values(2,2,2);
>> +insert into t1(a,b,c) values(3,3,3);
>> +insert into t1(a,b,c) values(4,4,4);
>> +insert into t1(a,b,c) values(21,21,26);
>> +insert into t1(a,b,c) values(31,31,35);
>> +insert into t1(a,b,c) values(41,41,45);
>> +insert into t1(a,b,c) values(22,22,24);
>> +insert into t1(a,b,c) values(32,32,33);
>> +insert into t1(a,b,c) values(42,42,43);
>> +explain select * from t1 where b=3;
>> +id select_type table type possible_keys key key_len ref rows Extra
>> +1 SIMPLE t1 const b b 5 const 1
>> +select * from t1 where b=3;
>> +a
>> +3
>> +explain select * from t1 where c=3;
>> +id select_type table type possible_keys key key_len ref rows Extra
>> +1 SIMPLE t1 const c c 5 const 1
>> +select * from t1 where c=3;
>> +a
>> +3
>> +create table t2 as select a,b,c from t1;
>> +desc t2;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +b int(11) YES NULL
>> +c int(11) YES NULL
>> +explain select * from t1,t2 where t1.b = t2.c and t1.c = t2.b;
>> +id select_type table type possible_keys key key_len ref rows Extra
>> +1 SIMPLE t2 ALL NULL NULL NULL NULL 10
>> +1 SIMPLE t1 ALL b,c NULL NULL NULL 10 Using where; Using join buffer (flat, BNL join)
>> +select * from t1,t2 where t1.b = t2.c and t1.c = t2.b;
>> +a a b c
>> +1 1 1 1
>> +2 2 2 2
>> +3 3 3 3
>> +4 4 4 4
>> +drop table t1,t2;
>> +#Unhide invisible columns
>> +create table t1 (a int primary key, b int invisible, c int invisible unique);
>> +show create table t1;
>> +Table Create Table
>> +t1 CREATE TABLE `t1` (
>> + `a` int(11) NOT NULL,
>> + `b` int(11) INVISIBLE DEFAULT NULL,
>> + `c` int(11) INVISIBLE DEFAULT NULL,
>> + PRIMARY KEY (`a`),
>> + UNIQUE KEY `c` (`c`)
>> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) NO PRI NULL
>> +b int(11) YES NULL INVISIBLE
>> +c int(11) YES UNI NULL INVISIBLE
>> +alter table t1 modify column b int;
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) NO PRI NULL
>> +b int(11) YES NULL
>> +c int(11) YES UNI NULL INVISIBLE
>> +alter table t1 change column c d int;
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) NO PRI NULL
>> +b int(11) YES NULL
>> +d int(11) YES UNI NULL
>> +drop table t1;
>> +SHOW STATUS LIKE 'Feature_invisible_columns';
>> +Variable_name Value
>> +#invisible is non reserved
>> +create table t1(a int unique , invisible int invisible, c int );
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) YES UNI NULL
>> +invisible int(11) YES NULL INVISIBLE
>> +c int(11) YES NULL
>> +alter table t1 change column invisible hid int invisible;
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) YES UNI NULL
>> +hid int(11) YES NULL INVISIBLE
>> +c int(11) YES NULL
>> +drop table t1;
>> +set debug_dbug= "+d,test_pseudo_invisible";
>> +create table t1(a int);
>> +set debug_dbug="";
>
> better do
>
> set @old_dbug=@@debug_dbug;
> ...
> set debug_dbug=@old_dbug;
>
> otherwise you you'll disable dbug when someone runs `./mtr --debug ...`
>
done.
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +show create table t1;
>> +Table Create Table
>> +t1 CREATE TABLE `t1` (
>> + `a` int(11) DEFAULT NULL
>> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
>> +insert into t1 values(1);
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +drop table t1;
>> +set debug_dbug= "+d,test_completely_invisible";
>> +create table t1(a int);
>> +set debug_dbug="";
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +show create table t1;
>> +Table Create Table
>> +t1 CREATE TABLE `t1` (
>> + `a` int(11) DEFAULT NULL
>> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
>> +insert into t1 values(1);
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +ERROR 42S22: Unknown column 'invisible' in 'field list'
>> +set debug_dbug= "+d,test_completely_invisible";
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +set debug_dbug="";
>> +drop table t1;
>> +set debug_dbug= "+d,test_pseudo_invisible";
>> +create table t1(a int);
>> +set debug_dbug="";
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +insert into t1 values(1);
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +ALTER table t1 change invisible b int;
>> +ERROR 42S22: Unknown column 'invisible' in 't1'
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +ALTER table t1 modify invisible char;
>> +ERROR 42S22: Unknown column 'invisible' in 't1'
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +ALTER table t1 drop invisible;
>> +ERROR 42000: Can't DROP COLUMN `invisible`; check that it exists
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +ALTER table t1 add invisible int;
>> +ERROR 42S21: Duplicate column name 'invisible'
>
> ouch. this violates the principle that these columns are *only*
> visible in SELECT. But, I guess, this effect is unavoidable :(
>
Right, Because if want to give correct error then this is unavoidable.
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +ALTER table t1 add invisible2 int default 2;
>> +select * from t1;
>> +a invisible2
>> +1 2
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +drop table t1;
>> +set debug_dbug= "+d,test_completely_invisible";
>> +create table t1(a int);
>> +desc t1;
>> +Field Type Null Key Default Extra
>> +a int(11) YES NULL
>> +insert into t1 values(1);
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +ALTER table t1 change invisible b int;
>> +ERROR 42S22: Unknown column 'invisible' in 't1'
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +ALTER table t1 modify invisible char;
>> +ERROR 42S22: Unknown column 'invisible' in 't1'
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +ALTER table t1 drop invisible;
>> +ERROR 42000: Can't DROP COLUMN `invisible`; check that it exists
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +ALTER table t1 add invisible int;
>> +ERROR 42S21: Duplicate column name 'invisible'
>
> Nope, for a completely invisible column, this ALTER should work.
> just rename the completely invisible column to something else.
>
Done.
>> +select * from t1;
>> +a
>> +1
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +set debug_dbug="";
>> +ALTER table t1 add hid int default 2;
>> +set debug_dbug= "+d,test_completely_invisible";
>> +select * from t1;
>> +a hid
>> +1 2
>> +select invisible ,a from t1;
>> +invisible a
>> +9 1
>> +drop table t1;
>> +set debug_dbug="";
>> +Create table t1( a int default(99) invisible, b int);
>> +insert into t1 values(1);
>> +insert into t1 values(2);
>> +insert into t1 values(3);
>> +insert into t1 values(4);
>> +select * from t1;
>> +b
>> +1
>> +2
>> +3
>> +4
>> +alter table t1 add index(a);
>> +alter table t1 add index(a,b);
>> +show index from t1;
>> +Table Non_unique Key_name Seq_in_index Column_name Collation Cardinality Sub_part Packed Null Index_type Comment Index_comment
>> +t1 1 a 1 a A NULL NULL NULL YES BTREE
>> +t1 1 a_2 1 a A NULL NULL NULL YES BTREE
>> +t1 1 a_2 2 b A NULL NULL NULL YES BTREE
>> +drop table t1;
>> +set debug_dbug= "+d,test_pseudo_invisible";
>> +Create table t1( a int default(99) invisible, b int);
>> +Create table t2( a int default(99) invisible, b int, unique(invisible));
>> +ERROR 42000: Key column 'invisible' doesn't exist in table
>> +set debug_dbug="";
>> +insert into t1 values(1);
>> +insert into t1 values(2);
>> +insert into t1 values(3);
>> +insert into t1 values(4);
>> +select * from t1;
>> +b
>> +1
>> +2
>> +3
>> +4
>> +select invisible, a, b from t1;
>> +invisible a b
>> +9 99 1
>> +9 99 2
>> +9 99 3
>> +9 99 4
>> +alter table t1 add index(invisible);
>> +ERROR 42000: Key column 'invisible' doesn't exist in table
>> +alter table t1 add index(b,invisible);
>> +ERROR 42000: Key column 'invisible' doesn't exist in table
>> +show index from t1;
>> +Table Non_unique Key_name Seq_in_index Column_name Collation Cardinality Sub_part Packed Null Index_type Comment Index_comment
>> +drop table t1;
>> +set debug_dbug= "+d,test_completely_invisible";
>> +Create table t1( a int default(99) invisible, b int);
>> +Create table t2( a int default(99) invisible, b int, unique(invisible));
>> +ERROR 42000: Key column 'invisible' doesn't exist in table
>> +insert into t1 values(1);
>> +insert into t1 values(2);
>> +insert into t1 values(3);
>> +insert into t1 values(4);
>> +select * from t1;
>> +b
>> +1
>> +2
>> +3
>> +4
>> +select invisible, a, b from t1;
>> +invisible a b
>> +9 99 1
>> +9 99 2
>> +9 99 3
>> +9 99 4
>> +set debug_dbug="";
>> +alter table t1 add index(invisible);
>> +ERROR 42000: Key column 'invisible' doesn't exist in table
>> +alter table t1 add index(b,invisible);
>> +ERROR 42000: Key column 'invisible' doesn't exist in table
>> +show index from t1;
>> +Table Non_unique Key_name Seq_in_index Column_name Collation Cardinality Sub_part Packed Null Index_type Comment Index_comment
>> +drop table t1;
>> +set debug_dbug= "+d,test_completely_invisible,test_invisible_index";
>
> hmm, please move all tests that use debug_dbug to a separate file.
> for example, hidden_field_debug.test
> so that hidden_field.test could also be run in non-debug builds.
>
> we only build with debug on fulltest2 builder, so currently your whole
> hidden_field.test is only run on that one single builder.
>
Done.
>> +Create table t1( a int default(99) , b int, index system_gen (invisible), index(b));
>> +set debug_dbug="";
>> +Show index from t1;
>> +Table Non_unique Key_name Seq_in_index Column_name Collation Cardinality Sub_part Packed Null Index_type Comment Index_comment
>> +t1 1 b 1 b A NULL NULL NULL YES BTREE
>> +select * from INFORMATION_SCHEMA.STATISTICS where TABLE_SCHEMA ='test' and table_name='t1';
>> +TABLE_CATALOG TABLE_SCHEMA TABLE_NAME NON_UNIQUE INDEX_SCHEMA INDEX_NAME SEQ_IN_INDEX COLUMN_NAME COLLATION CARDINALITY SUB_PART PACKED NULLABLE INDEX_TYPE COMMENT INDEX_COMMENT
>> +def test t1 1 test b 1 b A NULL NULL NULL YES BTREE
>> +show create table t1;
>> +Table Create Table
>> +t1 CREATE TABLE `t1` (
>> + `a` int(11) DEFAULT 99,
>> + `b` int(11) DEFAULT NULL,
>> + KEY `b` (`b`)
>> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
>
> I've just realized, there's one problem with that.
> completely invisible columns must be completely invisible, as if they
> don't exist at all. So ALTER TABLE ... ADD COLUMN should work,
> as if a completely invisible column didn't exist.
>
Done.
> But there can be only 64 (?) indexes per table. So one will be able to
> notice a completely invisible index, because one won't be able to
> create all 64 indexes.
>
> I don't see any solution for this, unfortunately.
>
>> +insert into t1 values(1,1);
>> +insert into t1 values(1,1);
>> +insert into t1 values(1,1);
>> +insert into t1 values(1,1);
>> +set debug_dbug= "+d,test_completely_invisible";
>> +select invisible, a ,b from t1;
>> +invisible a b
>> +9 1 1
>> +9 1 1
>> +9 1 1
>> +9 1 1
>> +explain select * from t1 where invisible =9;
>> +id select_type table type possible_keys key key_len ref rows Extra
>> +1 SIMPLE t1 ref system_gen system_gen 5 const 3
>> +set debug_dbug="";
>> +alter table t1 add x int default 3;
>> +set debug_dbug= "+d,test_completely_invisible";
>> +select invisible, a ,b from t1;
>> +invisible a b
>> +9 1 1
>> +9 1 1
>> +9 1 1
>> +9 1 1
>> +set debug_dbug="";
>> +Show index from t1;
>> +Table Non_unique Key_name Seq_in_index Column_name Collation Cardinality Sub_part Packed Null Index_type Comment Index_comment
>> +t1 1 b 1 b A NULL NULL NULL YES BTREE
>> +create index a1 on t1(invisible);
>> +ERROR 42000: Key column 'invisible' doesn't exist in table
>> +drop index system_gen on t1;
>> +ERROR 42000: Can't DROP INDEX `system_gen`; check that it exists
>> +drop table t1;
>> diff --git a/mysql-test/t/hidden_binlog.test b/mysql-test/t/hidden_binlog.test
>> new file mode 100644
>> index 00000000000..3e876dcca2e
>> --- /dev/null
>> +++ b/mysql-test/t/hidden_binlog.test
>> @@ -0,0 +1,32 @@
>> +--source include/master-slave.inc
>> +
>> +--connection master
>> +create table t1(a int , b int invisible);
>> +insert into t1 values(1);
>> +insert into t1(a,b) values(2,2);
>> +select a,b from t1;
>> +desc t1;
>> +
>> +create table t2(a int , b int invisible default 5);
>> +insert into t1 values(1);
>> +insert into t1(a,b) values(2,2);
>
> typo: you want to insert into t2
>
Done.
>> +select a,b from t2;
>> +desc t2;
>> +
>> +
>> +--sync_slave_with_master
>> +select * from t1;
>> +select a,b from t1;
>> +desc t1;
>> +show create table t1;
>> +
>> +select * from t2;
>> +select a,b from t2;
>> +desc t2;
>> +show create table t2;
>> +
>> +
>> +--connection master
>> +drop table t1,t2;
>> +
>> +--source include/rpl_end.inc
>> diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
>> index d0fb0ee1772..ddd26e1c080 100644
>> --- a/sql/share/errmsg-utf8.txt
>> +++ b/sql/share/errmsg-utf8.txt
>> @@ -7779,3 +7779,5 @@ ER_COMPRESSED_COLUMN_USED_AS_KEY
>> eng "Compressed column '%-.192s' can't be used in key specification"
>> ER_UNKNOWN_COMPRESSION_METHOD
>> eng "Unknown compression method: %s"
>> +ER_INVISIBLE_NOT_NULL_WITHOUT_DEFAULT
>> + eng "Hidden column '%s' must either have a default value or allow null values"
>
> 1. s/Hidden/Invisible/
> 2. s/allow null values/be nullable/
> 3. s/'%s'/%`s/
>
> that is "Invisible column %`s must either have a default value or be nullable"
>
> or you can say simply "Invisible column %`s must have a default value"
Done , used this.
> because a nullable column has a default value of NULL
>
>> diff --git a/sql/sql_class.h b/sql/sql_class.h
>> index 92f28a4dc07..2dba3fab72e 100644
>> --- a/sql/sql_class.h
>> +++ b/sql/sql_class.h
>> @@ -303,22 +303,25 @@ class Key :public Sql_alloc, public DDL_options {
>> LEX_CSTRING name;
>> engine_option_value *option_list;
>> bool generated;
>> + bool system_generated_invisible;
>
> again, why "system_generated_invisible"? what else can an invisible index be?
>
Changed.
>>
>> Key(enum Keytype type_par, const LEX_CSTRING *name_arg,
>> ha_key_alg algorithm_arg, bool generated_arg, DDL_options_st ddl_options)
>> :DDL_options(ddl_options),
>> type(type_par), key_create_info(default_key_create_info),
>> - name(*name_arg), option_list(NULL), generated(generated_arg)
>> + name(*name_arg), option_list(NULL), generated(generated_arg),
>> + system_generated_invisible(false)
>> {
>> key_create_info.algorithm= algorithm_arg;
>> }
>> Key(enum Keytype type_par, const LEX_CSTRING *name_arg,
>> KEY_CREATE_INFO *key_info_arg,
>> bool generated_arg, List<Key_part_spec> *cols,
>> engine_option_value *create_opt, DDL_options_st ddl_options)
>> :DDL_options(ddl_options),
>> type(type_par), key_create_info(*key_info_arg), columns(*cols),
>> - name(*name_arg), option_list(create_opt), generated(generated_arg)
>> + name(*name_arg), option_list(create_opt), generated(generated_arg),
>> + system_generated_invisible(false)
>> {}
>> Key(const Key &rhs, MEM_ROOT *mem_root);
>> virtual ~Key() {}
>> diff --git a/sql/field.cc b/sql/field.cc
>> index 4449d0ecf31..6c6f4d31534 100644
>> --- a/sql/field.cc
>> +++ b/sql/field.cc
>> @@ -1596,7 +1596,7 @@ Field::Field(uchar *ptr_arg,uint32 length_arg,uchar *null_ptr_arg,
>> unireg_check(unireg_check_arg), field_length(length_arg),
>> null_bit(null_bit_arg), is_created_from_null_item(FALSE),
>> read_stats(NULL), collected_stats(0), vcol_info(0), check_constraint(0),
>> - default_value(0)
>> + default_value(0),field_visibility(NOT_INVISIBLE)
>
> Hmm, I think gcc emits a warning for this. You need to initialize fields in
> the same order you declare them. Either move field_visibility here
> to be after ptr(ptr_arg) or move it's declaration to be after default_value.
>
Done.
>> {
>> flags=null_ptr ? 0: NOT_NULL_FLAG;
>> comment.str= (char*) "";
>> diff --git a/sql/table.h b/sql/table.h
>> index 9ecec6d636c..4e42827b003 100644
>> --- a/sql/table.h
>> +++ b/sql/table.h
>> @@ -334,6 +334,16 @@ enum enum_vcol_update_mode
>> VCOL_UPDATE_FOR_REPLACE
>> };
>>
>> +/* Field visibility enums */
>> +
>> +enum field_visible_type{
>> + NOT_INVISIBLE= 0,
>> + USER_DEFINED_INVISIBLE,
>> + // pseudo-columns (like ROWID). Can be queried explicitly in SELECT,
>> + //otherwise hidden from anything
>> + PSEUDO_COLUMN_INVISIBLE,
>
> no, don't call them pseudo-columns. In AS OF we'll have completely normal
> stored columns using this invisibility level. May be SYSTEM_INVISIBLE ?
> And a comment "automatically added by the server. Can be queried explicitly
> in SELECT, otherwise hidden from anything"
>
Done.
>> + COMPLETELY_INVISIBLE
>> +};
>>
>> /**
>> Category of table found in the table share.
>> diff --git a/sql/table.cc b/sql/table.cc
>> index 09eea1bb835..f16c591a4fe 100644
>> --- a/sql/table.cc
>> +++ b/sql/table.cc
>> @@ -1983,6 +1992,15 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
>> reg_field->field_index= i;
>> reg_field->comment=comment;
>> reg_field->vcol_info= vcol_info;
>> + if(field_properties!=NULL)
>> + {
>> + uint temp= *field_properties++;
>> + reg_field->field_visibility= static_cast<field_visible_type> (temp & 3);
>
> make it a macro, like f_packtype(), etc (see the end of field.h)
>
Done
>> + }
>> + if (reg_field->field_visibility == USER_DEFINED_INVISIBLE)
>> + status_var_increment(thd->status_var.feature_hidden_columns);
>> + if (reg_field->field_visibility == NOT_INVISIBLE)
>> + share->visible_fields++;
>> if (field_type == MYSQL_TYPE_BIT && !f_bit_as_char(pack_flag))
>> {
>> null_bits_are_used= 1;
>> diff --git a/sql/mysqld.cc b/sql/mysqld.cc
>> index 2fc5e3c27a5..b16903c9db8 100644
>> --- a/sql/mysqld.cc
>> +++ b/sql/mysqld.cc
>> @@ -8471,6 +8471,7 @@ SHOW_VAR status_vars[]= {
>> {"Feature_dynamic_columns", (char*) offsetof(STATUS_VAR, feature_dynamic_columns), SHOW_LONG_STATUS},
>> {"Feature_fulltext", (char*) offsetof(STATUS_VAR, feature_fulltext), SHOW_LONG_STATUS},
>> {"Feature_gis", (char*) offsetof(STATUS_VAR, feature_gis), SHOW_LONG_STATUS},
>> + {"Feature_hidden_columns", (char*) offsetof(STATUS_VAR, feature_hidden_columns), SHOW_LONG_STATUS},
>
> "invisibile" :)
>
Done.
>> {"Feature_locale", (char*) offsetof(STATUS_VAR, feature_locale), SHOW_LONG_STATUS},
>> {"Feature_subquery", (char*) offsetof(STATUS_VAR, feature_subquery), SHOW_LONG_STATUS},
>> {"Feature_timezone", (char*) offsetof(STATUS_VAR, feature_timezone), SHOW_LONG_STATUS},
>> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
>> index 93dd6239749..a625d3a897e 100644
>> --- a/sql/sql_base.cc
>> +++ b/sql/sql_base.cc
>> @@ -5479,6 +5479,11 @@ find_field_in_table(THD *thd, TABLE *table, const char *name, uint length,
>>
>> if (field_ptr && *field_ptr)
>> {
>> + if ((*field_ptr)->field_visibility == COMPLETELY_INVISIBLE)
>> + {
>> + DBUG_EVALUATE_IF("test_completely_invisible", {},
>> + {DBUG_RETURN((Field*) 0);});
>
> rather unconventional use of DBUG_EVALUATE_IF :)
> ok
>
>> + }
>> *cached_field_index_ptr= field_ptr - table->field;
>> field= *field_ptr;
>> }
>> @@ -7553,6 +7558,18 @@ insert_fields(THD *thd, Name_resolution_context *context, const char *db_name,
>>
>> for (; !field_iterator.end_of_fields(); field_iterator.next())
>> {
>> + /* field can be null here STODO->verify , shouldnt field be null for select * from table
>> + test case
>> + create table t1 (empnum smallint, grp int);
>> + create table t2 (empnum int, name char(5));
>> + insert into t1 values(1,1);
>> + insert into t2 values(1,'bob');
>> + create view v1 as select * from t2 inner join t1 using (empnum);
>> + select * from v1;
>> + */
>
> Thanks for the test case. The comment could better say
> /*
> field() is always NULL for views (see, e.g. Field_iterator_view or
> Field_iterator_natural_join).
> But view fields can never be invisible.
> */
> with this comment the test case is no longer needed here.
>
Done.
>> + if ((field= field_iterator.field()) &&
>> + field->field_visibility != NOT_INVISIBLE)
>> + continue;
>> Item *item;
>>
>> if (!(item= field_iterator.create_item(thd)))
>> @@ -8153,13 +8170,19 @@ fill_record(THD *thd, TABLE *table, Field **ptr, List<Item> &values,
>> List<TABLE> tbl_list;
>> bool all_fields_have_values= true;
>> Item *value;
>> - Field *field;
>> + Field *field, **f;
>> bool abort_on_warning_saved= thd->abort_on_warning;
>> uint autoinc_index= table->next_number_field
>> ? table->next_number_field->field_index
>> : ~0U;
>> + uint field_count= 0;
>> + bool need_default_value= false;
>> DBUG_ENTER("fill_record");
>> -
>> + //TODO will fields count be alwats equal to table->fields ?
>
> Nope, not always. You can see how fill_record() is called. For various
> internal temporary tables, **ptr can be table->field + something.
> For CREATE ... SELECT, it can also be table->field + something.
> In both these cases you shouldn't do anything special below.
>
> Internal temporary tables cannot have invisible fields, so that's fine.
> But CREATE ... SELECT can. Please, add the following test case:
>
> CREATE TABLE t1 (b int); INSERT t1 ...
> CREATE TABLE t2 (a int invisible) SELECT * FROM t1;
> CREATE TABLE t3 (b int, a int invisible) SELECT * FROM t1;
> CREATE TABLE t4 (b int invisible) SELECT * FROM t1;
> CREATE TABLE t2 (a int invisible) SELECT b as a FROM t1;
>
> and, may be, other variations of this theme.
> I *think* it'll mostly work fine, because CREATE ... SELECT sets
> missing fields to default values, and you do that too. So either way
> the result is the same.
>
> Anyway, I don't think you need to count fields here. Something simpler like
> if (table->s->tmp_table < INTERNAL_TMP_TABLE &&
> values.elements != table->s->fields)
> need_default_value= true;
>
> it'll set need_default_value in the case of CREATE...SELECT as above,
> but it won't loop over all fields in all other cases. And most
> fill_record() invocations are not for CREATE...SELECT. So it'll
> be almost always better than a loop.
>
//TODO
>> + for (f= ptr; f && (field= *f); f++)
>> + field_count++;
>> + if (field_count != values.elements)
>> + need_default_value= true;
>> if (!*ptr)
>> {
>> /* No fields to update, quite strange!*/
>> @@ -8177,12 +8200,16 @@ fill_record(THD *thd, TABLE *table, Field **ptr, List<Item> &values,
>> only one row.
>> */
>> table->auto_increment_field_not_null= FALSE;
>> + Name_resolution_context *context= & thd->lex->select_lex.context;
>> while ((field = *ptr++) && ! thd->is_error())
>> {
>> /* Ensure that all fields are from the same table */
>> DBUG_ASSERT(field->table == table);
>>
>> - value=v++;
>> + if (need_default_value && field->field_visibility != NOT_INVISIBLE)
>> + value = new (thd->mem_root) Item_default_value(thd,context);
>> + else
>> + value=v++;
>
> 1. Hmm, you will create a new Item for every inserted row?
> What if it's INSERT ... SELECT * FROM very_large_table?
>
> 2. please test inserts with stored procedures and prepared statements.
>
//TODO
>> if (field->field_index == autoinc_index)
>> table->auto_increment_field_not_null= TRUE;
>> if (field->vcol_info)
>> diff --git a/sql/sql_show.cc b/sql/sql_show.cc
>> index 68ded844938..a98bc0f5667 100644
>> --- a/sql/sql_show.cc
>> +++ b/sql/sql_show.cc
>> @@ -5568,6 +5579,8 @@ static int get_schema_column_record(THD *thd, TABLE_LIST *tables,
>>
>> for (; (field= *ptr) ; ptr++)
>> {
>> + if(field->field_visibility > USER_DEFINED_INVISIBLE)
>
> space before left parenthesys, please
> (here and everywhere, grep your patch to find other places)
>
Okay.
>> + continue;
>> uchar *pos;
>> char tmp[MAX_FIELD_WIDTH];
>> String type(tmp,sizeof(tmp), system_charset_info);
>> @@ -5640,13 +5653,20 @@ static int get_schema_column_record(THD *thd, TABLE_LIST *tables,
>> table->field[20]->store(STRING_WITH_LEN("ALWAYS"), cs);
>>
>> if (field->vcol_info->stored_in_db)
>> - table->field[17]->store(STRING_WITH_LEN("STORED GENERATED"), cs);
>> + buf.set(STRING_WITH_LEN("STORED GENERATED"), cs);
>> else
>> - table->field[17]->store(STRING_WITH_LEN("VIRTUAL GENERATED"), cs);
>> + buf.set(STRING_WITH_LEN("VIRTUAL GENERATED"), cs);
>> }
>> else
>> table->field[20]->store(STRING_WITH_LEN("NEVER"), cs);
>> -
>> + /*hidden can coexist with auto_increment and virtual */
>> + if(field->field_visibility == USER_DEFINED_INVISIBLE)
>> + {
>> + if (buf.length())
>> + buf.append(STRING_WITH_LEN(","));
>> + buf.append(STRING_WITH_LEN(" INVISIBLE"),cs);
>
> No space before "INVISIBLE". Instead, append STRING_WITH_LEN(", ")
>
Done.
>> + }
>> + table->field[17]->store(buf.ptr(), buf.length(), cs);
>> table->field[19]->store(field->comment.str, field->comment.length, cs);
>> if (schema_table_store_record(thd, table))
>> DBUG_RETURN(1);
>> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
>> index 4006e3aec4d..1c8398b0fd7 100644
>> --- a/sql/sql_table.cc
>> +++ b/sql/sql_table.cc
>> @@ -3233,7 +3233,37 @@ bool Column_definition::prepare_stage1_check_typelib_default()
>> return false;
>> }
>>
>> -
>> +/*
>> + This function create a hidden field.
>> + SYNOPSIS
>> + mysql_create_hidden_field()
>> + thd Thread Object
>> + field_name
>> + type_handler field data type
>> + field_visibility
>> + default value
>> + RETURN VALUE
>> + Create_field object
>> +*/
>> +static
>> +Create_field * mysql_create_hidden_field(THD *thd, const char *field_name,
>> + Type_handler *type_handler, field_visible_type field_visibility,
>> + Item* default_value)
>> +{
>> + Create_field *fld= new(thd->mem_root)Create_field();
>> + fld->set_handler(type_handler);
>> + fld->field_name.str= thd->strmake(field_name, strlen(field_name));
>> + fld->field_name.length= strlen(field_name);
>> + fld->field_visibility= field_visibility;
>> + if (default_value)
>> + {
>> + Virtual_column_info *v= new (thd->mem_root) Virtual_column_info();
>> + v->expr= default_value;
>> + v->utf8= 0;
>> + fld->default_value= v;
>> + }
>> + return fld;
>> +}
>
> This will be a compiler warning in optimized builds, because a static
> function is not used anywhere.
>
Removed static.
>> /*
>> Preparation for table creation
>>
>
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx
Thanks for the review.
--
Regards
sachin
Follow ups
References
