← Back to team overview

maria-developers team mailing list archive

review of MDEV-10177 Hidden columns

 

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

>  	/* 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.

>  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"

> +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

> +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

> +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

> +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 ...`

> +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 :(

> +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.

> +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.

> +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.

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

> +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"
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?

>  
>    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.

>  {
>    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"

> +	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)

> +    }
> +    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" :)

>    {"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.

> +      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.

> +  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.

>      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)

> +      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(", ")

> +    }
> +    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.

>  /*
>    Preparation for table creation
>  

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx


Follow ups

References