From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B594743040B for ; Tue, 11 Aug 2020 03:34:06 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3594.4.19\)) From: Roman Khabibov In-Reply-To: Date: Tue, 11 Aug 2020 03:34:00 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <3FF95208-4C8E-4094-A005-7A50415A8651@tarantool.org> References: <20200403152752.8923-1-roman.habibov@tarantool.org> <20200403152752.8923-3-roman.habibov@tarantool.org> <5CF72787-A1F0-4C48-BA8F-08F02B6960F6@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 2/2] sql: support column addition List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. > On Jul 12, 2020, at 19:45, Vladislav Shpilevoy = wrote: >=20 > Hi! Thanks for the patch! >=20 > See 33 comments below. >=20 >> commit 82c448dc66f6233faeb40dda353652c2fd5a3d29 >> Author: Roman Khabibov >> Date: Thu Jan 2 19:06:14 2020 +0300 >>=20 >> sql: support column addition >>=20 >> Enable to add column to existing space with >> statement. Column definition can be >> supplemented with the four types of constraints, , >> clauses and <[NOT] NULL>, AUTOINCREMENT. >>=20 >> Closes #2349, #3075 >>=20 >> @TarantoolBot document >> Title: Add columns to existing tables in SQL >> Now, it is possible to add columns to existing empty spaces using >> >> statement. The column definition is the same as in >> statement. >>=20 >> For example: >>=20 >> tarantool> box.execute([[CREATE TABLE test ( >> a INTEGER PRIMARY KEY >> );]]) >> --- >> - row_count: 1 >> ... >>=20 >> tarantool> box.execute([[ALTER TABLE test ADD COLUMN >> b TEXT >> NOT NULL >> DEFAULT ('a') >> COLLATE "unicode_ci" >> ;]]) >> --- >> - row_count: 0 >> ... >> --- >=20 > 1. The commit message is different from what I see on the branch. On = the > branch there is no 'Closes #2349'. What is the most actual version? >=20 > 2. The example from the doc request does not work: >=20 > tarantool> box.execute([[CREATE TABLE test (a INTEGER PRIMARY = KEY);]]) > --- > - row_count: 1 > ... >=20 > tarantool> box.execute([[ALTER TABLE test ADD COLUMN b TEXT NOT = NULL DEFAULT ('a') COLLATE "unicode_ci";]]) > --- > - null > - 'At line 1 at or near position 22: keyword ''COLUMN'' is = reserved. Please use double > quotes if ''COLUMN'' is an identifier.' > ... >=20 >> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c >> index 486b6b30d..dea047241 100644 >> --- a/extra/mkkeywordhash.c >> +++ b/extra/mkkeywordhash.c >> @@ -76,7 +76,7 @@ static Keyword aKeywordTable[] =3D { >> { "CHECK", "TK_CHECK", true }, >> { "COLLATE", "TK_COLLATE", true }, >> { "COLUMN_NAME", "TK_COLUMN_NAME", true }, >> - { "COLUMN", "TK_STANDARD", true }, >> + { "COLUMN", "TK_COLUMN", true }, >=20 > 3. This is how the same hunk looks on the branch: >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c > index 006285622..51c8cbb18 100644 > --- a/extra/mkkeywordhash.c > +++ b/extra/mkkeywordhash.c > @@ -75,11 +75,7 @@ static Keyword aKeywordTable[] =3D { > { "CAST", "TK_CAST", false }, > { "CHECK", "TK_CHECK", true }, > { "COLLATE", "TK_COLLATE", true }, > - /* gh-3075: Reserved until ALTER ADD COLUMN is implemeneted. > - * Move it back to ALTER when done. > - */ > - /* { "COLUMN", "TK_COLUMNKW", true }, */ > - { "COLUMN", "TK_STANDARD", true }, > + { "COLUMN", "TK_COLUMN", true }, > { "COMMIT", "TK_COMMIT", true }, > { "CONFLICT", "TK_CONFLICT", false }, > { "CONSTRAINT", "TK_CONSTRAINT", true }, > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 > It is very different. Why? >=20 >> diff --git a/src/box/errcode.h b/src/box/errcode.h >> index d1e4d02a9..3e94bee7a 100644 >> --- a/src/box/errcode.h >> +++ b/src/box/errcode.h >> @@ -266,6 +266,8 @@ struct errcode_record { >> /*211 */_(ER_WRONG_QUERY_ID, "Prepared statement with = id %u does not exist") \ >> /*212 */_(ER_SEQUENCE_NOT_STARTED, "Sequence '%s' = is not started") \ >> /*213 */_(ER_NO_SUCH_SESSION_SETTING, "Session setting %s = doesn't exist") \ >> + /*214 */_(ER_SQL_CANT_ADD_COLUMN_TO_VIEW, "Can't add = column '%s'. '%s' is a view") \ >> + /*215 */_(ER_SQL_CANT_ADD_AUTOINC, "Can't add = AUTOINCREMENT: the space '%s' already has one auto-incremented field") \ >=20 > 4. The same here: >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- a/src/box/errcode.h > +++ b/src/box/errcode.h > @@ -264,6 +264,7 @@ struct errcode_record { > /*209 */_(ER_SESSION_SETTING_INVALID_VALUE, "Session = setting %s expected a value of type %s") \ > /*210 */_(ER_SQL_PREPARE, "Failed to prepare SQL = statement: %s") \ > /*211 */_(ER_WRONG_QUERY_ID, "Prepared statement = with id %u does not exist") \ > + /*212 */_(ER_SQL_CANT_ADD_COLUMN_TO_VIEW, "Can't add = column '%s' to the view '%s'") \ >=20 > /* > * !IMPORTANT! Please follow instructions at start of the file > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 > =46rom this point I will review the email, not the branch. > Generally the patch still looks very raw. I hope this is mostly > because I couldn't review it properly locally. There was two branches on GitHub: romanhabibov/gh-3075-add-column and = romanhabibov/gh-3075-add-column-v2. I dropped the first so we don't get confused anymore. >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index ac42fe842..45fb90d38 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -285,48 +285,112 @@ sql_field_retrieve(Parse *parser, struct = space_def *space_def, uint32_t id) >> return field; >> } >>=20 >> -/* >> - * Add a new column to the table currently being constructed. >> +/** >> + * Make shallow copy of @a space on region. >> * >> - * The parser calls this routine once for each column declaration >> - * in a CREATE TABLE statement. sqlStartTable() gets called >> - * first to get things going. Then this routine is called for each >> - * column. >> + * Function is used to add a new column to the existing space with >> + * . Copy info about indexes and >> + * definition to create constraints appeared in the statement. >=20 > 5. I don't think I understood anything from the comment. Why is it = needed > to create a copy (I remember why, a bit, but I mostly forgot it)? To reuse the existing code. This is the best way, I think. Otherwise, = we will have to essentially rewrite the functions for creating constraints. -/* - * Add a new column to the table currently being constructed. +/** + * Make shallow copy of @a space on region. * - * The parser calls this routine once for each column declaration - * in a CREATE TABLE statement. sqlStartTable() gets called - * first to get things going. Then this routine is called for each - * column. + * Function is used to add a new column to an existing space with + * statement. Copy space def and index + * array to create constraints appeared in the statement. The + * index array copy will be modified by adding new elements to it. + * It is necessary, because the statement may contain several + * index definitions (constraints). */ +static struct space * +sql_shallow_space_copy(struct Parse *parse, struct space *space) >> */ >> +static struct space * >> +sql_shallow_space_copy(struct Parse *parse, struct space *space) >> +{ >> + assert(space->def !=3D NULL); >> + struct space *ret =3D sql_ephemeral_space_new(parse, = space->def->name); >> + if (ret =3D=3D NULL) >> + return NULL; >> + ret->index_count =3D space->index_count; >> + ret->index_id_max =3D space->index_id_max; >> + uint32_t indexes_sz =3D sizeof(struct index *) * = (ret->index_count); >> + ret->index =3D (struct index **) malloc(indexes_sz); >=20 > 6. When is this array freed? In sql_create_column_end(struct Parse *parse) + /* + * Clean up array allocated in sql_shallow_space_copy(). + */ + free(space->index); >> + if (ret->index =3D=3D NULL) { >> + diag_set(OutOfMemory, indexes_sz, "realloc", = "ret->index"); >> + return NULL; >> + } >> + for (uint32_t i =3D 0; i < ret->index_count; i++) >> + ret->index[i] =3D space->index[i]; >=20 > 7. What is the problem to make memcpy() instead of the cycle? No problem. See below. >> + memcpy(ret->def, space->def, sizeof(struct space_def)); >> + ret->def->opts.is_temporary =3D true; >> + ret->def->opts.is_ephemeral =3D true; >> + uint32_t fields_size =3D sizeof(struct field_def) * = ret->def->field_count; >> + ret->def->fields =3D region_alloc(&parse->region, fields_size); >=20 > 8. Use region_alloc_array. Otherwise it will crash in ASAN build. It > should be visible in CI. Ok. >> + if (ret->def->fields =3D=3D NULL) { >> + diag_set(OutOfMemory, fields_size, "region_alloc", >> + "ret->def->fields"); >=20 > 9. index array leaks here. Fixed. +static struct space * +sql_shallow_space_copy(struct Parse *parse, struct space *space) +{ + assert(space->def !=3D NULL); + struct space *ret =3D sql_ephemeral_space_new(parse, = space->def->name); + if (ret =3D=3D NULL) + return NULL; + ret->index_count =3D space->index_count; + ret->index_id_max =3D space->index_id_max; + uint32_t indexes_sz =3D sizeof(struct index *) * = (ret->index_count); + ret->index =3D (struct index **) malloc(indexes_sz); + if (ret->index =3D=3D NULL) { + diag_set(OutOfMemory, indexes_sz, "realloc", = "ret->index"); + return NULL; + } + memcpy(ret->index, space->index, indexes_sz); + memcpy(ret->def, space->def, sizeof(struct space_def)); + ret->def->opts.is_temporary =3D true; + ret->def->opts.is_ephemeral =3D true; + if (ret->def->field_count !=3D 0) { + uint32_t fields_size =3D 0; + ret->def->fields =3D + region_alloc_array(&parse->region, + typeof(struct field_def), + ret->def->field_count, = &fields_size); + if (ret->def->fields =3D=3D NULL) { + diag_set(OutOfMemory, fields_size, = "region_alloc", + "ret->def->fields"); + free(ret->index); + return NULL; + } + memcpy(ret->def->fields, space->def->fields, = fields_size); + } + + return ret; +} >> + return NULL; >> + } >> + memcpy(ret->def->fields, space->def->fields, fields_size); >> + >> + return ret; >> +} >> + >> void >> -sqlAddColumn(Parse * pParse, Token * pName, struct type_def = *type_def) >> +sql_create_column_start(struct Parse *parse) >> { >> - assert(type_def !=3D NULL); >> - char *z; >> - sql *db =3D pParse->db; >> - if (pParse->create_table_def.new_space =3D=3D NULL) >> - return; >> - struct space_def *def =3D = pParse->create_table_def.new_space->def; >> + struct create_column_def *create_column_def =3D = &parse->create_column_def; >> + struct alter_entity_def *alter_entity_def =3D >> + &create_column_def->base.base; >> + assert(alter_entity_def->entity_type =3D=3D ENTITY_TYPE_COLUMN); >> + assert(alter_entity_def->alter_action =3D=3D = ALTER_ACTION_CREATE); >> + struct space *space =3D parse->create_table_def.new_space; >> + bool is_alter =3D space =3D=3D NULL; >> + struct sql *db =3D parse->db; >> + if (is_alter) { >> + const char *space_name =3D >> + alter_entity_def->entity_name->a[0].zName; >> + space =3D space_by_name(space_name); >> + if (space =3D=3D NULL) { >> + diag_set(ClientError, ER_NO_SUCH_SPACE, = space_name); >> + goto tnt_error; >> + } >> + space =3D sql_shallow_space_copy(parse, space); >> + if (space =3D=3D NULL) >> + goto tnt_error; >> + } >> + create_column_def->space =3D space; >> + struct space_def *def =3D space->def; >> + assert(def->opts.is_ephemeral); >>=20 >> #if SQL_MAX_COLUMN >> if ((int)def->field_count + 1 > db->aLimit[SQL_LIMIT_COLUMN]) { >> diag_set(ClientError, ER_SQL_COLUMN_COUNT_MAX, = def->name, >> def->field_count + 1, = db->aLimit[SQL_LIMIT_COLUMN]); >> - pParse->is_aborted =3D true; >> - return; >> + goto tnt_error; >=20 > 10. Was there a leak before your patch? Because of not > called sqlSrcListDelete(db, alter_entity_def->entity_name); No, because alter_entity_def is a part of create_column_def. create_column_def didn=E2=80=99t exist before my patch. >> } >> #endif >> + >> + struct region *region =3D &parse->region; >> + struct Token *name =3D &create_column_def->base.name; >> + char *column_name =3D >> + sql_normalized_name_region_new(region, name->z, = name->n); >> + if (column_name =3D=3D NULL) >> + goto tnt_error; >> + >> + if (parse->create_table_def.new_space =3D=3D NULL && = def->opts.is_view) { >=20 > 11. You have is_alter flag for that. Yes. Fixed. >> + diag_set(ClientError, ER_SQL_CANT_ADD_COLUMN_TO_VIEW, >> + column_name, def->name); >> + goto tnt_error; >> + } >> + >> /* >> - * As sql_field_retrieve will allocate memory on region >> - * ensure that def is also temporal and would be dropped. >> + * Format can be set in Lua, then exact_field_count can be >> + * zero, but field_count is not. >> */ >> - assert(def->opts.is_ephemeral); >> - if (sql_field_retrieve(pParse, def, def->field_count) =3D=3D = NULL) >> - return; >> - struct region *region =3D &pParse->region; >> - z =3D sql_normalized_name_region_new(region, pName->z, = pName->n); >> - if (z =3D=3D NULL) { >> - pParse->is_aborted =3D true; >> + if (def->exact_field_count =3D=3D 0) >> + def->exact_field_count =3D def->field_count; >> + if (sql_field_retrieve(parse, def, def->field_count) =3D=3D = NULL) >> return; >> + >> + for (uint32_t i =3D 0; i < def->field_count; i++) { >> + if (strcmp(column_name, def->fields[i].name) =3D=3D 0) { >> + diag_set(ClientError, = ER_SPACE_FIELD_IS_DUPLICATE, >> + column_name); >> + goto tnt_error; >> + } >=20 > 12. I remember this code was deliberately removed, because the same = check is > already done in box. Why did you return it? Sorry, removed. >> } >> struct field_def *column_def =3D &def->fields[def->field_count]; >> - memcpy(column_def, &field_def_default, = sizeof(field_def_default)); >> - column_def->name =3D z; >> + memcpy(column_def, &field_def_default, sizeof(struct = field_def)); >=20 > 13. Unnecessary change of memcpy(). Yes. Returned old version. +sql_create_column_start(struct Parse *parse) { - assert(type_def !=3D NULL); - char *z; - sql *db =3D pParse->db; - if (pParse->create_table_def.new_space =3D=3D NULL) - return; - struct space_def *def =3D = pParse->create_table_def.new_space->def; + struct create_column_def *create_column_def =3D = &parse->create_column_def; + struct alter_entity_def *alter_entity_def =3D + &create_column_def->base.base; + assert(alter_entity_def->entity_type =3D=3D ENTITY_TYPE_COLUMN); + assert(alter_entity_def->alter_action =3D=3D = ALTER_ACTION_CREATE); + struct space *space =3D parse->create_table_def.new_space; + bool is_alter =3D space =3D=3D NULL; + struct sql *db =3D parse->db; + if (is_alter) { + const char *space_name =3D + alter_entity_def->entity_name->a[0].zName; + space =3D space_by_name(space_name); + if (space =3D=3D NULL) { + diag_set(ClientError, ER_NO_SUCH_SPACE, = space_name); + goto tnt_error; + } + space =3D sql_shallow_space_copy(parse, space); + if (space =3D=3D NULL) + goto tnt_error; + } + create_column_def->space =3D space; + struct space_def *def =3D space->def; + assert(def->opts.is_ephemeral); =20 #if SQL_MAX_COLUMN if ((int)def->field_count + 1 > db->aLimit[SQL_LIMIT_COLUMN]) { diag_set(ClientError, ER_SQL_COLUMN_COUNT_MAX, = def->name, def->field_count + 1, = db->aLimit[SQL_LIMIT_COLUMN]); - pParse->is_aborted =3D true; - return; + goto tnt_error; } #endif + + struct region *region =3D &parse->region; + struct Token *name =3D &create_column_def->base.name; + char *column_name =3D + sql_normalized_name_region_new(region, name->z, = name->n); + if (column_name =3D=3D NULL) + goto tnt_error; + + if (is_alter && def->opts.is_view) { + diag_set(ClientError, ER_SQL_CANT_ADD_COLUMN_TO_VIEW, + column_name, def->name); + goto tnt_error; + } + /* - * As sql_field_retrieve will allocate memory on region - * ensure that def is also temporal and would be dropped. + * Format can be set in Lua, then exact_field_count can be + * zero, but field_count is not. */ - assert(def->opts.is_ephemeral); - if (sql_field_retrieve(pParse, def, def->field_count) =3D=3D = NULL) + if (def->exact_field_count =3D=3D 0) + def->exact_field_count =3D def->field_count; + if (sql_field_retrieve(parse, def, def->field_count) =3D=3D = NULL) return; - struct region *region =3D &pParse->region; - z =3D sql_normalized_name_region_new(region, pName->z, = pName->n); - if (z =3D=3D NULL) { - pParse->is_aborted =3D true; - return; - } + struct field_def *column_def =3D &def->fields[def->field_count]; memcpy(column_def, &field_def_default, = sizeof(field_def_default)); - column_def->name =3D z; + column_def->name =3D column_name; /* * Marker ON_CONFLICT_ACTION_DEFAULT is used to detect * attempts to define NULL multiple time or to detect @@ -334,18 +399,86 @@ sqlAddColumn(Parse * pParse, Token * pName, struct = type_def *type_def) */ column_def->nullable_action =3D ON_CONFLICT_ACTION_DEFAULT; column_def->is_nullable =3D true; - column_def->type =3D type_def->type; + column_def->type =3D create_column_def->type_def->type; def->field_count++; + + sqlSrcListDelete(db, alter_entity_def->entity_name); + return; +tnt_error: + parse->is_aborted =3D true; + sqlSrcListDelete(db, alter_entity_def->entity_name); +} + >> + column_def->name =3D column_name; >> /* >> * Marker ON_CONFLICT_ACTION_DEFAULT is used to detect >> * attempts to define NULL multiple time or to detect >> @@ -334,19 +398,100 @@ sqlAddColumn(Parse * pParse, Token * pName, = struct type_def *type_def) >> */ >> column_def->nullable_action =3D ON_CONFLICT_ACTION_DEFAULT; >> column_def->is_nullable =3D true; >> - column_def->type =3D type_def->type; >> + column_def->type =3D create_column_def->type_def->type; >> def->field_count++; >> + >> + sqlSrcListDelete(db, alter_entity_def->entity_name); >> + return; >> +tnt_error: >> + parse->is_aborted =3D true; >> + sqlSrcListDelete(db, alter_entity_def->entity_name); >> +} >> + >> +static void >> +sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id); >> + >> +void >> +sql_create_column_end(struct Parse *parse) >> +{ >> + struct create_column_def *create_column_def =3D = &parse->create_column_def; >> + struct space *space =3D parse->create_table_def.new_space; >> + bool is_alter =3D space =3D=3D NULL; >> + space =3D create_column_def->space; >> + struct space_def *def =3D space->def; >> + if (is_alter) { >> + struct field_def *field =3D = &def->fields[def->field_count - 1]; >> + if (field->nullable_action =3D=3D = ON_CONFLICT_ACTION_DEFAULT) { >> + if (create_column_def->is_pk) { >> + field->nullable_action =3D >> + ON_CONFLICT_ACTION_ABORT; >> + field->is_nullable =3D false; >> + } else { >> + field->nullable_action =3D >> + ON_CONFLICT_ACTION_NONE; >> + field->is_nullable =3D true; >> + } >> + } >> + /* >> + * Encode the format array and emit code to update = _space. >> + */ >> + uint32_t table_stmt_sz =3D 0; >> + struct region *region =3D &parse->region; >> + char *table_stmt =3D sql_encode_table(region, def, >> + &table_stmt_sz); >> + char *raw =3D sqlDbMallocRaw(parse->db, table_stmt_sz); >> + if (table_stmt =3D=3D NULL || raw =3D=3D NULL) { >> + parse->is_aborted =3D true; >> + return; >> + } >> + memcpy(raw, table_stmt, table_stmt_sz); >> + >> + struct Vdbe *v =3D sqlGetVdbe(parse); >> + assert(v !=3D NULL); >> + >> + struct space *system_space =3D = space_by_id(BOX_SPACE_ID); >> + assert(system_space !=3D NULL); >> + int cursor =3D parse->nTab++; >> + vdbe_emit_open_cursor(parse, cursor, 0, system_space); >> + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); >> + >> + int key_reg =3D ++parse->nMem; >> + sqlVdbeAddOp2(v, OP_Integer, def->id, key_reg); >> + int addr =3D sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, = key_reg, 1); >> + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT); >> + sqlVdbeJumpHere(v, addr); >> + >> + int tuple_reg =3D sqlGetTempRange(parse, = box_space_field_MAX + 1); >> + for (int i =3D 0; i < box_space_field_MAX - 1; ++i) >> + sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg = + i); >> + sqlVdbeAddOp1(v, OP_Close, cursor); >> + >> + sqlVdbeAddOp2(v, OP_Integer, def->field_count, tuple_reg = + 4); >> + sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, tuple_reg + 6, >> + SQL_SUBTYPE_MSGPACK, raw, P4_DYNAMIC); >> + sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, = box_space_field_MAX, >> + tuple_reg + box_space_field_MAX); >> + sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + = box_space_field_MAX, >> + 0, 0, (char *) system_space, P4_SPACEPTR); >> + sql_vdbe_create_constraints(parse, key_reg); >> + } >> + >> + memset(create_column_def, 0, sizeof(struct create_column_def)); >> + create_column_def->nullable_action =3D = ON_CONFLICT_ACTION_DEFAULT; >=20 > 14. Why do you touch column creation def after its usage? Removed. +void +sql_create_column_end(struct Parse *parse) +{ + struct space *space =3D parse->create_column_def.space; + assert(space !=3D NULL); + struct space_def *def =3D space->def; + struct field_def *field =3D &def->fields[def->field_count - 1]; + if (field->nullable_action =3D=3D ON_CONFLICT_ACTION_DEFAULT) { + field->nullable_action =3D ON_CONFLICT_ACTION_NONE; + field->is_nullable =3D true; + } + /* + * Encode the format array and emit code to update _space. + */ + uint32_t table_stmt_sz =3D 0; + struct region *region =3D &parse->region; + char *table_stmt =3D sql_encode_table(region, def, = &table_stmt_sz); + char *raw =3D sqlDbMallocRaw(parse->db, table_stmt_sz); + if (table_stmt =3D=3D NULL || raw =3D=3D NULL) { + parse->is_aborted =3D true; + return; + } + memcpy(raw, table_stmt, table_stmt_sz); + + struct Vdbe *v =3D sqlGetVdbe(parse); + assert(v !=3D NULL); + + struct space *system_space =3D space_by_id(BOX_SPACE_ID); + assert(system_space !=3D NULL); + int cursor =3D parse->nTab++; + vdbe_emit_open_cursor(parse, cursor, 0, system_space); + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); + + int key_reg =3D ++parse->nMem; + sqlVdbeAddOp2(v, OP_Integer, def->id, key_reg); + int addr =3D sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, = 1); + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT); + sqlVdbeJumpHere(v, addr); + + int tuple_reg =3D sqlGetTempRange(parse, box_space_field_MAX + = 1); + for (int i =3D 0; i < box_space_field_MAX - 1; ++i) + sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i); + sqlVdbeAddOp1(v, OP_Close, cursor); + + sqlVdbeAddOp2(v, OP_Integer, def->field_count, tuple_reg + 4); + sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, tuple_reg + 6, + SQL_SUBTYPE_MSGPACK, raw, P4_DYNAMIC); + sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, box_space_field_MAX, + tuple_reg + box_space_field_MAX); + sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + box_space_field_MAX, = 0, 0, + (char *) system_space, P4_SPACEPTR); + sql_vdbe_create_constraints(parse, key_reg); + + /* + * Clean up array allocated in sql_shallow_space_copy(). + */ + free(space->index); } >> } >>=20 >> void >> sql_column_add_nullable_action(struct Parse *parser, >> enum on_conflict_action nullable_action) >> { >> - struct space *space =3D parser->create_table_def.new_space; >> - if (space =3D=3D NULL || NEVER(space->def->field_count < 1)) >> + struct space_def *def =3D NULL; >> + struct field_def *field =3D NULL; >> + struct space *space =3D parser->create_column_def.space; >> + assert (space !=3D NULL); >=20 > 15. Please, don't put whitespace after macro/function name and its = arguments. Sorry. Fixed. >> + def =3D space->def; >> + if (NEVER(def->field_count < 1)) >> return; >> - struct space_def *def =3D space->def; >> - struct field_def *field =3D &def->fields[def->field_count - 1]; >> + field =3D &def->fields[def->field_count - 1]; >=20 > 16. Why did you do that change about struct field_def? It seems not to > be needed. Yes. void sql_column_add_nullable_action(struct Parse *parser, enum on_conflict_action nullable_action) { - struct space *space =3D parser->create_table_def.new_space; - if (space =3D=3D NULL || NEVER(space->def->field_count < 1)) + assert(parser->create_column_def.space !=3D NULL); + struct space_def *def =3D parser->create_column_def.space->def; + if (NEVER(def->field_count < 1)) return; - struct space_def *def =3D space->def; struct field_def *field =3D &def->fields[def->field_count - 1]; if (field->nullable_action !=3D ON_CONFLICT_ACTION_DEFAULT && nullable_action !=3D field->nullable_action) { @@ -364,51 +497,42 @@ sql_column_add_nullable_action(struct Parse = *parser, } >=20 >> if (field->nullable_action !=3D ON_CONFLICT_ACTION_DEFAULT && >> nullable_action !=3D field->nullable_action) { >> /* Prevent defining nullable_action many times. */ >> @@ -364,51 +509,46 @@ sql_column_add_nullable_action(struct Parse = *parser, >> } >>=20 >> /* >> - * The expression is the default value for the most recently added = column >> - * of the table currently under construction. >> + * The expression is the default value for the most recently added >> + * column. >> * >> * Default value expressions must be constant. Raise an exception if = this >> * is not the case. >> * >> * This routine is called by the parser while in the middle of >> - * parsing a CREATE TABLE statement. >> + * parsing a or a >> + * statement. >> */ >> void >> sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan) >> { >> sql *db =3D pParse->db; >> - struct space *p =3D pParse->create_table_def.new_space; >> - if (p !=3D NULL) { >> - assert(p->def->opts.is_ephemeral); >> - struct space_def *def =3D p->def; >> - if (!sqlExprIsConstantOrFunction >> - (pSpan->pExpr, db->init.busy)) { >> - const char *column_name =3D >> - def->fields[def->field_count - 1].name; >> - diag_set(ClientError, ER_CREATE_SPACE, = def->name, >> - tt_sprintf("default value of column = '%s' is "\ >> - "not constant", = column_name)); >> + struct space_def *def =3D NULL; >> + struct field_def *field =3D NULL; >> + struct space *space =3D pParse->create_column_def.space; >> + assert (space !=3D NULL); >> + def =3D space->def; >=20 > 17. Why can't you declare and initialize space_def in one line? The = same > for field. Fixed. void sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan) { sql *db =3D pParse->db; - struct space *p =3D pParse->create_table_def.new_space; - if (p !=3D NULL) { - assert(p->def->opts.is_ephemeral); - struct space_def *def =3D p->def; - if (!sqlExprIsConstantOrFunction - (pSpan->pExpr, db->init.busy)) { - const char *column_name =3D - def->fields[def->field_count - 1].name; - diag_set(ClientError, ER_CREATE_SPACE, = def->name, - tt_sprintf("default value of column = '%s' is "\ - "not constant", = column_name)); + assert(pParse->create_column_def.space !=3D NULL); + struct space_def *def =3D pParse->create_column_def.space->def; + struct field_def *field =3D &def->fields[def->field_count - 1]; + if (!sqlExprIsConstantOrFunction(pSpan->pExpr, db->init.busy)) { + diag_set(ClientError, ER_CREATE_SPACE, def->name, + tt_sprintf("default value of column '%s' is not = " + "constant", field->name)); + pParse->is_aborted =3D true; + } else { + struct region *region =3D &pParse->region; + uint32_t default_length =3D (int)(pSpan->zEnd - = pSpan->zStart); + field->default_value =3D region_alloc(region, = default_length + 1); + if (field->default_value =3D=3D NULL) { + diag_set(OutOfMemory, default_length + 1, + "region_alloc", = "field->default_value"); pParse->is_aborted =3D true; - } else { - assert(def !=3D NULL); - struct field_def *field =3D - &def->fields[def->field_count - 1]; - struct region *region =3D &pParse->region; - uint32_t default_length =3D (int)(pSpan->zEnd - = pSpan->zStart); - field->default_value =3D region_alloc(region, - = default_length + 1); - if (field->default_value =3D=3D NULL) { - diag_set(OutOfMemory, default_length + = 1, - "region_alloc", - "field->default_value"); - pParse->is_aborted =3D true; - return; - } - strncpy(field->default_value, pSpan->zStart, - default_length); - field->default_value[default_length] =3D '\0'; + goto add_default_value_exit; } + strncpy(field->default_value, pSpan->zStart, = default_length); + field->default_value[default_length] =3D '\0'; } +add_default_value_exit: sql_expr_delete(db, pSpan->pExpr, false); } >> + field =3D &def->fields[def->field_count - 1]; >> + if (!sqlExprIsConstantOrFunction(pSpan->pExpr, db->init.busy)) { >> + diag_set(ClientError, ER_CREATE_SPACE, def->name, >> + tt_sprintf("default value of column '%s' is not = " >> + "constant", field->name)); >> + pParse->is_aborted =3D true; >> + } else { >> + assert(def !=3D NULL); >> + struct region *region =3D &pParse->region; >> + uint32_t default_length =3D (int)(pSpan->zEnd - = pSpan->zStart); >> + field->default_value =3D region_alloc(region, = default_length + 1); >> + if (field->default_value =3D=3D NULL) { >> + diag_set(OutOfMemory, default_length + 1, >> + "region_alloc", = "field->default_value"); >> pParse->is_aborted =3D true; >> - } else { >> - assert(def !=3D NULL); >> - struct field_def *field =3D >> - &def->fields[def->field_count - 1]; >> - struct region *region =3D &pParse->region; >> - uint32_t default_length =3D (int)(pSpan->zEnd - = pSpan->zStart); >> - field->default_value =3D region_alloc(region, >> - = default_length + 1); >> - if (field->default_value =3D=3D NULL) { >> - diag_set(OutOfMemory, default_length + = 1, >> - "region_alloc", >> - "field->default_value"); >> - pParse->is_aborted =3D true; >> - return; >> - } >> - strncpy(field->default_value, pSpan->zStart, >> - default_length); >> - field->default_value[default_length] =3D '\0'; >> + goto add_default_value_exit; >> } >> + strncpy(field->default_value, pSpan->zStart, = default_length); >> + field->default_value[default_length] =3D '\0'; >> } >> +add_default_value_exit: >> sql_expr_delete(db, pSpan->pExpr, false); >> } >>=20 >> @@ -447,6 +587,8 @@ sqlAddPrimaryKey(struct Parse *pParse) >> int nTerm; >> struct ExprList *pList =3D pParse->create_index_def.cols; >> struct space *space =3D pParse->create_table_def.new_space; >> + if (space =3D=3D NULL) >> + space =3D pParse->create_column_def.space; >> if (space =3D=3D NULL) >> goto primary_key_exit; >> if (sql_space_primary_key(space) !=3D NULL) { >> @@ -574,8 +716,10 @@ sql_create_check_contraint(struct Parse *parser) >> (struct alter_entity_def *) create_ck_def; >> assert(alter_def->entity_type =3D=3D ENTITY_TYPE_CK); >> (void) alter_def; >> - struct space *space =3D parser->create_table_def.new_space; >> - bool is_alter =3D space =3D=3D NULL; >> + struct space *space =3D parser->create_column_def.space; >> + if (space =3D=3D NULL) >> + space =3D parser->create_table_def.new_space; >> + bool is_alter_add_constr =3D space =3D=3D NULL; >=20 > 18. Why is it called 'is_alter' in other functions, but = 'is_alter_add_constr' > here? If I added =E2=80=98is_alter_add_constr=E2=80=99, it means that we = should distinguish current statements: or = (maybe ). It=E2=80=99s important for branching within constraint creation = functions (index, ck, fk). >>=20 >> /* Prepare payload for ck constraint definition. */ >> struct region *region =3D &parser->region; >> @@ -589,9 +733,18 @@ sql_create_check_contraint(struct Parse *parser) >> return; >> } >> } else { >> - assert(! is_alter); >> - uint32_t ck_idx =3D = ++parser->create_table_def.check_count; >> - name =3D tt_sprintf("ck_unnamed_%s_%d", = space->def->name, ck_idx); >> + assert(!is_alter_add_constr); >> + uint32_t idx =3D ++parser->check_count; >> + if (parser->create_table_def.new_space =3D=3D NULL) { >> + struct space *original_space =3D >> + space_by_name(space->def->name); >> + assert(original_space !=3D NULL); >> + struct rlist *checks =3D = &original_space->ck_constraint; >> + struct ck_constraint *ck; >> + rlist_foreach_entry(ck, checks, link) >> + idx++; >> + } >> + name =3D tt_sprintf("ck_unnamed_%s_%d", = space->def->name, idx); >> } >> size_t name_len =3D strlen(name); >>=20 >> @@ -634,9 +787,9 @@ sql_create_check_contraint(struct Parse *parser) >> trim_space_snprintf(ck_def->expr_str, expr_str, expr_str_len); >> memcpy(ck_def->name, name, name_len); >> ck_def->name[name_len] =3D '\0'; >> - if (is_alter) { >> + if (is_alter_add_constr) { >> const char *space_name =3D = alter_def->entity_name->a[0].zName; >> - struct space *space =3D space_by_name(space_name); >> + space =3D space_by_name(space_name); >=20 > 19. Why this change? Dropped. >=20 >> if (space =3D=3D NULL) { >> diag_set(ClientError, ER_NO_SUCH_SPACE, = space_name); >> parser->is_aborted =3D true; >> @@ -652,8 +805,7 @@ sql_create_check_contraint(struct Parse *parser) >> sqlVdbeCountChanges(v); >> sqlVdbeChangeP5(v, OPFLAG_NCHANGE); >> } else { >> - rlist_add_entry(&parser->create_table_def.new_check, = ck_parse, >> - link); >> + rlist_add_entry(&parser->checks, ck_parse, link); >> } >> } >>=20 >> @@ -664,18 +816,19 @@ sql_create_check_contraint(struct Parse = *parser) >> void >> sqlAddCollateType(Parse * pParse, Token * pToken) >> { >> - struct space *space =3D pParse->create_table_def.new_space; >> - if (space =3D=3D NULL) >> - return; >> + struct space *space =3D pParse->create_column_def.space; >> + uint32_t *coll_id =3D NULL; >> + assert(space !=3D NULL); >> uint32_t i =3D space->def->field_count - 1; >> + coll_id =3D &space->def->fields[i].coll_id; >=20 > 20. Why not declare and initialize coll_id in one line? Done. void sqlAddCollateType(Parse * pParse, Token * pToken) { - struct space *space =3D pParse->create_table_def.new_space; - if (space =3D=3D NULL) - return; + struct space *space =3D pParse->create_column_def.space; + assert(space !=3D NULL); uint32_t i =3D space->def->field_count - 1; sql *db =3D pParse->db; char *coll_name =3D sql_name_from_token(db, pToken); >> sql *db =3D pParse->db; >> char *coll_name =3D sql_name_from_token(db, pToken); >> if (coll_name =3D=3D NULL) { >> pParse->is_aborted =3D true; >> return; >> } >> - uint32_t *coll_id =3D &space->def->fields[i].coll_id; >> - if (sql_get_coll_seq(pParse, coll_name, coll_id) !=3D NULL) { >> + if (sql_get_coll_seq(pParse, coll_name, coll_id) !=3D NULL && >> + space !=3D NULL) { >=20 > 21. You have assert(space !=3D NULL) above. Why do you need this check = again? Removed. >> /* If the column is declared as " PRIMARY KEY = COLLATE ", >> * then an index may have been created on this column = before the >> * collation type was added. Correct this if it is the = case. >> @@ -692,6 +845,7 @@ sqlAddCollateType(Parse * pParse, Token * pToken) >> sqlDbFree(db, coll_name); >> } >>=20 >> + >=20 > 22. ??? Oops. Removed. >> struct coll * >> sql_column_collation(struct space_def *def, uint32_t column, uint32_t = *coll_id) >> { >> @@ -1147,6 +1304,105 @@ resolve_link(struct Parse *parse_context, = const struct space_def *def, >> return -1; >> } >>=20 >> +/** >> + * Emit code to create sequences, indexes, check and foreign >> + * constraints appeared in or >> + * statement. >> + */ >> +static void >> +sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id) >=20 > 23. Such huge code movements better do in a separate commit. I can't > tell whether you changed anything here, or just moved the code. Done. See commit in the new version of the patch set. >> +{ >> + assert(reg_space_id !=3D 0); >> + struct space *space =3D parse->create_table_def.new_space; >> + bool is_alter =3D space =3D=3D NULL; >> + uint32_t i =3D 0; >> + if (is_alter) { >> + space =3D parse->create_column_def.space; >> + i =3D space_by_name(space->def->name)->index_count; >> + } >> + assert(space !=3D NULL); >> + for (; i < space->index_count; ++i) { >> + struct index *idx =3D space->index[i]; >> + vdbe_emit_create_index(parse, space->def, idx->def, >> + reg_space_id, idx->def->iid); >> + } >> + >> + /* >> + * Check to see if we need to create an _sequence table >> + * for keeping track of autoincrement keys. >> + */ >> + if (parse->has_autoinc) { >> + /* Do an insertion into _sequence. */ >> + int reg_seq_id =3D ++parse->nMem; >> + struct Vdbe *v =3D sqlGetVdbe(parse); >> + assert(v !=3D NULL); >> + sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id); >> + int reg_seq_record =3D >> + emitNewSysSequenceRecord(parse, reg_seq_id, >> + space->def->name); >> + const char *error_msg =3D >> + = tt_sprintf(tnt_errcode_desc(ER_SQL_CANT_ADD_AUTOINC), >> + space->def->name); >> + if (vdbe_emit_halt_with_presence_test(parse, >> + BOX_SEQUENCE_ID, = 2, >> + reg_seq_record + = 3, 1, >> + = ER_SQL_CANT_ADD_AUTOINC, >> + error_msg, false, >> + OP_NoConflict) !=3D = 0) >> + return; >> + sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, = reg_seq_record); >> + /* Do an insertion into _space_sequence. */ >> + int reg_space_seq_record =3D >> + emitNewSysSpaceSequenceRecord(parse, = reg_space_id, >> + reg_seq_id); >> + sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID, >> + reg_space_seq_record); >> + } >> + >> + /* Code creation of FK constraints, if any. */ >> + struct fk_constraint_parse *fk_parse; >> + rlist_foreach_entry(fk_parse, &parse->fkeys, link) { >> + struct fk_constraint_def *fk_def =3D fk_parse->fk_def; >> + if (fk_parse->selfref_cols !=3D NULL) { >> + struct ExprList *cols =3D = fk_parse->selfref_cols; >> + for (uint32_t i =3D 0; i < fk_def->field_count; = ++i) { >> + if (resolve_link(parse, space->def, >> + cols->a[i].zName, >> + = &fk_def->links[i].parent_field, >> + fk_def->name) !=3D 0) >> + return; >> + } >> + fk_def->parent_id =3D reg_space_id; >> + } else if (fk_parse->is_self_referenced) { >> + struct key_def *pk_key_def =3D >> + = sql_space_primary_key(space)->def->key_def; >> + if (pk_key_def->part_count !=3D = fk_def->field_count) { >> + diag_set(ClientError, = ER_CREATE_FK_CONSTRAINT, >> + fk_def->name, "number of = columns in "\ >> + "foreign key does not match the = "\ >> + "number of columns in the = primary "\ >> + "index of referenced table"); >> + parse->is_aborted =3D true; >> + return; >> + } >> + for (uint32_t i =3D 0; i < fk_def->field_count; = ++i) { >> + fk_def->links[i].parent_field =3D >> + pk_key_def->parts[i].fieldno; >> + } >> + fk_def->parent_id =3D reg_space_id; >> + } >> + fk_def->child_id =3D reg_space_id; >> + vdbe_emit_fk_constraint_create(parse, fk_def, = space->def->name); >> + } >> + >> + /* Code creation of CK constraints, if any. */ >> + struct ck_constraint_parse *ck_parse; >> + rlist_foreach_entry(ck_parse, &parse->checks, link) { >> + vdbe_emit_ck_constraint_create(parse, ck_parse->ck_def, >> + reg_space_id, = space->def->name); >> + } >> +} >> + >> /* >> * This routine is called to report the final ")" that terminates >> * a CREATE TABLE statement. >> @@ -1213,73 +1469,7 @@ sqlEndTable(struct Parse *pParse) >>=20 >> int reg_space_id =3D getNewSpaceId(pParse); >> vdbe_emit_space_create(pParse, reg_space_id, name_reg, = new_space); >> - for (uint32_t i =3D 0; i < new_space->index_count; ++i) { >> - struct index *idx =3D new_space->index[i]; >> - vdbe_emit_create_index(pParse, new_space->def, idx->def, >> - reg_space_id, idx->def->iid); >> - } >> - >> - /* >> - * Check to see if we need to create an _sequence table >> - * for keeping track of autoincrement keys. >> - */ >> - if (pParse->create_table_def.has_autoinc) { >> - assert(reg_space_id !=3D 0); >> - /* Do an insertion into _sequence. */ >> - int reg_seq_id =3D ++pParse->nMem; >> - sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id); >> - int reg_seq_record =3D >> - emitNewSysSequenceRecord(pParse, reg_seq_id, >> - new_space->def->name); >> - sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, = reg_seq_record); >> - /* Do an insertion into _space_sequence. */ >> - int reg_space_seq_record =3D >> - emitNewSysSpaceSequenceRecord(pParse, = reg_space_id, >> - reg_seq_id); >> - sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID, >> - reg_space_seq_record); >> - } >> - /* Code creation of FK constraints, if any. */ >> - struct fk_constraint_parse *fk_parse; >> - rlist_foreach_entry(fk_parse, = &pParse->create_table_def.new_fkey, >> - link) { >> - struct fk_constraint_def *fk_def =3D fk_parse->fk_def; >> - if (fk_parse->selfref_cols !=3D NULL) { >> - struct ExprList *cols =3D = fk_parse->selfref_cols; >> - for (uint32_t i =3D 0; i < fk_def->field_count; = ++i) { >> - if (resolve_link(pParse, new_space->def, >> - cols->a[i].zName, >> - = &fk_def->links[i].parent_field, >> - fk_def->name) !=3D 0) >> - return; >> - } >> - fk_def->parent_id =3D reg_space_id; >> - } else if (fk_parse->is_self_referenced) { >> - struct index *pk =3D = sql_space_primary_key(new_space); >> - if (pk->def->key_def->part_count !=3D = fk_def->field_count) { >> - diag_set(ClientError, = ER_CREATE_FK_CONSTRAINT, >> - fk_def->name, "number of = columns in "\ >> - "foreign key does not match the = "\ >> - "number of columns in the = primary "\ >> - "index of referenced table"); >> - pParse->is_aborted =3D true; >> - return; >> - } >> - for (uint32_t i =3D 0; i < fk_def->field_count; = ++i) { >> - fk_def->links[i].parent_field =3D >> - = pk->def->key_def->parts[i].fieldno; >> - } >> - fk_def->parent_id =3D reg_space_id; >> - } >> - fk_def->child_id =3D reg_space_id; >> - vdbe_emit_fk_constraint_create(pParse, fk_def, = space_name_copy); >> - } >> - struct ck_constraint_parse *ck_parse; >> - rlist_foreach_entry(ck_parse, = &pParse->create_table_def.new_check, >> - link) { >> - vdbe_emit_ck_constraint_create(pParse, ck_parse->ck_def, >> - reg_space_id, = space_name_copy); >> - } >> + sql_vdbe_create_constraints(pParse, reg_space_id); >> } >> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y >> index 995875566..5904414a3 100644 >> --- a/src/box/sql/parse.y >> +++ b/src/box/sql/parse.y >> @@ -314,6 +320,7 @@ ccons ::=3D cconsname(N) PRIMARY KEY = sortorder(Z). { >> create_index_def_init(&pParse->create_index_def, NULL, &N, NULL, >> SQL_INDEX_TYPE_CONSTRAINT_PK, Z, false); >> sqlAddPrimaryKey(pParse); >> + pParse->create_column_def.is_pk =3D true; >=20 > 24. Why can't this be done in sqlAddPrimaryKey(pParse);? What if this > is CREATE TABLE, and there are many columns in PK? I removed this field from create_column_def. It turned out to be = unnecessary. >> } >> ccons ::=3D cconsname(N) UNIQUE. { >> create_index_def_init(&pParse->create_index_def, NULL, &N, NULL, >> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c >> index a5a258805..b31bac437 100644 >> --- a/src/box/sql/prepare.c >> +++ b/src/box/sql/prepare.c >> @@ -197,9 +197,13 @@ sql_parser_create(struct Parse *parser, struct = sql *db, uint32_t sql_flags) >> { >> memset(parser, 0, sizeof(struct Parse)); >> parser->db =3D db; >> + parser->create_column_def.nullable_action =3D = ON_CONFLICT_ACTION_DEFAULT; >=20 > 25. Why isn't there a proper constructor for create_column_def = structure? Dropped. >> parser->sql_flags =3D sql_flags; >> parser->line_count =3D 1; >> parser->line_pos =3D 1; >> + rlist_create(&parser->fkeys); >> + rlist_create(&parser->checks); > 26. Why did you merge these fields into struct Parse? They were = specially > moved out of here for the purpose of some kind of isolation, into = parse_def.h > structures. Because these fields are convenient to use right here, so as not to = duplicate the code in build.c. These fields are used by two statements: CREATE = TABLE and ALTER TABLE ADD COLUMN. >> + parser->has_autoinc =3D false; >> region_create(&parser->region, &cord()->slabc); >> } >>=20 >> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h >> index aa6a470f8..3143ec521 100644 >> --- a/src/box/sql/sqlInt.h >> +++ b/src/box/sql/sqlInt.h >> @@ -2249,12 +2249,26 @@ struct Parse { >> struct enable_entity_def enable_entity_def; >> }; >> /** >> - * Table def is not part of union since information >> - * being held must survive till the end of parsing of >> - * whole CREATE TABLE statement (to pass it to >> - * sqlEndTable() function). >> + * Table def or column def is not part of union since >> + * information being held must survive till the end of >> + * parsing of whole or >> + * statement (to pass it to >> + * sqlEndTable() sql_create_column_end() function). >> */ >> struct create_table_def create_table_def; >> + struct create_column_def create_column_def; >> + /** >> + * FK and CK constraints appeared in a or >> + * a statement. >> + */ >> + struct rlist fkeys; >> + struct rlist checks; >> + uint32_t fkey_count; >> + uint32_t check_count; >> + /** True, if column to be created has . */ >> + bool has_autoinc; >=20 > 27. What column? This is struct Parse, it is not a column. I know, but I haven't come up with anything better. diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index fa87e7bd2..32142a871 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2251,22 +2251,26 @@ struct Parse { struct enable_entity_def enable_entity_def; }; /** - * Table def is not part of union since information - * being held must survive till the end of parsing of - * whole CREATE TABLE statement (to pass it to - * sqlEndTable() function). + * Table def or column def is not part of union since + * information being held must survive till the end of + * parsing of whole or + * statement (to pass it to + * sqlEndTable() sql_create_column_end() function). */ struct create_table_def create_table_def; + struct create_column_def create_column_def; /* - * FK and CK constraints appeared in a . + * FK and CK constraints appeared in a or + * an statement. */ struct rlist fkeys; struct rlist checks; uint32_t fkey_count; uint32_t check_count; /* - * True, if column within a statement to be - * created has . + * True, if column in a or an + * statement to be created has + * . */ bool has_autoinc; /* Id of field with . */ >> + /** Id of field with . */ >> + uint32_t autoinc_fieldno; >> bool initiateTTrans; /* Initiate Tarantool transaction */ >> /** If set - do not emit byte code at all, just parse. */ >> bool parse_only; >> @@ -2844,15 +2858,31 @@ struct space *sqlResultSetOfSelect(Parse *, = Select *); >>=20 >> struct space * >> sqlStartTable(Parse *, Token *); >> -void sqlAddColumn(Parse *, Token *, struct type_def *); >> + >> +/** >> + * Add new field to the format of ephemeral space in >> + * create_table_def. If it is create shallow copy of >> + * the existing space and add field to its format. >> + */ >> +void >> +sql_create_column_start(struct Parse *parse); >> + >> +/** >> + * Nullify create_column_def. If it is emit code to >> + * update entry in _space and code to create constraints (entries >> + * in _index, _ck_constraint, _fk_constraint) described with this >> + * column. >> + */ >> +void >> +sql_create_column_end(struct Parse *parse); >>=20 >> /** >> * This routine is called by the parser while in the middle of >> - * parsing a CREATE TABLE statement. A "NOT NULL" constraint has >> - * been seen on a column. This routine sets the is_nullable flag >> - * on the column currently under construction. >> - * If nullable_action has been already set, this function raises >> - * an error. >> + * parsing a or a >> + * statement. A "NOT NULL" constraint has been seen on a column. >> + * This routine sets the is_nullable flag on the column currently >> + * under construction. If nullable_action has been already set, >> + * this function raises an error. >> * >> * @param parser SQL Parser object. >> * @param nullable_action on_conflict_action value. >> diff --git a/test/box/error.result b/test/box/error.result >> index 2196fa541..eb55f9cdf 100644 >> --- a/test/box/error.result >> +++ b/test/box/error.result >> @@ -432,6 +432,8 @@ t; >> | 211: box.error.WRONG_QUERY_ID >> | 212: box.error.SEQUENCE_NOT_STARTED >> | 213: box.error.NO_SUCH_SESSION_SETTING >> + | 214: box.error.SQL_CANT_ADD_COLUMN_TO_VIEW >> + | 215: box.error.SQL_CANT_ADD_AUTOINC >> | ... >>=20 >> test_run:cmd("setopt delimiter ''"); >> diff --git a/test/sql/add-column.result b/test/sql/add-column.result >> new file mode 100644 >> index 000000000..06c95facb >> --- /dev/null >> +++ b/test/sql/add-column.result >=20 > 28. You may want to look at the original SQLite tests. They probably > have some. I looked. I took everything that is relevant for Tarantool from there. >> @@ -0,0 +1,276 @@ >> +-- test-run result file version 2 >> +test_run =3D require('test_run').new() >> + | --- >> + | ... >> +engine =3D test_run:get_cfg('engine') >> + | --- >> + | ... >> +_ =3D box.space._session_settings:update('sql_default_engine', = {{'=3D', 2, engine}}) >> + | --- >> + | ... >> + >> +-- >> +-- gh-3075: Check statement. >> +-- >> +box.execute('CREATE TABLE t1 (a INTEGER PRIMARY KEY);') >> + | --- >> + | - row_count: 1 >> + | ... >> +-- >> +-- COLUMN keyword is optional. Check it here, but omit it below. >> +-- >> +box.execute('ALTER TABLE t1 ADD COLUMN b INTEGER;') >> + | --- >> + | - row_count: 0 >> + | ... >> + >> +-- >> +-- Can't add column to a view. >> +-- >> +box.execute('CREATE VIEW v AS SELECT * FROM t1;') >> + | --- >> + | - row_count: 1 >> + | ... >> +box.execute('ALTER TABLE v ADD b INTEGER;') >> + | --- >> + | - null >> + | - Can't add column 'B'. 'V' is a view >> + | ... >> +box.execute('DROP VIEW v;') >> + | --- >> + | - row_count: 1 >> + | ... >> + >> +-- >> +-- Check column constraints typing and work. >> +-- >> +box.execute('CREATE TABLE t2 (a INTEGER CONSTRAINT pk_constr PRIMARY = KEY);') >> + | --- >> + | - row_count: 1 >> + | ... >> +box.execute('ALTER TABLE t2 DROP CONSTRAINT pk_constr') >> + | --- >> + | - row_count: 1 >> + | ... >> +test_run:cmd("setopt delimiter ';'"); >> + | --- >> + | - true >> + | ... >> +box.execute([[ALTER TABLE t2 ADD b INTEGER CONSTRAINT pk_constr = PRIMARY KEY >> + CHECK (b > 0) >> + REFERENCES t1(a) >> + CONSTRAINT u_constr UNIQUE]]) >> +test_run:cmd("setopt delimiter ''"); >> + | --- >> + | ... >> +box.execute('INSERT INTO t1 VALUES (1, 1);') >> + | --- >> + | - row_count: 1 >> + | ... >> +box.execute('INSERT INTO t2 VALUES (1, 1);') >> + | --- >> + | - row_count: 1 >> + | ... >> +box.execute('INSERT INTO t2 VALUES (1, 1);') >> + | --- >> + | - null >> + | - Duplicate key exists in unique index 'PK_CONSTR' in space 'T2' >> + | ... >> + >> +box.execute('INSERT INTO t1 VALUES (0, 1);') >> + | --- >> + | - row_count: 1 >> + | ... >> +box.execute('INSERT INTO t2 VALUES (2, 0);') >> + | --- >> + | - null >> + | - 'Check constraint failed ''ck_unnamed_T2_1'': b > 0' >> + | ... >> + >> +box.execute('INSERT INTO t2 VALUES (2, 3);') >> + | --- >> + | - null >> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint = failed' >> + | ... >> + >> +box.execute('DROP TABLE t2;') >> + | --- >> + | - row_count: 1 >> + | ... >> + >> +-- >> +-- Check self-referenced FK creation. >> +-- >> +box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY);') >> + | --- >> + | - row_count: 1 >> + | ... >> +box.execute('ALTER TABLE t2 ADD b INT REFERENCES t1') >> + | --- >> + | - row_count: 0 >=20 > 29. Worth checking if it works. Not just created and dropped right = away. See the new version of tests in the diff. >> + | ... >> + >> +box.execute('DROP TABLE t2;') >> + | --- >> + | - row_count: 1 >> + | ... >> + >> +-- >> +-- Check AUTOINCREMENT work. >> +-- >> +box.execute("CREATE TABLE t2(a INTEGER CONSTRAINT pk PRIMARY KEY);") >> + | --- >> + | - row_count: 1 >> + | ... >> +box.execute("ALTER TABLE t2 DROP CONSTRAINT pk;") >> + | --- >> + | - row_count: 1 >> + | ... >> +box.execute("ALTER TABLE t2 ADD b INTEGER PRIMARY KEY = AUTOINCREMENT;") >> + | --- >> + | - row_count: 0 >=20 > 30. The same. >=20 >> + | ... >> +box.execute("ALTER TABLE t2 ADD c INTEGER AUTOINCREMENT;") >> + | --- >> + | - null >> + | - 'Can''t add AUTOINCREMENT: the space ''T2'' already has one = auto-incremented field' >> + | ... >> + >> +box.execute('DROP TABLE t2;') >> + | --- >> + | - row_count: 1 >> + | ... >> + >> +-- >> +-- Check clauses after column typing and work. >> +-- >> +box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY, b INTEGER);') >> + | --- >> + | - row_count: 1 >> + | ... >> +test_run:cmd("setopt delimiter ';'"); >> + | --- >> + | - true >> + | ... >> +box.execute([[ALTER TABLE t2 ADD c TEXT NOT NULL DEFAULT ('a') >> + COLLATE "unicode_ci";]]); >> + | --- >> + | - row_count: 0 >> + | ... >> +test_run:cmd("setopt delimiter ''"); >> + | --- >> + | - true >> + | ... >> +box.execute('INSERT INTO t2(a, b) VALUES (1, 1);') >> + | --- >> + | - row_count: 1 >> + | ... >> +box.execute('SELECT * FROM t2;') >> + | --- >> + | - metadata: >> + | - name: A >> + | type: integer >> + | - name: B >> + | type: integer >> + | - name: C >> + | type: string >> + | rows: >> + | - [1, 1, 'a'] >> + | ... >> +box.execute('INSERT INTO t2 VALUES (2, 2, NULL);') >> + | --- >> + | - null >> + | - 'Failed to execute SQL statement: NOT NULL constraint failed: = T2.C' >> + | ... >> +box.execute('SELECT * FROM t2 WHERE c LIKE \'A\';') >> + | --- >> + | - metadata: >> + | - name: A >> + | type: integer >> + | - name: B >> + | type: integer >> + | - name: C >> + | type: string >> + | rows: >> + | - [1, 1, 'a'] >> + | ... >> + >> +-- >> +-- Try to add to a non-empty space a [non-]nullable field. >> +-- >> +box.execute('ALTER TABLE t2 ADD d INTEGER;') >> + | --- >> + | - null >> + | - Tuple field count 3 does not match space field count 4 >> + | ... >> +box.execute('ALTER TABLE t2 ADD d TEXT NOT NULL'); >> + | --- >> + | - null >> + | - Tuple field count 3 does not match space field count 4 >> + | ... >> +box.execute('ALTER TABLE t2 ADD e TEXT NULL'); >> + | --- >> + | - null >> + | - Tuple field count 3 does not match space field count 4 >> + | ... >> + >> +-- >> +-- Add to a space with no-SQL adjusted or without format. >> +-- >> +_ =3D box.schema.space.create('WITHOUT') >> + | --- >> + | ... >> +box.execute("ALTER TABLE WITHOUT ADD a INTEGER;") >=20 > 31. No need to caps table names in SQL. Fixed. > 32. Need to check if it actually worked. The same below. >=20 >> + | --- >> + | - row_count: 0 >> + | ... >> +box.execute("DROP TABLE WITHOUT;") >> + | --- >> + | - row_count: 1 >> + | ... >> + >> +s =3D box.schema.space.create('NOSQL') >> + | --- >> + | ... >> +s:format{{name =3D 'A', type =3D 'unsigned'}} >> + | --- >> + | ... >> +box.execute("ALTER TABLE NOSQL ADD b INTEGER") >> + | --- >> + | - row_count: 0 >> + | ... >> + >> +box.execute('DROP TABLE t2;') >> + | --- >> + | - row_count: 1 >> + | ... >> +-- >> +-- Add multiple columns inside a transaction. >> +-- >> +box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY)') >> + | --- >> + | - row_count: 1 >> + | ... >> +box.begin() = \ >> +box.execute('ALTER TABLE t2 ADD b INT') = \ >> +box.execute('ALTER TABLE t2 ADD c INT') = \ >> +box.commit() >> + | --- >> + | ... >> + >> +box.execute('INSERT INTO t2 VALUES (1, 1, 1)') >> + | --- >> + | - row_count: 1 >> + | ... >> +box.execute('SELECT * FROM t2;') >> + | --- >> + | - metadata: >> + | - name: A >> + | type: integer >> + | - name: B >> + | type: integer >> + | - name: C >> + | type: integer >> + | rows: >> + | - [1, 1, 1] >=20 > 33. What if I add a column with UNIQUE constraint? Added. P.S. You can see that I added tests in checks.test.lua and = foreign-keys.test.lua. I tested the naming of these constraints. This naming is (maybe isn=E2=80=99= t) temporary, because it allows such a case: box.execute("CREATE TABLE t1 (a INT PRIMARY KEY);") --- - row_count: 1 ... box.execute("CREATE TABLE check_naming (a INT PRIMARY KEY REFERENCES = t1(a));") --- - row_count: 1 ... box.execute("ALTER TABLE check_naming ADD b INT REFERENCES t1(a);") --- - row_count: 0 ... box.execute("ALTER TABLE check_naming DROP CONSTRAINT = \"fk_unnamed_CHECK_NAMING_1\"") --- - row_count: 1 ... box.execute("ALTER TABLE check_naming ADD c INT REFERENCES t1(a);") --- - null - FOREIGN KEY constraint 'fk_unnamed_CHECK_NAMING_2' already exists in = space 'CHECK_NAMING' ... To avoid such collisions, we need to use a hash table of names. Let=E2=80=99= s put it aside for later. commit d0fb61f0283f64f1253307d38d1c8cfdea4c933b Author: Roman Khabibov Date: Thu Jan 2 19:06:14 2020 +0300 sql: support column addition =20 Enable to add column to existing space with statement. Column definition can be supplemented with the four types of constraints, , clauses and <[NOT] NULL>, AUTOINCREMENT. =20 Closes #2349, #3075 =20 @TarantoolBot document Title: Add columns to existing tables in SQL Now, it is possible to add columns to existing empty spaces using statement. The column definition is the same as in statement. =20 For example: =20 ``` tarantool> box.execute("CREATE TABLE test (a INTEGER PRIMARY KEY)") --- - row_count: 1 ... =20 tarantool> box.execute([[ALTER TABLE test ADD COLUMN b TEXT > CHECK = (LENGTH(b) > 1) > NOT NULL > DEFAULT ('aa') > COLLATE = "unicode_ci" > ]]) --- - row_count: 0 ... ``` diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c index 486b6b30d..dea047241 100644 --- a/extra/mkkeywordhash.c +++ b/extra/mkkeywordhash.c @@ -76,7 +76,7 @@ static Keyword aKeywordTable[] =3D { { "CHECK", "TK_CHECK", true }, { "COLLATE", "TK_COLLATE", true }, { "COLUMN_NAME", "TK_COLUMN_NAME", true }, - { "COLUMN", "TK_STANDARD", true }, + { "COLUMN", "TK_COLUMN", true }, { "COMMIT", "TK_COMMIT", true }, { "CONFLICT", "TK_CONFLICT", false }, { "CONSTRAINT", "TK_CONSTRAINT", true }, diff --git a/src/box/errcode.h b/src/box/errcode.h index 3c21375f5..cbcffb3a8 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -271,6 +271,8 @@ struct errcode_record { /*216 */_(ER_SYNC_QUORUM_TIMEOUT, "Quorum collection for = a synchronous transaction is timed out") \ /*217 */_(ER_SYNC_ROLLBACK, "A rollback for a = synchronous transaction is received") \ /*218 */_(ER_TUPLE_METADATA_IS_TOO_BIG, "Can't create tuple: = metadata size %u is too big") \ + /*219 */_(ER_SQL_CANT_ADD_COLUMN_TO_VIEW, "Can't add = column '%s'. '%s' is a view") \ + /*220 */_(ER_SQL_CANT_ADD_AUTOINC, "Can't add = AUTOINCREMENT: the space '%s' already has one auto incremented field") \ =20 /* * !IMPORTANT! Please follow instructions at start of the file diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 9013bc86f..6f3d2747d 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -285,48 +285,113 @@ sql_field_retrieve(Parse *parser, struct = space_def *space_def, uint32_t id) return field; } =20 -/* - * Add a new column to the table currently being constructed. +/** + * Make shallow copy of @a space on region. * - * The parser calls this routine once for each column declaration - * in a CREATE TABLE statement. sqlStartTable() gets called - * first to get things going. Then this routine is called for each - * column. + * Function is used to add a new column to an existing space with + * statement. Copy space def and index + * array to create constraints appeared in the statement. The + * index array copy will be modified by adding new elements to it. + * It is necessary, because the statement may contain several + * index definitions (constraints). */ +static struct space * +sql_shallow_space_copy(struct Parse *parse, struct space *space) +{ + assert(space->def !=3D NULL); + struct space *ret =3D sql_ephemeral_space_new(parse, = space->def->name); + if (ret =3D=3D NULL) + return NULL; + ret->index_count =3D space->index_count; + ret->index_id_max =3D space->index_id_max; + uint32_t indexes_sz =3D sizeof(struct index *) * = (ret->index_count); + ret->index =3D (struct index **) malloc(indexes_sz); + if (ret->index =3D=3D NULL) { + diag_set(OutOfMemory, indexes_sz, "realloc", = "ret->index"); + return NULL; + } + memcpy(ret->index, space->index, indexes_sz); + memcpy(ret->def, space->def, sizeof(struct space_def)); + ret->def->opts.is_temporary =3D true; + ret->def->opts.is_ephemeral =3D true; + if (ret->def->field_count !=3D 0) { + uint32_t fields_size =3D 0; + ret->def->fields =3D + region_alloc_array(&parse->region, + typeof(struct field_def), + ret->def->field_count, = &fields_size); + if (ret->def->fields =3D=3D NULL) { + diag_set(OutOfMemory, fields_size, = "region_alloc", + "ret->def->fields"); + free(ret->index); + return NULL; + } + memcpy(ret->def->fields, space->def->fields, = fields_size); + } + + return ret; +} + void -sqlAddColumn(Parse * pParse, Token * pName, struct type_def *type_def) +sql_create_column_start(struct Parse *parse) { - assert(type_def !=3D NULL); - char *z; - sql *db =3D pParse->db; - if (pParse->create_table_def.new_space =3D=3D NULL) - return; - struct space_def *def =3D = pParse->create_table_def.new_space->def; + struct create_column_def *create_column_def =3D = &parse->create_column_def; + struct alter_entity_def *alter_entity_def =3D + &create_column_def->base.base; + assert(alter_entity_def->entity_type =3D=3D ENTITY_TYPE_COLUMN); + assert(alter_entity_def->alter_action =3D=3D = ALTER_ACTION_CREATE); + struct space *space =3D parse->create_table_def.new_space; + bool is_alter =3D space =3D=3D NULL; + struct sql *db =3D parse->db; + if (is_alter) { + const char *space_name =3D + alter_entity_def->entity_name->a[0].zName; + space =3D space_by_name(space_name); + if (space =3D=3D NULL) { + diag_set(ClientError, ER_NO_SUCH_SPACE, = space_name); + goto tnt_error; + } + space =3D sql_shallow_space_copy(parse, space); + if (space =3D=3D NULL) + goto tnt_error; + } + create_column_def->space =3D space; + struct space_def *def =3D space->def; + assert(def->opts.is_ephemeral); =20 #if SQL_MAX_COLUMN if ((int)def->field_count + 1 > db->aLimit[SQL_LIMIT_COLUMN]) { diag_set(ClientError, ER_SQL_COLUMN_COUNT_MAX, = def->name, def->field_count + 1, = db->aLimit[SQL_LIMIT_COLUMN]); - pParse->is_aborted =3D true; - return; + goto tnt_error; } #endif + + struct region *region =3D &parse->region; + struct Token *name =3D &create_column_def->base.name; + char *column_name =3D + sql_normalized_name_region_new(region, name->z, = name->n); + if (column_name =3D=3D NULL) + goto tnt_error; + + if (is_alter && def->opts.is_view) { + diag_set(ClientError, ER_SQL_CANT_ADD_COLUMN_TO_VIEW, + column_name, def->name); + goto tnt_error; + } + /* - * As sql_field_retrieve will allocate memory on region - * ensure that def is also temporal and would be dropped. + * Format can be set in Lua, then exact_field_count can be + * zero, but field_count is not. */ - assert(def->opts.is_ephemeral); - if (sql_field_retrieve(pParse, def, def->field_count) =3D=3D = NULL) + if (def->exact_field_count =3D=3D 0) + def->exact_field_count =3D def->field_count; + if (sql_field_retrieve(parse, def, def->field_count) =3D=3D = NULL) return; - struct region *region =3D &pParse->region; - z =3D sql_normalized_name_region_new(region, pName->z, = pName->n); - if (z =3D=3D NULL) { - pParse->is_aborted =3D true; - return; - } + struct field_def *column_def =3D &def->fields[def->field_count]; memcpy(column_def, &field_def_default, = sizeof(field_def_default)); - column_def->name =3D z; + column_def->name =3D column_name; /* * Marker ON_CONFLICT_ACTION_DEFAULT is used to detect * attempts to define NULL multiple time or to detect @@ -334,18 +399,86 @@ sqlAddColumn(Parse * pParse, Token * pName, struct = type_def *type_def) */ column_def->nullable_action =3D ON_CONFLICT_ACTION_DEFAULT; column_def->is_nullable =3D true; - column_def->type =3D type_def->type; + column_def->type =3D create_column_def->type_def->type; def->field_count++; + + sqlSrcListDelete(db, alter_entity_def->entity_name); + return; +tnt_error: + parse->is_aborted =3D true; + sqlSrcListDelete(db, alter_entity_def->entity_name); +} + +static void +sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id); + +void +sql_create_column_end(struct Parse *parse) +{ + struct space *space =3D parse->create_column_def.space; + assert(space !=3D NULL); + struct space_def *def =3D space->def; + struct field_def *field =3D &def->fields[def->field_count - 1]; + if (field->nullable_action =3D=3D ON_CONFLICT_ACTION_DEFAULT) { + field->nullable_action =3D ON_CONFLICT_ACTION_NONE; + field->is_nullable =3D true; + } + /* + * Encode the format array and emit code to update _space. + */ + uint32_t table_stmt_sz =3D 0; + struct region *region =3D &parse->region; + char *table_stmt =3D sql_encode_table(region, def, = &table_stmt_sz); + char *raw =3D sqlDbMallocRaw(parse->db, table_stmt_sz); + if (table_stmt =3D=3D NULL || raw =3D=3D NULL) { + parse->is_aborted =3D true; + return; + } + memcpy(raw, table_stmt, table_stmt_sz); + + struct Vdbe *v =3D sqlGetVdbe(parse); + assert(v !=3D NULL); + + struct space *system_space =3D space_by_id(BOX_SPACE_ID); + assert(system_space !=3D NULL); + int cursor =3D parse->nTab++; + vdbe_emit_open_cursor(parse, cursor, 0, system_space); + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); + + int key_reg =3D ++parse->nMem; + sqlVdbeAddOp2(v, OP_Integer, def->id, key_reg); + int addr =3D sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, = 1); + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT); + sqlVdbeJumpHere(v, addr); + + int tuple_reg =3D sqlGetTempRange(parse, box_space_field_MAX + = 1); + for (int i =3D 0; i < box_space_field_MAX - 1; ++i) + sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i); + sqlVdbeAddOp1(v, OP_Close, cursor); + + sqlVdbeAddOp2(v, OP_Integer, def->field_count, tuple_reg + 4); + sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, tuple_reg + 6, + SQL_SUBTYPE_MSGPACK, raw, P4_DYNAMIC); + sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, box_space_field_MAX, + tuple_reg + box_space_field_MAX); + sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + box_space_field_MAX, = 0, 0, + (char *) system_space, P4_SPACEPTR); + sql_vdbe_create_constraints(parse, key_reg); + + /* + * Clean up array allocated in sql_shallow_space_copy(). + */ + free(space->index); } =20 void sql_column_add_nullable_action(struct Parse *parser, enum on_conflict_action nullable_action) { - struct space *space =3D parser->create_table_def.new_space; - if (space =3D=3D NULL || NEVER(space->def->field_count < 1)) + assert(parser->create_column_def.space !=3D NULL); + struct space_def *def =3D parser->create_column_def.space->def; + if (NEVER(def->field_count < 1)) return; - struct space_def *def =3D space->def; struct field_def *field =3D &def->fields[def->field_count - 1]; if (field->nullable_action !=3D ON_CONFLICT_ACTION_DEFAULT && nullable_action !=3D field->nullable_action) { @@ -364,51 +497,42 @@ sql_column_add_nullable_action(struct Parse = *parser, } =20 /* - * The expression is the default value for the most recently added = column - * of the table currently under construction. + * The expression is the default value for the most recently added + * column. * * Default value expressions must be constant. Raise an exception if = this * is not the case. * * This routine is called by the parser while in the middle of - * parsing a CREATE TABLE statement. + * parsing a or a + * statement. */ void sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan) { sql *db =3D pParse->db; - struct space *p =3D pParse->create_table_def.new_space; - if (p !=3D NULL) { - assert(p->def->opts.is_ephemeral); - struct space_def *def =3D p->def; - if (!sqlExprIsConstantOrFunction - (pSpan->pExpr, db->init.busy)) { - const char *column_name =3D - def->fields[def->field_count - 1].name; - diag_set(ClientError, ER_CREATE_SPACE, = def->name, - tt_sprintf("default value of column = '%s' is "\ - "not constant", = column_name)); + assert(pParse->create_column_def.space !=3D NULL); + struct space_def *def =3D pParse->create_column_def.space->def; + struct field_def *field =3D &def->fields[def->field_count - 1]; + if (!sqlExprIsConstantOrFunction(pSpan->pExpr, db->init.busy)) { + diag_set(ClientError, ER_CREATE_SPACE, def->name, + tt_sprintf("default value of column '%s' is not = " + "constant", field->name)); + pParse->is_aborted =3D true; + } else { + struct region *region =3D &pParse->region; + uint32_t default_length =3D (int)(pSpan->zEnd - = pSpan->zStart); + field->default_value =3D region_alloc(region, = default_length + 1); + if (field->default_value =3D=3D NULL) { + diag_set(OutOfMemory, default_length + 1, + "region_alloc", = "field->default_value"); pParse->is_aborted =3D true; - } else { - assert(def !=3D NULL); - struct field_def *field =3D - &def->fields[def->field_count - 1]; - struct region *region =3D &pParse->region; - uint32_t default_length =3D (int)(pSpan->zEnd - = pSpan->zStart); - field->default_value =3D region_alloc(region, - = default_length + 1); - if (field->default_value =3D=3D NULL) { - diag_set(OutOfMemory, default_length + = 1, - "region_alloc", - "field->default_value"); - pParse->is_aborted =3D true; - return; - } - strncpy(field->default_value, pSpan->zStart, - default_length); - field->default_value[default_length] =3D '\0'; + goto add_default_value_exit; } + strncpy(field->default_value, pSpan->zStart, = default_length); + field->default_value[default_length] =3D '\0'; } +add_default_value_exit: sql_expr_delete(db, pSpan->pExpr, false); } =20 @@ -447,6 +571,8 @@ sqlAddPrimaryKey(struct Parse *pParse) int nTerm; struct ExprList *pList =3D pParse->create_index_def.cols; struct space *space =3D pParse->create_table_def.new_space; + if (space =3D=3D NULL) + space =3D pParse->create_column_def.space; if (space =3D=3D NULL) goto primary_key_exit; if (sql_space_primary_key(space) !=3D NULL) { @@ -574,8 +700,10 @@ sql_create_check_contraint(struct Parse *parser) (struct alter_entity_def *) create_ck_def; assert(alter_def->entity_type =3D=3D ENTITY_TYPE_CK); (void) alter_def; - struct space *space =3D parser->create_table_def.new_space; - bool is_alter =3D space =3D=3D NULL; + struct space *space =3D parser->create_column_def.space; + if (space =3D=3D NULL) + space =3D parser->create_table_def.new_space; + bool is_alter_add_constr =3D space =3D=3D NULL; =20 /* Prepare payload for ck constraint definition. */ struct region *region =3D &parser->region; @@ -589,9 +717,23 @@ sql_create_check_contraint(struct Parse *parser) return; } } else { - assert(! is_alter); - uint32_t ck_idx =3D ++parser->check_count; - name =3D tt_sprintf("ck_unnamed_%s_%d", = space->def->name, ck_idx); + assert(!is_alter_add_constr); + uint32_t idx =3D ++parser->check_count; + /* + * If it is we should + * count the existing CHECK constraints in the + * space and form a name based on this. + */ + if (parser->create_table_def.new_space =3D=3D NULL) { + struct space *original_space =3D + space_by_name(space->def->name); + assert(original_space !=3D NULL); + struct rlist *checks =3D = &original_space->ck_constraint; + struct ck_constraint *ck; + rlist_foreach_entry(ck, checks, link) + idx++; + } + name =3D tt_sprintf("ck_unnamed_%s_%d", = space->def->name, idx); } size_t name_len =3D strlen(name); =20 @@ -634,7 +776,7 @@ sql_create_check_contraint(struct Parse *parser) trim_space_snprintf(ck_def->expr_str, expr_str, expr_str_len); memcpy(ck_def->name, name, name_len); ck_def->name[name_len] =3D '\0'; - if (is_alter) { + if (is_alter_add_constr) { const char *space_name =3D = alter_def->entity_name->a[0].zName; struct space *space =3D space_by_name(space_name); if (space =3D=3D NULL) { @@ -663,9 +805,8 @@ sql_create_check_contraint(struct Parse *parser) void sqlAddCollateType(Parse * pParse, Token * pToken) { - struct space *space =3D pParse->create_table_def.new_space; - if (space =3D=3D NULL) - return; + struct space *space =3D pParse->create_column_def.space; + assert(space !=3D NULL); uint32_t i =3D space->def->field_count - 1; sql *db =3D pParse->db; char *coll_name =3D sql_name_from_token(db, pToken); @@ -704,8 +845,7 @@ sql_column_collation(struct space_def *def, uint32_t = column, uint32_t *coll_id) * * In cases mentioned above collation is fetched by id. */ - if (space =3D=3D NULL) { - assert(def->opts.is_ephemeral); + if (def->opts.is_ephemeral) { assert(column < (uint32_t)def->field_count); *coll_id =3D def->fields[column].coll_id; struct coll_id *collation =3D coll_by_id(*coll_id); @@ -794,7 +934,8 @@ vdbe_emit_create_index(struct Parse *parse, struct = space_def *def, memcpy(raw, index_parts, index_parts_sz); index_parts =3D raw; =20 - if (parse->create_table_def.new_space !=3D NULL) { + if (parse->create_table_def.new_space !=3D NULL || + parse->create_column_def.space !=3D NULL) { sqlVdbeAddOp2(v, OP_SCopy, space_id_reg, entry_reg); sqlVdbeAddOp2(v, OP_Integer, idx_def->iid, entry_reg + = 1); } else { @@ -1032,18 +1173,21 @@ vdbe_emit_fk_constraint_create(struct Parse = *parse_context, P4_DYNAMIC); /* * In case we are adding FK constraints during execution - * of statement, we don't have child - * id, but we know register where it will be stored. + * of or + * statement, we don't have child id, but we know register + * where it will be stored. */ - if (parse_context->create_table_def.new_space !=3D NULL) { + bool is_alter_add_constr =3D + parse_context->create_table_def.new_space =3D=3D NULL && + parse_context->create_column_def.space =3D=3D NULL; + if (!is_alter_add_constr) { sqlVdbeAddOp2(vdbe, OP_SCopy, fk->child_id, constr_tuple_reg + 1); } else { sqlVdbeAddOp2(vdbe, OP_Integer, fk->child_id, constr_tuple_reg + 1); } - if (parse_context->create_table_def.new_space !=3D NULL && - fk_constraint_is_self_referenced(fk)) { + if (!is_alter_add_constr && = fk_constraint_is_self_referenced(fk)) { sqlVdbeAddOp2(vdbe, OP_SCopy, fk->parent_id, constr_tuple_reg + 2); } else { @@ -1107,7 +1251,7 @@ vdbe_emit_fk_constraint_create(struct Parse = *parse_context, constr_tuple_reg + 9); sqlVdbeAddOp2(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, constr_tuple_reg + 9); - if (parse_context->create_table_def.new_space =3D=3D NULL) { + if (is_alter_add_constr) { sqlVdbeCountChanges(vdbe); sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE); } @@ -1148,15 +1292,21 @@ resolve_link(struct Parse *parse_context, const = struct space_def *def, =20 /** * Emit code to create sequences, indexes, check and foreign key - * constraints appeared in . + * constraints appeared in or + * . */ static void sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id) { assert(reg_space_id !=3D 0); struct space *space =3D parse->create_table_def.new_space; - assert(space !=3D NULL); + bool is_alter =3D space =3D=3D NULL; uint32_t i =3D 0; + if (is_alter) { + space =3D parse->create_column_def.space; + i =3D space_by_name(space->def->name)->index_count; + } + assert(space !=3D NULL); for (; i < space->index_count; ++i) { struct index *idx =3D space->index[i]; vdbe_emit_create_index(parse, space->def, idx->def, @@ -1175,6 +1325,21 @@ sql_vdbe_create_constraints(struct Parse *parse, = int reg_space_id) sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id); int reg_seq_rec =3D emitNewSysSequenceRecord(parse, = reg_seq_id, = space->def->name); + if (is_alter) { + int errcode =3D ER_SQL_CANT_ADD_AUTOINC; + const char *error_msg =3D + tt_sprintf(tnt_errcode_desc(errcode), + space->def->name); + if (vdbe_emit_halt_with_presence_test(parse, + = BOX_SEQUENCE_ID, + 2, + = reg_seq_rec + 3, + 1, = errcode, + error_msg, = false, + = OP_NoConflict) + !=3D 0) + return; + } sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, = reg_seq_rec); /* Do an insertion into _space_sequence. */ int reg_space_seq_record =3D @@ -1873,24 +2038,28 @@ sql_create_foreign_key(struct Parse = *parse_context) char *parent_name =3D NULL; char *constraint_name =3D NULL; bool is_self_referenced =3D false; + struct space *space =3D parse_context->create_column_def.space; struct create_table_def *table_def =3D = &parse_context->create_table_def; - struct space *space =3D table_def->new_space; + if (space =3D=3D NULL) + space =3D table_def->new_space; /* - * Space under construction during CREATE TABLE - * processing. NULL for ALTER TABLE statement handling. + * Space under construction during + * processing or shallow copy of space during . NULL for statement handling. */ - bool is_alter =3D space =3D=3D NULL; + bool is_alter_add_constr =3D space =3D=3D NULL; uint32_t child_cols_count; struct ExprList *child_cols =3D create_fk_def->child_cols; if (child_cols =3D=3D NULL) { - assert(!is_alter); + assert(!is_alter_add_constr); child_cols_count =3D 1; } else { child_cols_count =3D child_cols->nExpr; } struct ExprList *parent_cols =3D create_fk_def->parent_cols; struct space *child_space =3D NULL; - if (is_alter) { + if (is_alter_add_constr) { const char *child_name =3D = alter_def->entity_name->a[0].zName; child_space =3D space_by_name(child_name); if (child_space =3D=3D NULL) { @@ -1908,6 +2077,8 @@ sql_create_foreign_key(struct Parse = *parse_context) goto tnt_error; } memset(fk_parse, 0, sizeof(*fk_parse)); + if (parse_context->create_column_def.space !=3D NULL) + child_space =3D space; rlist_add_entry(&parse_context->fkeys, fk_parse, link); } struct Token *parent =3D create_fk_def->parent_name; @@ -1920,28 +2091,45 @@ sql_create_foreign_key(struct Parse = *parse_context) * self-referenced, but in this case parent (which is * also child) table will definitely exist. */ - is_self_referenced =3D !is_alter && + is_self_referenced =3D !is_alter_add_constr && strcmp(parent_name, space->def->name) =3D=3D = 0; struct space *parent_space =3D space_by_name(parent_name); - if (parent_space =3D=3D NULL) { - if (is_self_referenced) { - struct fk_constraint_parse *fk =3D - rlist_first_entry(&parse_context->fkeys, - struct = fk_constraint_parse, - link); - fk->selfref_cols =3D parent_cols; - fk->is_self_referenced =3D true; - } else { - diag_set(ClientError, ER_NO_SUCH_SPACE, = parent_name);; - goto tnt_error; - } + if (parent_space =3D=3D NULL && !is_self_referenced) { + diag_set(ClientError, ER_NO_SUCH_SPACE, parent_name); + goto tnt_error; + } + if (is_self_referenced) { + struct fk_constraint_parse *fk =3D + rlist_first_entry(&parse_context->fkeys, + struct fk_constraint_parse, + link); + fk->selfref_cols =3D parent_cols; + fk->is_self_referenced =3D true; } - if (!is_alter) { + if (!is_alter_add_constr) { if (create_def->name.n =3D=3D 0) { - constraint_name =3D - sqlMPrintf(db, "fk_unnamed_%s_%d", - space->def->name, - ++parse_context->fkey_count); + uint32_t idx =3D ++parse_context->fkey_count; + /* + * If it is we + * should count the existing FK + * constraints in the space and form a + * name based on this. + */ + if (table_def->new_space =3D=3D NULL) { + struct space *original_space =3D + space_by_name(space->def->name); + assert(original_space !=3D NULL); + struct rlist *child_fk =3D + = &original_space->child_fk_constraint; + if (!rlist_empty(child_fk)) { + struct fk_constraint *fk; + rlist_foreach_entry(fk, = child_fk, + = in_child_space) + idx++; + } + } + constraint_name =3D sqlMPrintf(db, = "fk_unnamed_%s_%d", + space->def->name, = idx); } else { constraint_name =3D sql_name_from_token(db, = &create_def->name); @@ -2001,7 +2189,8 @@ sql_create_foreign_key(struct Parse = *parse_context) } int actions =3D create_fk_def->actions; fk_def->field_count =3D child_cols_count; - fk_def->child_id =3D child_space !=3D NULL ? = child_space->def->id : 0; + fk_def->child_id =3D table_def->new_space =3D=3D NULL ? + child_space->def->id : 0; fk_def->parent_id =3D parent_space !=3D NULL ? = parent_space->def->id : 0; fk_def->is_deferred =3D create_constr_def->is_deferred; fk_def->match =3D (enum fk_constraint_match) = (create_fk_def->match); @@ -2021,7 +2210,7 @@ sql_create_foreign_key(struct Parse = *parse_context) constraint_name) !=3D 0) { goto exit_create_fk; } - if (!is_alter) { + if (!is_alter_add_constr) { if (child_cols =3D=3D NULL) { assert(i =3D=3D 0); /* @@ -2050,12 +2239,13 @@ sql_create_foreign_key(struct Parse = *parse_context) memcpy(fk_def->name, constraint_name, name_len); fk_def->name[name_len] =3D '\0'; /* - * In case of CREATE TABLE processing, all foreign keys - * constraints must be created after space itself, so - * lets delay it until sqlEndTable() call and simply + * In case of or processing, all foreign keys constraints must + * be created after space itself, so lets delay it until + * sqlEndTable() or sql_add_column_end() call and simply * maintain list of all FK constraints inside parser. */ - if (!is_alter) { + if (!is_alter_add_constr) { struct fk_constraint_parse *fk_parse =3D rlist_first_entry(&parse_context->fkeys, struct fk_constraint_parse, = link); @@ -2407,7 +2597,10 @@ sql_create_index(struct Parse *parse) { * Find the table that is to be indexed. * Return early if not found. */ - struct space *space =3D NULL; + struct space *space =3D parse->create_table_def.new_space; + if (space =3D=3D NULL) + space =3D parse->create_column_def.space; + bool is_create_table_or_add_col =3D space !=3D NULL; struct Token token =3D create_entity_def->name; if (tbl_name !=3D NULL) { assert(token.n > 0 && token.z !=3D NULL); @@ -2420,10 +2613,8 @@ sql_create_index(struct Parse *parse) { } goto exit_create_index; } - } else { - if (parse->create_table_def.new_space =3D=3D NULL) - goto exit_create_index; - space =3D parse->create_table_def.new_space; + } else if (space =3D=3D NULL) { + goto exit_create_index; } struct space_def *def =3D space->def; =20 @@ -2458,7 +2649,7 @@ sql_create_index(struct Parse *parse) { * 2) UNIQUE constraint is non-named and standard * auto-index name will be generated. */ - if (parse->create_table_def.new_space =3D=3D NULL) { + if (!is_create_table_or_add_col) { assert(token.z !=3D NULL); name =3D sql_name_from_token(db, &token); if (name =3D=3D NULL) { @@ -2624,7 +2815,7 @@ sql_create_index(struct Parse *parse) { * constraint, but has different onError (behavior on * constraint violation), then an error is raised. */ - if (parse->create_table_def.new_space !=3D NULL) { + if (is_create_table_or_add_col) { for (uint32_t i =3D 0; i < space->index_count; ++i) { struct index *existing_idx =3D space->index[i]; uint32_t iid =3D existing_idx->def->iid; @@ -2712,7 +2903,7 @@ sql_create_index(struct Parse *parse) { sqlVdbeAddOp0(vdbe, OP_Expire); } =20 - if (tbl_name !=3D NULL) + if (!is_create_table_or_add_col) goto exit_create_index; table_add_index(space, index); index =3D NULL; diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 995875566..0c9887851 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -226,19 +226,24 @@ create_table_end ::=3D . { sqlEndTable(pParse); } */ =20 columnlist ::=3D columnlist COMMA tcons. -columnlist ::=3D columnlist COMMA columnname carglist autoinc(I). { - uint32_t fieldno =3D = pParse->create_table_def.new_space->def->field_count - 1; - if (I =3D=3D 1 && sql_add_autoincrement(pParse, fieldno) !=3D 0) - return; +columnlist ::=3D columnlist COMMA column_def create_column_end. +columnlist ::=3D column_def create_column_end. + +column_def ::=3D column_name_and_type carglist. + +column_name_and_type ::=3D nm(A) typedef(Y). { + create_column_def_init(&pParse->create_column_def, NULL, &A, &Y); + sql_create_column_start(pParse); } =20 -columnlist ::=3D columnname carglist autoinc(I). { - uint32_t fieldno =3D = pParse->create_table_def.new_space->def->field_count - 1; +create_column_end ::=3D autoinc(I). { + uint32_t fieldno =3D = pParse->create_column_def.space->def->field_count - 1; if (I =3D=3D 1 && sql_add_autoincrement(pParse, fieldno) !=3D 0) return; + if (pParse->create_table_def.new_space =3D=3D NULL) + sql_create_column_end(pParse); } columnlist ::=3D tcons. -columnname(A) ::=3D nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);} =20 // An IDENTIFIER can be a generic identifier, or one of several // keywords. Any non-standard keyword can also be an identifier. @@ -281,9 +286,11 @@ nm(A) ::=3D id(A). { } } =20 -// "carglist" is a list of additional constraints that come after the -// column name and column type in a CREATE TABLE statement. -// +/* + * "carglist" is a list of additional constraints and clauses that + * come after the column name and column type in a + * or statement. + */ carglist ::=3D carglist ccons. carglist ::=3D . %type cconsname { struct Token } @@ -1735,11 +1742,28 @@ alter_table_start(A) ::=3D ALTER TABLE = fullname(T) . { A =3D T; } =20 %type alter_add_constraint {struct alter_args} alter_add_constraint(A) ::=3D alter_table_start(T) ADD CONSTRAINT = nm(N). { + A.table_name =3D T; + A.name =3D N; + pParse->initiateTTrans =3D true; + } + +%type alter_add_column {struct alter_args} +alter_add_column(A) ::=3D alter_table_start(T) ADD column_name(N). { A.table_name =3D T; A.name =3D N; pParse->initiateTTrans =3D true; } =20 +column_name(N) ::=3D COLUMN nm(A). { N =3D A; } +column_name(N) ::=3D nm(A). { N =3D A; } + +cmd ::=3D alter_column_def carglist create_column_end. + +alter_column_def ::=3D alter_add_column(N) typedef(Y). { + create_column_def_init(&pParse->create_column_def, N.table_name, = &N.name, &Y); + sql_create_column_start(pParse); +} + cmd ::=3D alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP = REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R) = defer_subclause_opt(D). { create_fk_def_init(&pParse->create_fk_def, N.table_name, &N.name, FA, = &T, TA, diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h index 1105fda6e..336914c57 100644 --- a/src/box/sql/parse_def.h +++ b/src/box/sql/parse_def.h @@ -35,6 +35,7 @@ #include "box/fk_constraint.h" #include "box/key_def.h" #include "box/sql.h" +#include "box/constraint_id.h" =20 /** * This file contains auxiliary structures and functions which @@ -154,6 +155,7 @@ enum sql_index_type { =20 enum entity_type { ENTITY_TYPE_TABLE =3D 0, + ENTITY_TYPE_COLUMN, ENTITY_TYPE_VIEW, ENTITY_TYPE_INDEX, ENTITY_TYPE_TRIGGER, @@ -207,6 +209,14 @@ struct create_table_def { struct space *new_space; }; =20 +struct create_column_def { + struct create_entity_def base; + /** Shallow space_def copy. */ + struct space *space; + /** Column type. */ + struct type_def *type_def; +}; + struct create_view_def { struct create_entity_def base; /** @@ -464,6 +474,16 @@ create_table_def_init(struct create_table_def = *table_def, struct Token *name, if_not_exists); } =20 +static inline void +create_column_def_init(struct create_column_def *column_def, + struct SrcList *table_name, struct Token *name, + struct type_def *type_def) +{ + create_entity_def_init(&column_def->base, ENTITY_TYPE_COLUMN, + table_name, name, false); + column_def->type_def =3D type_def; +} + static inline void create_view_def_init(struct create_view_def *view_def, struct Token = *name, struct Token *create, struct ExprList *aliases, diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index fa87e7bd2..32142a871 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2251,22 +2251,26 @@ struct Parse { struct enable_entity_def enable_entity_def; }; /** - * Table def is not part of union since information - * being held must survive till the end of parsing of - * whole CREATE TABLE statement (to pass it to - * sqlEndTable() function). + * Table def or column def is not part of union since + * information being held must survive till the end of + * parsing of whole or + * statement (to pass it to + * sqlEndTable() sql_create_column_end() function). */ struct create_table_def create_table_def; + struct create_column_def create_column_def; /* - * FK and CK constraints appeared in a . + * FK and CK constraints appeared in a or + * an statement. */ struct rlist fkeys; struct rlist checks; uint32_t fkey_count; uint32_t check_count; /* - * True, if column within a statement to be - * created has . + * True, if column in a or an + * statement to be created has + * . */ bool has_autoinc; /* Id of field with . */ @@ -2860,15 +2864,30 @@ struct space *sqlResultSetOfSelect(Parse *, = Select *); =20 struct space * sqlStartTable(Parse *, Token *); -void sqlAddColumn(Parse *, Token *, struct type_def *); + +/** + * Add new field to the format of ephemeral space in + * create_table_def. If it is create shallow copy of + * the existing space and add field to its format. + */ +void +sql_create_column_start(struct Parse *parse); + +/** + * Emit code to update entry in _space and code to create + * constraints (entries in _index, _ck_constraint, _fk_constraint) + * described with this column. + */ +void +sql_create_column_end(struct Parse *parse); =20 /** * This routine is called by the parser while in the middle of - * parsing a CREATE TABLE statement. A "NOT NULL" constraint has - * been seen on a column. This routine sets the is_nullable flag - * on the column currently under construction. - * If nullable_action has been already set, this function raises - * an error. + * parsing a or a + * statement. A "NOT NULL" constraint has been seen on a column. + * This routine sets the is_nullable flag on the column currently + * under construction. If nullable_action has been already set, + * this function raises an error. * * @param parser SQL Parser object. * @param nullable_action on_conflict_action value. diff --git a/test/box/error.result b/test/box/error.result index cdecdb221..6fc3cb99f 100644 --- a/test/box/error.result +++ b/test/box/error.result @@ -437,6 +437,8 @@ t; | 216: box.error.SYNC_QUORUM_TIMEOUT | 217: box.error.SYNC_ROLLBACK | 218: box.error.TUPLE_METADATA_IS_TOO_BIG + | 219: box.error.SQL_CANT_ADD_COLUMN_TO_VIEW + | 220: box.error.SQL_CANT_ADD_AUTOINC | ... =20 test_run:cmd("setopt delimiter ''"); diff --git a/test/sql/add-column.result b/test/sql/add-column.result new file mode 100644 index 000000000..f86259105 --- /dev/null +++ b/test/sql/add-column.result @@ -0,0 +1,471 @@ +-- test-run result file version 2 +-- +-- gh-3075: Check statement. +-- +CREATE TABLE t1 (a INT PRIMARY KEY); + | --- + | - row_count: 1 + | ... + +-- +-- COLUMN keyword is optional. Check it here, but omit it below. +-- +ALTER TABLE t1 ADD COLUMN b INT; + | --- + | - row_count: 0 + | ... + +-- +-- A column with the same name already exists. +-- +ALTER TABLE t1 ADD b SCALAR; + | --- + | - null + | - Space field 'B' is duplicate + | ... + +-- +-- Can't add column to a view. +-- +CREATE VIEW v AS SELECT * FROM t1; + | --- + | - row_count: 1 + | ... +ALTER TABLE v ADD b INT; + | --- + | - null + | - Can't add column 'B'. 'V' is a view + | ... +DROP VIEW v; + | --- + | - row_count: 1 + | ... + +-- +-- Check PRIMARY KEY constraint works with an added column. +-- +CREATE TABLE pk_check (a INT CONSTRAINT pk PRIMARY KEY); + | --- + | - row_count: 1 + | ... +ALTER TABLE pk_check DROP CONSTRAINT pk; + | --- + | - row_count: 1 + | ... +ALTER TABLE pk_check ADD b INT PRIMARY KEY; + | --- + | - row_count: 0 + | ... +INSERT INTO pk_check VALUES (1, 1); + | --- + | - row_count: 1 + | ... +INSERT INTO pk_check VALUES (1, 1); + | --- + | - null + | - Duplicate key exists in unique index 'pk_unnamed_PK_CHECK_1' in = space 'PK_CHECK' + | ... +DROP TABLE pk_check; + | --- + | - row_count: 1 + | ... + +-- +-- Check UNIQUE constraint works with an added column. +-- +CREATE TABLE unique_check (a INT PRIMARY KEY); + | --- + | - row_count: 1 + | ... +ALTER TABLE unique_check ADD b INT UNIQUE; + | --- + | - row_count: 0 + | ... +INSERT INTO unique_check VALUES (1, 1); + | --- + | - row_count: 1 + | ... +INSERT INTO unique_check VALUES (2, 1); + | --- + | - null + | - Duplicate key exists in unique index = 'unique_unnamed_UNIQUE_CHECK_2' in space 'UNIQUE_CHECK' + | ... +DROP TABLE unique_check; + | --- + | - row_count: 1 + | ... + +-- +-- Check CHECK constraint works with an added column. +-- +CREATE TABLE ck_check (a INT PRIMARY KEY); + | --- + | - row_count: 1 + | ... +ALTER TABLE ck_check ADD b INT CHECK (b > 0); + | --- + | - row_count: 0 + | ... +INSERT INTO ck_check VALUES (1, 0); + | --- + | - null + | - 'Check constraint failed ''ck_unnamed_CK_CHECK_1'': b > 0' + | ... +DROP TABLE ck_check; + | --- + | - row_count: 1 + | ... + +-- +-- Check FOREIGN KEY constraint works with an added column. +-- +CREATE TABLE fk_check (a INT PRIMARY KEY); + | --- + | - row_count: 1 + | ... +ALTER TABLE fk_check ADD b INT REFERENCES t1(a); + | --- + | - row_count: 0 + | ... +INSERT INTO fk_check VALUES (0, 1); + | --- + | - null + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed' + | ... +INSERT INTO fk_check VALUES (2, 0); + | --- + | - null + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed' + | ... +INSERT INTO fk_check VALUES (2, 1); + | --- + | - null + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed' + | ... +DROP TABLE fk_check; + | --- + | - row_count: 1 + | ... +DROP TABLE t1; + | --- + | - row_count: 1 + | ... +-- +-- Check FOREIGN KEY (self-referenced) constraint works with an +-- added column. +-- +CREATE TABLE self (id INT PRIMARY KEY AUTOINCREMENT, a INT UNIQUE) + | --- + | - row_count: 1 + | ... +ALTER TABLE self ADD b INT REFERENCES self(a) + | --- + | - row_count: 0 + | ... +INSERT INTO self(a,b) VALUES(1, 1); + | --- + | - autoincrement_ids: + | - 1 + | row_count: 1 + | ... +UPDATE self SET a =3D 2, b =3D 2; + | --- + | - row_count: 1 + | ... +UPDATE self SET b =3D 3; + | --- + | - null + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed' + | ... +UPDATE self SET a =3D 3; + | --- + | - null + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed' + | ... +DROP TABLE self; + | --- + | - row_count: 1 + | ... + +-- +-- Check AUTOINCREMENT works with an added column. +-- +CREATE TABLE autoinc_check (a INT CONSTRAINT pk PRIMARY KEY); + | --- + | - row_count: 1 + | ... +ALTER TABLE autoinc_check DROP CONSTRAINT pk; + | --- + | - row_count: 1 + | ... +ALTER TABLE autoinc_check ADD b INT PRIMARY KEY AUTOINCREMENT; + | --- + | - row_count: 0 + | ... +INSERT INTO autoinc_check(a) VALUES(1); + | --- + | - autoincrement_ids: + | - 1 + | row_count: 1 + | ... +INSERT INTO autoinc_check(a) VALUES(1); + | --- + | - autoincrement_ids: + | - 2 + | row_count: 1 + | ... +TRUNCATE TABLE autoinc_check; + | --- + | - row_count: 0 + | ... + +-- +-- Can't add second column with AUTOINCREMENT. +-- +ALTER TABLE autoinc_check ADD c INT AUTOINCREMENT; + | --- + | - null + | - 'Can''t add AUTOINCREMENT: the space ''AUTOINC_CHECK'' already has = one auto incremented + | field' + | ... +DROP TABLE autoinc_check; + | --- + | - row_count: 1 + | ... + +-- +-- Check COLLATE clause works with an added column. +-- +CREATE TABLE collate_check (a INT PRIMARY KEY); + | --- + | - row_count: 1 + | ... +ALTER TABLE collate_check ADD b TEXT COLLATE "unicode_ci"; + | --- + | - row_count: 0 + | ... +INSERT INTO collate_check VALUES (1, 'a'); + | --- + | - row_count: 1 + | ... +INSERT INTO collate_check VALUES (2, 'A'); + | --- + | - row_count: 1 + | ... +SELECT * FROM collate_check WHERE b LIKE 'a'; + | --- + | - metadata: + | - name: A + | type: integer + | - name: B + | type: string + | rows: + | - [1, 'a'] + | - [2, 'A'] + | ... +DROP TABLE collate_check; + | --- + | - row_count: 1 + | ... + +-- +-- Check DEFAULT clause works with an added column. +-- +CREATE TABLE default_check (a INT PRIMARY KEY); + | --- + | - row_count: 1 + | ... +ALTER TABLE default_check ADD b TEXT DEFAULT ('a'); + | --- + | - row_count: 0 + | ... +INSERT INTO default_check(a) VALUES (1); + | --- + | - row_count: 1 + | ... +SELECT * FROM default_check; + | --- + | - metadata: + | - name: A + | type: integer + | - name: B + | type: string + | rows: + | - [1, 'a'] + | ... +DROP TABLE default_check; + | --- + | - row_count: 1 + | ... + +-- +-- Check NULL constraint works with an added column. +-- +CREATE TABLE null_check (a INT PRIMARY KEY); + | --- + | - row_count: 1 + | ... +ALTER TABLE null_check ADD b TEXT NULL; + | --- + | - row_count: 0 + | ... +INSERT INTO null_check(a) VALUES (1); + | --- + | - row_count: 1 + | ... +DROP TABLE null_check; + | --- + | - row_count: 1 + | ... + +-- +-- Check NOT NULL constraint works with an added column. +-- +CREATE TABLE notnull_check (a INT PRIMARY KEY); + | --- + | - row_count: 1 + | ... +ALTER TABLE notnull_check ADD b TEXT NOT NULL; + | --- + | - row_count: 0 + | ... +INSERT INTO notnull_check(a) VALUES (1); + | --- + | - null + | - 'Failed to execute SQL statement: NOT NULL constraint failed: = NOTNULL_CHECK.B' + | ... +DROP TABLE notnull_check; + | --- + | - row_count: 1 + | ... + +-- +-- Can't add a column with DEAFULT or NULL to a non-empty space. +-- This ability isn't implemented yet. +-- +CREATE TABLE non_empty (a INT PRIMARY KEY); + | --- + | - row_count: 1 + | ... +INSERT INTO non_empty VALUES (1); + | --- + | - row_count: 1 + | ... +ALTER TABLE non_empty ADD b INT NULL; + | --- + | - null + | - Tuple field count 1 does not match space field count 2 + | ... +ALTER TABLE non_empty ADD b INT DEFAULT (1); + | --- + | - null + | - Tuple field count 1 does not match space field count 2 + | ... +DROP TABLE non_empty; + | --- + | - row_count: 1 + | ... + +-- +-- Add to a no-SQL adjusted space without format. +-- +\set language lua + | --- + | - true + | ... +_ =3D box.schema.space.create('WITHOUT_FORMAT') + | --- + | ... +\set language sql + | --- + | - true + | ... +ALTER TABLE without_format ADD a INT PRIMARY KEY; + | --- + | - row_count: 0 + | ... +INSERT INTO without_format VALUES (1); + | --- + | - row_count: 1 + | ... +DROP TABLE without_format; + | --- + | - row_count: 1 + | ... + +-- +-- Add to a no-SQL adjusted space with format. +-- +\set language lua + | --- + | - true + | ... +with_format =3D box.schema.space.create('WITH_FORMAT') + | --- + | ... +with_format:format{{name =3D 'A', type =3D 'unsigned'}} + | --- + | ... +\set language sql + | --- + | - true + | ... +ALTER TABLE with_format ADD b INT PRIMARY KEY; + | --- + | - row_count: 0 + | ... +INSERT INTO with_format VALUES (1, 1); + | --- + | - row_count: 1 + | ... +DROP TABLE with_format; + | --- + | - row_count: 1 + | ... + +-- +-- Add multiple columns (with a constraint) inside a transaction. +-- +CREATE TABLE t2 (a INT PRIMARY KEY) + | --- + | - row_count: 1 + | ... +\set language lua + | --- + | - true + | ... +box.begin() = \ +box.execute('ALTER TABLE t2 ADD b INT') = \ +box.execute('ALTER TABLE t2 ADD c INT UNIQUE') = \ +box.commit() + | --- + | ... +\set language sql + | --- + | - true + | ... +INSERT INTO t2 VALUES (1, 1, 1); + | --- + | - row_count: 1 + | ... +INSERT INTO t2 VALUES (2, 1, 1); + | --- + | - null + | - Duplicate key exists in unique index 'unique_unnamed_T2_2' in = space 'T2' + | ... +SELECT * FROM t2; + | --- + | - metadata: + | - name: A + | type: integer + | - name: B + | type: integer + | - name: C + | type: integer + | rows: + | - [1, 1, 1] + | ... +DROP TABLE t2; + | --- + | - row_count: 1 + | ... diff --git a/test/sql/add-column.test.sql b/test/sql/add-column.test.sql new file mode 100644 index 000000000..f8ab3f756 --- /dev/null +++ b/test/sql/add-column.test.sql @@ -0,0 +1,167 @@ +-- +-- gh-3075: Check statement. +-- +CREATE TABLE t1 (a INT PRIMARY KEY); + +-- +-- COLUMN keyword is optional. Check it here, but omit it below. +-- +ALTER TABLE t1 ADD COLUMN b INT; + +-- +-- A column with the same name already exists. +-- +ALTER TABLE t1 ADD b SCALAR; + +-- +-- Can't add column to a view. +-- +CREATE VIEW v AS SELECT * FROM t1; +ALTER TABLE v ADD b INT; +DROP VIEW v; + +-- +-- Check PRIMARY KEY constraint works with an added column. +-- +CREATE TABLE pk_check (a INT CONSTRAINT pk PRIMARY KEY); +ALTER TABLE pk_check DROP CONSTRAINT pk; +ALTER TABLE pk_check ADD b INT PRIMARY KEY; +INSERT INTO pk_check VALUES (1, 1); +INSERT INTO pk_check VALUES (1, 1); +DROP TABLE pk_check; + +-- +-- Check UNIQUE constraint works with an added column. +-- +CREATE TABLE unique_check (a INT PRIMARY KEY); +ALTER TABLE unique_check ADD b INT UNIQUE; +INSERT INTO unique_check VALUES (1, 1); +INSERT INTO unique_check VALUES (2, 1); +DROP TABLE unique_check; + +-- +-- Check CHECK constraint works with an added column. +-- +CREATE TABLE ck_check (a INT PRIMARY KEY); +ALTER TABLE ck_check ADD b INT CHECK (b > 0); +INSERT INTO ck_check VALUES (1, 0); +DROP TABLE ck_check; + +-- +-- Check FOREIGN KEY constraint works with an added column. +-- +CREATE TABLE fk_check (a INT PRIMARY KEY); +ALTER TABLE fk_check ADD b INT REFERENCES t1(a); +INSERT INTO fk_check VALUES (0, 1); +INSERT INTO fk_check VALUES (2, 0); +INSERT INTO fk_check VALUES (2, 1); +DROP TABLE fk_check; +DROP TABLE t1; +-- +-- Check FOREIGN KEY (self-referenced) constraint works with an +-- added column. +-- +CREATE TABLE self (id INT PRIMARY KEY AUTOINCREMENT, a INT UNIQUE) +ALTER TABLE self ADD b INT REFERENCES self(a) +INSERT INTO self(a,b) VALUES(1, 1); +UPDATE self SET a =3D 2, b =3D 2; +UPDATE self SET b =3D 3; +UPDATE self SET a =3D 3; +DROP TABLE self; + +-- +-- Check AUTOINCREMENT works with an added column. +-- +CREATE TABLE autoinc_check (a INT CONSTRAINT pk PRIMARY KEY); +ALTER TABLE autoinc_check DROP CONSTRAINT pk; +ALTER TABLE autoinc_check ADD b INT PRIMARY KEY AUTOINCREMENT; +INSERT INTO autoinc_check(a) VALUES(1); +INSERT INTO autoinc_check(a) VALUES(1); +TRUNCATE TABLE autoinc_check; + +-- +-- Can't add second column with AUTOINCREMENT. +-- +ALTER TABLE autoinc_check ADD c INT AUTOINCREMENT; +DROP TABLE autoinc_check; + +-- +-- Check COLLATE clause works with an added column. +-- +CREATE TABLE collate_check (a INT PRIMARY KEY); +ALTER TABLE collate_check ADD b TEXT COLLATE "unicode_ci"; +INSERT INTO collate_check VALUES (1, 'a'); +INSERT INTO collate_check VALUES (2, 'A'); +SELECT * FROM collate_check WHERE b LIKE 'a'; +DROP TABLE collate_check; + +-- +-- Check DEFAULT clause works with an added column. +-- +CREATE TABLE default_check (a INT PRIMARY KEY); +ALTER TABLE default_check ADD b TEXT DEFAULT ('a'); +INSERT INTO default_check(a) VALUES (1); +SELECT * FROM default_check; +DROP TABLE default_check; + +-- +-- Check NULL constraint works with an added column. +-- +CREATE TABLE null_check (a INT PRIMARY KEY); +ALTER TABLE null_check ADD b TEXT NULL; +INSERT INTO null_check(a) VALUES (1); +DROP TABLE null_check; + +-- +-- Check NOT NULL constraint works with an added column. +-- +CREATE TABLE notnull_check (a INT PRIMARY KEY); +ALTER TABLE notnull_check ADD b TEXT NOT NULL; +INSERT INTO notnull_check(a) VALUES (1); +DROP TABLE notnull_check; + +-- +-- Can't add a column with DEAFULT or NULL to a non-empty space. +-- This ability isn't implemented yet. +-- +CREATE TABLE non_empty (a INT PRIMARY KEY); +INSERT INTO non_empty VALUES (1); +ALTER TABLE non_empty ADD b INT NULL; +ALTER TABLE non_empty ADD b INT DEFAULT (1); +DROP TABLE non_empty; + +-- +-- Add to a no-SQL adjusted space without format. +-- +\set language lua +_ =3D box.schema.space.create('WITHOUT_FORMAT') +\set language sql +ALTER TABLE without_format ADD a INT PRIMARY KEY; +INSERT INTO without_format VALUES (1); +DROP TABLE without_format; + +-- +-- Add to a no-SQL adjusted space with format. +-- +\set language lua +with_format =3D box.schema.space.create('WITH_FORMAT') +with_format:format{{name =3D 'A', type =3D 'unsigned'}} +\set language sql +ALTER TABLE with_format ADD b INT PRIMARY KEY; +INSERT INTO with_format VALUES (1, 1); +DROP TABLE with_format; + +-- +-- Add multiple columns (with a constraint) inside a transaction. +-- +CREATE TABLE t2 (a INT PRIMARY KEY) +\set language lua +box.begin() = \ +box.execute('ALTER TABLE t2 ADD b INT') = \ +box.execute('ALTER TABLE t2 ADD c INT UNIQUE') = \ +box.commit() +\set language sql +INSERT INTO t2 VALUES (1, 1, 1); +INSERT INTO t2 VALUES (2, 1, 1); +SELECT * FROM t2; +DROP TABLE t2; diff --git a/test/sql/checks.result b/test/sql/checks.result index 7b18e5d6b..513ed1b62 100644 --- a/test/sql/checks.result +++ b/test/sql/checks.result @@ -856,6 +856,26 @@ box.execute("DROP TABLE t6") --- - row_count: 1 ... +-- +-- gh-3075: Check the auto naming of CHECK constraints in +-- . +-- +box.execute("CREATE TABLE check_naming (a INT PRIMARY KEY CHECK (a > = 0))") +--- +- row_count: 1 +... +box.execute("ALTER TABLE check_naming ADD b INT CHECK (b > 0)") +--- +- row_count: 0 +... +box.execute("ALTER TABLE check_naming DROP CONSTRAINT = \"ck_unnamed_CHECK_NAMING_2\"") +--- +- row_count: 1 +... +box.execute("DROP TABLE check_naming") +--- +- row_count: 1 +... test_run:cmd("clear filter") --- - true diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua index 301f8ea69..a79131466 100644 --- a/test/sql/checks.test.lua +++ b/test/sql/checks.test.lua @@ -280,4 +280,13 @@ box.func.MYFUNC:drop() box.execute("INSERT INTO t6 VALUES(11);"); box.execute("DROP TABLE t6") =20 +-- +-- gh-3075: Check the auto naming of CHECK constraints in +-- . +-- +box.execute("CREATE TABLE check_naming (a INT PRIMARY KEY CHECK (a > = 0))") +box.execute("ALTER TABLE check_naming ADD b INT CHECK (b > 0)") +box.execute("ALTER TABLE check_naming DROP CONSTRAINT = \"ck_unnamed_CHECK_NAMING_2\"") +box.execute("DROP TABLE check_naming") + test_run:cmd("clear filter") diff --git a/test/sql/foreign-keys.result b/test/sql/foreign-keys.result index 33689a06e..de2a0c512 100644 --- a/test/sql/foreign-keys.result +++ b/test/sql/foreign-keys.result @@ -499,5 +499,33 @@ box.space.S:drop() box.space.T:drop() --- ... +-- +-- gh-3075: Check the auto naming of FOREIGN KEY constraints in +-- . +-- +box.execute("CREATE TABLE t1 (a INT PRIMARY KEY)") +--- +- row_count: 1 +... +box.execute("CREATE TABLE check_naming (a INT PRIMARY KEY REFERENCES = t1(a))") +--- +- row_count: 1 +... +box.execute("ALTER TABLE check_naming ADD b INT REFERENCES t1(a)") +--- +- row_count: 0 +... +box.execute("ALTER TABLE check_naming DROP CONSTRAINT = \"fk_unnamed_CHECK_NAMING_2\"") +--- +- row_count: 1 +... +box.execute("DROP TABLE check_naming") +--- +- row_count: 1 +... +box.execute("DROP TABLE t1") +--- +- row_count: 1 +... --- Clean-up SQL DD hash. -test_run:cmd('restart server default with cleanup=3D1') diff --git a/test/sql/foreign-keys.test.lua = b/test/sql/foreign-keys.test.lua index d2dd88d28..29918c5d4 100644 --- a/test/sql/foreign-keys.test.lua +++ b/test/sql/foreign-keys.test.lua @@ -209,5 +209,16 @@ box.space.T:select() box.space.S:drop() box.space.T:drop() =20 +-- +-- gh-3075: Check the auto naming of FOREIGN KEY constraints in +-- . +-- +box.execute("CREATE TABLE t1 (a INT PRIMARY KEY)") +box.execute("CREATE TABLE check_naming (a INT PRIMARY KEY REFERENCES = t1(a))") +box.execute("ALTER TABLE check_naming ADD b INT REFERENCES t1(a)") +box.execute("ALTER TABLE check_naming DROP CONSTRAINT = \"fk_unnamed_CHECK_NAMING_2\"") +box.execute("DROP TABLE check_naming") +box.execute("DROP TABLE t1") + --- Clean-up SQL DD hash. -test_run:cmd('restart server default with cleanup=3D1')