Hi serg or sanja can you please review this patch and let me know is the approach correct?
If it is ok, than in the commit message I explained the problems with 7 tests I couldn't figure out why they occur and if you can give me hint how to solve them.
If it is not ok, than just suggest me from which angle to look 
.
Thanks.
Hi serg, sanja_byelkin.
I have changed the patch 263a2c14c3a.
So the state of parser regarding adding new CONSTRAINT name:
- There is already opt_constraint which do needed modification. Adding it to the opt_check_constraint I must use
+ /* empty */ { $$= (Virtual_column_info*) 0; } %prec PREC_BELOW_IDENTIFIER_OPT_SPECIAL_CASEas well as %left CONSTRAINT in order to prevent shift/reduce conflicts, there is no other way.
However, when using this I got the error in key_def rule with FOREIGN KEY_SYM option and I don't know how to solve that (test that fails is innodb.foreign_key. - Note: Since field_constraints are now added while creating the table , and also visible in show create table (before only table constraints were visible), with the patch we have duplicating of IS.table_constraints and IS.field_constraints - this needs to be solved (after finishing with the parser
) . - Have looked into MySQL implementation but they have different way to declare table_element vs field_spec in our case.
- I tried to disable default reduction /empty/ in opt_check_constraint in order to prevent shift/reduce conflicts and tried to figure out is the result of rule (semantic) NULL, something like this:
@@ -5999,8 +6002,12 @@ field_spec:LEX *lex=Lex;lex->parsing_options.lookup_keywords_after_qualifier= false;$$= $<create_field>2;-- $$->check_constraint= $4;+ Virtual_column_info *vfield;+ if (!&$4)+ vfield= new (thd->mem_root) Virtual_column_info();+ else+ vfield= $4;+ $$->check_constraint= vfield;
but I got the failure in bootstrap, it doesn't work.
Is it possible to drop empty rule and test the result of rule in the action ?
So as I see this, in order to finish this - there is a way to modify all way from field_spec in order to use single CONSTRAINT token, or if it is possible to check for NULL in action or to tweak somehow with shift/reduce conflicts with some generic rule (I have tried with some patches but not successful).
The problem here is in the following part of the grammar:
opt_check_constraint: /* empty */ | opt_constraint check_constraint ;
|
|
|
field_spec: field_ident field_type_or_serial opt_check_constraint ;
|
|
|
column_def: field_spec | field_spec opt_constraint references ;
|
So when the parser sees the token CONSTRAINT after the field type, it does not know whether it's part of opt_check_constraint or already opt_constraint references
No %prec will be able to fix it. You have to rewrite the grammar to avoid the conflict.
Hi serg,
can you please review PR 2773, I have following questions (commit message):
1. Is this approach correct, last change was done in MDEV-12421?
|
|
|
2. Added constraints for `json_valid` as implicit check constraint
|
for `JSON` aka `longtext` data type.
|
Problems with this change:
|
a) Double occurence in IS tables - same table/field check constraints
|
added - don't know how to properly add constraint except
|
`lex->add_constraint`
|
b) Name of constraint for `JSON` vs `longtext` (maybe not problem, but noted in
|
`type_json.test`)
|
- in case column type is `longtext` automatic name is applied for
|
the `check`constraint
|
- in case column type is `JSON` name of constraint equal to name of
|
column.
|
|
|
3. Handling constraints with defaults (`default.test`)
|
- Check constraint is not added - don't know how to add it properly
|
|
|
4. Bug in `main.check_constraint`
|
- Subqueries are not allowed in check constraint
|
- MDEV-12421, commit 22d8bb2
|
```
|
create table t1 (a int check (@b in (select user from mysql.user)));
|
```
|
- BT:
|
```
|
sql/sql_base.cc:6204(find_field_in_table_ref(THD*, TABLE_LIST*, char const*, unsigned long, char const*, char const*, char const*, Item**, bool, bool, unsigned int*, bool, TABLE_LIST**))[0x556a34728852]
|
sql/sql_base.cc:6507(find_field_in_tables(THD*, Item_ident*, TABLE_LIST*, TABLE_LIST*, Item**, find_item_error_report_type, bool, bool))[0x556a34729580]
|
sql/item.cc:5904(Item_field::fix_fields(THD*, Item**))[0x556a34b49326]
|
sql/item.h:966(Item::fix_fields_if_needed(THD*, Item**))[0x556a346a8613]
|
sql/item.h:970(Item::fix_fields_if_needed_for_scalar(THD*, Item**))[0x556a346a864d]
|
sql/sql_base.cc:7744(setup_fields(THD*, Bounds_checked_array<Item*>, List<Item>&, enum_column_usage, List<Item>*, List<Item>*, bool))[0x556a3472c7e2]
|
sql/sql_select.cc:1330(JOIN::prepare(TABLE_LIST*, unsigned int, Item*, unsigned int, st_order*, bool, st_order*, Item*, st_order*, st_select_lex*, st_select_lex_unit*))[0x556a34812c81]
|
sql/item_subselect.cc:3804(subselect_single_select_engine::prepare(THD*))[0x556a34c106ab]
|
sql/item_subselect.cc:289(Item_subselect::fix_fields(THD*, Item**))[0x556a34c02999]
|
sql/item_subselect.cc:3466(Item_in_subselect::fix_fields(THD*, Item**))[0x556a34c0f576]
|
sql/table.cc:3288(Virtual_column_info::fix_expr(THD*))[0x556a34902fb0]
|
sql/table.cc:3458(Virtual_column_info::fix_and_check_expr(THD*, TABLE*))[0x556a34903967]
|
sql/table.cc:3590(unpack_vcol_info_from_frm(THD*, TABLE*, String*, Virtual_column_info**, bool*))[0x556a34903f3a]
|
sql/table.cc:1209(parse_vcol_defs(THD*, st_mem_root*, TABLE*, bool*, vcol_init_mode))[0x556a348fb47c]
|
sql/table.cc:3996(open_table_from_share(THD*, TABLE_SHARE*, st_mysql_const_lex_string const*, unsigned int, unsigned int, unsigned int, TABLE*, bool, List<String>*))[0x556a34905671]
|
sql/handler.cc:5313(ha_create_table(THD*, char const*, char const*, char const*, HA_CREATE_INFO*, st_mysql_const_unsigned_lex_string*))[0x556a34b2748c]
|
sql/sql_table.cc:5182(create_table_impl(THD*, st_mysql_const_lex_string const&, st_mysql_const_lex_string const&, st_mysql_const_lex_string const&, st_mysql_const_lex_string const&, char const*, DDL_options_st, HA_CREATE_INFO*, Alter_info*, int, bool*, st_key**, unsigned int*, st_mysql_const_unsigned_lex_string*))[0x556a348b8205]
|
sql/sql_table.cc:5266(mysql_create_table_no_lock(THD*, st_mysql_const_lex_string const*, st_mysql_const_lex_string const*, Table_specification_st*, Alter_info*, bool*, int, TABLE_LIST*))[0x556a348b8645]
|
sql/sql_table.cc:5415(mysql_create_table(THD*, TABLE_LIST*, Table_specification_st*, Alter_info*))[0x556a348b8bc9]
|
sql/sql_table.cc:11817(Sql_cmd_create_table_like::execute(THD*))[0x556a348cc2ae]
|
Hi,
after researching I found that MySQL is treating all field constrains as table constrains
create table t(t0 int, |
t1 int check (t1>0), |
t2 int constraint check(t2>0), |
t3 int constraint t_field check(t3>0), |
constraint tbl_c check(t2>0 and t3<0), |
constraint check(t1>0 and t2<0)); |
|
|
show create table t; |
CREATE TABLE `t` ( |
`t0` int DEFAULT NULL, |
`t1` int DEFAULT NULL, |
`t2` int DEFAULT NULL, |
`t3` int DEFAULT NULL, |
CONSTRAINT `t_chk_1` CHECK ((`t1` > 0)), |
CONSTRAINT `t_chk_2` CHECK ((`t2` > 0)), |
CONSTRAINT `t_chk_3` CHECK (((`t1` > 0) and (`t2` < 0))), |
CONSTRAINT `t_field` CHECK ((`t3` > 0)), |
CONSTRAINT `tbl_c` CHECK (((`t2` > 0) and (`t3` < 0))) |
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci |
while in MariaDB currently we have field constraints inline with columns
create table t(t0 int, |
t1 int check (t1>0), |
constraint tbl_c check(t1>0 and t1>10), |
constraint check(t1>0 and t1>20)); |
|
|
show create table t; |
t CREATE TABLE `t` ( |
`t0` int(11) DEFAULT NULL, |
`t1` int(11) DEFAULT NULL CHECK (`t1` > 0), |
CONSTRAINT `tbl_c` CHECK (`t1` > 0 and `t1` > 10), |
CONSTRAINT `CONSTRAINT_1` CHECK (`t1` > 0 and `t1` > 20) |
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 |
serg
1. Would it harm if we add field constraints the same way as we add table constraints? IMHO I like contiguous sequence of CONSTRAINTS. However this will imply changes of outputs for IS.table_constraints and IS.check_constraints - we will have double constraints tuples. Updated PR 2773 with this commit
2. What should look at the end expected output of show create table? All checks across code base are checking for alter_info.check_constraint_lists, that are table check constraints and adding new class member alter_info.field_check_constraint_lists will need lot of changes in order to check for duplicates and for pack_vcols, but I'm not sure what benefit would be from this approach.
Seems rather drastic for a fix for "cannot specify a name for a column check constraint", doesn't it?
The issue is only about the parser, why would you want to change the output?
Thanks, have updated accordingly PR 2773, if you can please review.