From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (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 CAE20445320 for ; Sun, 12 Jul 2020 19:45:48 +0300 (MSK) References: <20200403152752.8923-1-roman.habibov@tarantool.org> <20200403152752.8923-3-roman.habibov@tarantool.org> <5CF72787-A1F0-4C48-BA8F-08F02B6960F6@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sun, 12 Jul 2020 18:45:46 +0200 MIME-Version: 1.0 In-Reply-To: <5CF72787-A1F0-4C48-BA8F-08F02B6960F6@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! See 33 comments below. > commit 82c448dc66f6233faeb40dda353652c2fd5a3d29 > Author: Roman Khabibov > Date: Thu Jan 2 19:06:14 2020 +0300 > > sql: support column addition > > 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. > > Closes #2349, #3075 > > @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. > > For example: > > tarantool> box.execute([[CREATE TABLE test ( > a INTEGER PRIMARY KEY > );]]) > --- > - row_count: 1 > ... > > tarantool> box.execute([[ALTER TABLE test ADD COLUMN > b TEXT > NOT NULL > DEFAULT ('a') > COLLATE "unicode_ci" > ;]]) > --- > - row_count: 0 > ... > --- 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? 2. The example from the doc request does not work: tarantool> box.execute([[CREATE TABLE test (a INTEGER PRIMARY KEY);]]) --- - row_count: 1 ... 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.' ... > 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[] = { > { "CHECK", "TK_CHECK", true }, > { "COLLATE", "TK_COLLATE", true }, > { "COLUMN_NAME", "TK_COLUMN_NAME", true }, > - { "COLUMN", "TK_STANDARD", true }, > + { "COLUMN", "TK_COLUMN", true }, 3. This is how the same hunk looks on the branch: ==================== 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[] = { { "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 }, ==================== It is very different. Why? > 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") \ 4. The same here: ==================== --- 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'") \ /* * !IMPORTANT! Please follow instructions at start of the file ==================== >From 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. > 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; > } > > -/* > - * 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. 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)? > */ > +static struct space * > +sql_shallow_space_copy(struct Parse *parse, struct space *space) > +{ > + assert(space->def != NULL); > + struct space *ret = sql_ephemeral_space_new(parse, space->def->name); > + if (ret == NULL) > + return NULL; > + ret->index_count = space->index_count; > + ret->index_id_max = space->index_id_max; > + uint32_t indexes_sz = sizeof(struct index *) * (ret->index_count); > + ret->index = (struct index **) malloc(indexes_sz); 6. When is this array freed? > + if (ret->index == NULL) { > + diag_set(OutOfMemory, indexes_sz, "realloc", "ret->index"); > + return NULL; > + } > + for (uint32_t i = 0; i < ret->index_count; i++) > + ret->index[i] = space->index[i]; 7. What is the problem to make memcpy() instead of the cycle? > + memcpy(ret->def, space->def, sizeof(struct space_def)); > + ret->def->opts.is_temporary = true; > + ret->def->opts.is_ephemeral = true; > + uint32_t fields_size = sizeof(struct field_def) * ret->def->field_count; > + ret->def->fields = region_alloc(&parse->region, fields_size); 8. Use region_alloc_array. Otherwise it will crash in ASAN build. It should be visible in CI. > + if (ret->def->fields == NULL) { > + diag_set(OutOfMemory, fields_size, "region_alloc", > + "ret->def->fields"); 9. index array leaks here. > + 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 != NULL); > - char *z; > - sql *db = pParse->db; > - if (pParse->create_table_def.new_space == NULL) > - return; > - struct space_def *def = pParse->create_table_def.new_space->def; > + struct create_column_def *create_column_def = &parse->create_column_def; > + struct alter_entity_def *alter_entity_def = > + &create_column_def->base.base; > + assert(alter_entity_def->entity_type == ENTITY_TYPE_COLUMN); > + assert(alter_entity_def->alter_action == ALTER_ACTION_CREATE); > + struct space *space = parse->create_table_def.new_space; > + bool is_alter = space == NULL; > + struct sql *db = parse->db; > + if (is_alter) { > + const char *space_name = > + alter_entity_def->entity_name->a[0].zName; > + space = space_by_name(space_name); > + if (space == NULL) { > + diag_set(ClientError, ER_NO_SUCH_SPACE, space_name); > + goto tnt_error; > + } > + space = sql_shallow_space_copy(parse, space); > + if (space == NULL) > + goto tnt_error; > + } > + create_column_def->space = space; > + struct space_def *def = space->def; > + assert(def->opts.is_ephemeral); > > #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 = true; > - return; > + goto tnt_error; 10. Was there a leak before your patch? Because of not called sqlSrcListDelete(db, alter_entity_def->entity_name); > } > #endif > + > + struct region *region = &parse->region; > + struct Token *name = &create_column_def->base.name; > + char *column_name = > + sql_normalized_name_region_new(region, name->z, name->n); > + if (column_name == NULL) > + goto tnt_error; > + > + if (parse->create_table_def.new_space == NULL && def->opts.is_view) { 11. You have is_alter flag for that. > + 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) == NULL) > - return; > - struct region *region = &pParse->region; > - z = sql_normalized_name_region_new(region, pName->z, pName->n); > - if (z == NULL) { > - pParse->is_aborted = true; > + if (def->exact_field_count == 0) > + def->exact_field_count = def->field_count; > + if (sql_field_retrieve(parse, def, def->field_count) == NULL) > return; > + > + for (uint32_t i = 0; i < def->field_count; i++) { > + if (strcmp(column_name, def->fields[i].name) == 0) { > + diag_set(ClientError, ER_SPACE_FIELD_IS_DUPLICATE, > + column_name); > + goto tnt_error; > + } 12. I remember this code was deliberately removed, because the same check is already done in box. Why did you return it? > } > struct field_def *column_def = &def->fields[def->field_count]; > - memcpy(column_def, &field_def_default, sizeof(field_def_default)); > - column_def->name = z; > + memcpy(column_def, &field_def_default, sizeof(struct field_def)); 13. Unnecessary change of memcpy(). > + column_def->name = 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 = ON_CONFLICT_ACTION_DEFAULT; > column_def->is_nullable = true; > - column_def->type = type_def->type; > + column_def->type = create_column_def->type_def->type; > def->field_count++; > + > + sqlSrcListDelete(db, alter_entity_def->entity_name); > + return; > +tnt_error: > + parse->is_aborted = 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 = &parse->create_column_def; > + struct space *space = parse->create_table_def.new_space; > + bool is_alter = space == NULL; > + space = create_column_def->space; > + struct space_def *def = space->def; > + if (is_alter) { > + struct field_def *field = &def->fields[def->field_count - 1]; > + if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) { > + if (create_column_def->is_pk) { > + field->nullable_action = > + ON_CONFLICT_ACTION_ABORT; > + field->is_nullable = false; > + } else { > + field->nullable_action = > + ON_CONFLICT_ACTION_NONE; > + field->is_nullable = true; > + } > + } > + /* > + * Encode the format array and emit code to update _space. > + */ > + uint32_t table_stmt_sz = 0; > + struct region *region = &parse->region; > + char *table_stmt = sql_encode_table(region, def, > + &table_stmt_sz); > + char *raw = sqlDbMallocRaw(parse->db, table_stmt_sz); > + if (table_stmt == NULL || raw == NULL) { > + parse->is_aborted = true; > + return; > + } > + memcpy(raw, table_stmt, table_stmt_sz); > + > + struct Vdbe *v = sqlGetVdbe(parse); > + assert(v != NULL); > + > + struct space *system_space = space_by_id(BOX_SPACE_ID); > + assert(system_space != NULL); > + int cursor = parse->nTab++; > + vdbe_emit_open_cursor(parse, cursor, 0, system_space); > + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); > + > + int key_reg = ++parse->nMem; > + sqlVdbeAddOp2(v, OP_Integer, def->id, key_reg); > + int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 1); > + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT); > + sqlVdbeJumpHere(v, addr); > + > + int tuple_reg = sqlGetTempRange(parse, box_space_field_MAX + 1); > + for (int i = 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 = ON_CONFLICT_ACTION_DEFAULT; 14. Why do you touch column creation def after its usage? > } > > void > sql_column_add_nullable_action(struct Parse *parser, > enum on_conflict_action nullable_action) > { > - struct space *space = parser->create_table_def.new_space; > - if (space == NULL || NEVER(space->def->field_count < 1)) > + struct space_def *def = NULL; > + struct field_def *field = NULL; > + struct space *space = parser->create_column_def.space; > + assert (space != NULL); 15. Please, don't put whitespace after macro/function name and its arguments. > + def = space->def; > + if (NEVER(def->field_count < 1)) > return; > - struct space_def *def = space->def; > - struct field_def *field = &def->fields[def->field_count - 1]; > + field = &def->fields[def->field_count - 1]; 16. Why did you do that change about struct field_def? It seems not to be needed. > if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT && > nullable_action != field->nullable_action) { > /* Prevent defining nullable_action many times. */ > @@ -364,51 +509,46 @@ sql_column_add_nullable_action(struct Parse *parser, > } > > /* > - * 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 = pParse->db; > - struct space *p = pParse->create_table_def.new_space; > - if (p != NULL) { > - assert(p->def->opts.is_ephemeral); > - struct space_def *def = p->def; > - if (!sqlExprIsConstantOrFunction > - (pSpan->pExpr, db->init.busy)) { > - const char *column_name = > - 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 = NULL; > + struct field_def *field = NULL; > + struct space *space = pParse->create_column_def.space; > + assert (space != NULL); > + def = space->def; 17. Why can't you declare and initialize space_def in one line? The same for field. > + field = &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 = true; > + } else { > + assert(def != NULL); > + struct region *region = &pParse->region; > + uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart); > + field->default_value = region_alloc(region, default_length + 1); > + if (field->default_value == NULL) { > + diag_set(OutOfMemory, default_length + 1, > + "region_alloc", "field->default_value"); > pParse->is_aborted = true; > - } else { > - assert(def != NULL); > - struct field_def *field = > - &def->fields[def->field_count - 1]; > - struct region *region = &pParse->region; > - uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart); > - field->default_value = region_alloc(region, > - default_length + 1); > - if (field->default_value == NULL) { > - diag_set(OutOfMemory, default_length + 1, > - "region_alloc", > - "field->default_value"); > - pParse->is_aborted = true; > - return; > - } > - strncpy(field->default_value, pSpan->zStart, > - default_length); > - field->default_value[default_length] = '\0'; > + goto add_default_value_exit; > } > + strncpy(field->default_value, pSpan->zStart, default_length); > + field->default_value[default_length] = '\0'; > } > +add_default_value_exit: > sql_expr_delete(db, pSpan->pExpr, false); > } > > @@ -447,6 +587,8 @@ sqlAddPrimaryKey(struct Parse *pParse) > int nTerm; > struct ExprList *pList = pParse->create_index_def.cols; > struct space *space = pParse->create_table_def.new_space; > + if (space == NULL) > + space = pParse->create_column_def.space; > if (space == NULL) > goto primary_key_exit; > if (sql_space_primary_key(space) != NULL) { > @@ -574,8 +716,10 @@ sql_create_check_contraint(struct Parse *parser) > (struct alter_entity_def *) create_ck_def; > assert(alter_def->entity_type == ENTITY_TYPE_CK); > (void) alter_def; > - struct space *space = parser->create_table_def.new_space; > - bool is_alter = space == NULL; > + struct space *space = parser->create_column_def.space; > + if (space == NULL) > + space = parser->create_table_def.new_space; > + bool is_alter_add_constr = space == NULL; 18. Why is it called 'is_alter' in other functions, but 'is_alter_add_constr' here? > > /* Prepare payload for ck constraint definition. */ > struct region *region = &parser->region; > @@ -589,9 +733,18 @@ sql_create_check_contraint(struct Parse *parser) > return; > } > } else { > - assert(! is_alter); > - uint32_t ck_idx = ++parser->create_table_def.check_count; > - name = tt_sprintf("ck_unnamed_%s_%d", space->def->name, ck_idx); > + assert(!is_alter_add_constr); > + uint32_t idx = ++parser->check_count; > + if (parser->create_table_def.new_space == NULL) { > + struct space *original_space = > + space_by_name(space->def->name); > + assert(original_space != NULL); > + struct rlist *checks = &original_space->ck_constraint; > + struct ck_constraint *ck; > + rlist_foreach_entry(ck, checks, link) > + idx++; > + } > + name = tt_sprintf("ck_unnamed_%s_%d", space->def->name, idx); > } > size_t name_len = strlen(name); > > @@ -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] = '\0'; > - if (is_alter) { > + if (is_alter_add_constr) { > const char *space_name = alter_def->entity_name->a[0].zName; > - struct space *space = space_by_name(space_name); > + space = space_by_name(space_name); 19. Why this change? > if (space == NULL) { > diag_set(ClientError, ER_NO_SUCH_SPACE, space_name); > parser->is_aborted = 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); > } > } > > @@ -664,18 +816,19 @@ sql_create_check_contraint(struct Parse *parser) > void > sqlAddCollateType(Parse * pParse, Token * pToken) > { > - struct space *space = pParse->create_table_def.new_space; > - if (space == NULL) > - return; > + struct space *space = pParse->create_column_def.space; > + uint32_t *coll_id = NULL; > + assert(space != NULL); > uint32_t i = space->def->field_count - 1; > + coll_id = &space->def->fields[i].coll_id; 20. Why not declare and initialize coll_id in one line? > sql *db = pParse->db; > char *coll_name = sql_name_from_token(db, pToken); > if (coll_name == NULL) { > pParse->is_aborted = true; > return; > } > - uint32_t *coll_id = &space->def->fields[i].coll_id; > - if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL) { > + if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL && > + space != NULL) { 21. You have assert(space != NULL) above. Why do you need this check again? > /* 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); > } > > + 22. ??? > 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; > } > > +/** > + * 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) 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. > +{ > + assert(reg_space_id != 0); > + struct space *space = parse->create_table_def.new_space; > + bool is_alter = space == NULL; > + uint32_t i = 0; > + if (is_alter) { > + space = parse->create_column_def.space; > + i = space_by_name(space->def->name)->index_count; > + } > + assert(space != NULL); > + for (; i < space->index_count; ++i) { > + struct index *idx = 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 = ++parse->nMem; > + struct Vdbe *v = sqlGetVdbe(parse); > + assert(v != NULL); > + sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id); > + int reg_seq_record = > + emitNewSysSequenceRecord(parse, reg_seq_id, > + space->def->name); > + const char *error_msg = > + 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) != 0) > + return; > + sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record); > + /* Do an insertion into _space_sequence. */ > + int reg_space_seq_record = > + 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 = fk_parse->fk_def; > + if (fk_parse->selfref_cols != NULL) { > + struct ExprList *cols = fk_parse->selfref_cols; > + for (uint32_t i = 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) != 0) > + return; > + } > + fk_def->parent_id = reg_space_id; > + } else if (fk_parse->is_self_referenced) { > + struct key_def *pk_key_def = > + sql_space_primary_key(space)->def->key_def; > + if (pk_key_def->part_count != 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 = true; > + return; > + } > + for (uint32_t i = 0; i < fk_def->field_count; ++i) { > + fk_def->links[i].parent_field = > + pk_key_def->parts[i].fieldno; > + } > + fk_def->parent_id = reg_space_id; > + } > + fk_def->child_id = 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) > > int reg_space_id = getNewSpaceId(pParse); > vdbe_emit_space_create(pParse, reg_space_id, name_reg, new_space); > - for (uint32_t i = 0; i < new_space->index_count; ++i) { > - struct index *idx = 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 != 0); > - /* Do an insertion into _sequence. */ > - int reg_seq_id = ++pParse->nMem; > - sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id); > - int reg_seq_record = > - 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 = > - 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 = fk_parse->fk_def; > - if (fk_parse->selfref_cols != NULL) { > - struct ExprList *cols = fk_parse->selfref_cols; > - for (uint32_t i = 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) != 0) > - return; > - } > - fk_def->parent_id = reg_space_id; > - } else if (fk_parse->is_self_referenced) { > - struct index *pk = sql_space_primary_key(new_space); > - if (pk->def->key_def->part_count != 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 = true; > - return; > - } > - for (uint32_t i = 0; i < fk_def->field_count; ++i) { > - fk_def->links[i].parent_field = > - pk->def->key_def->parts[i].fieldno; > - } > - fk_def->parent_id = reg_space_id; > - } > - fk_def->child_id = 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 ::= 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 = true; 24. Why can't this be done in sqlAddPrimaryKey(pParse);? What if this is CREATE TABLE, and there are many columns in PK? > } > ccons ::= 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 = db; > + parser->create_column_def.nullable_action = ON_CONFLICT_ACTION_DEFAULT; 25. Why isn't there a proper constructor for create_column_def structure? > parser->sql_flags = sql_flags; > parser->line_count = 1; > parser->line_pos = 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. > + parser->has_autoinc = false; > region_create(&parser->region, &cord()->slabc); > } > > 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; 27. What column? This is struct Parse, it is not a column. > + /** 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 *); > > 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); > > /** > * 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 > | ... > > 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 28. You may want to look at the original SQLite tests. They probably have some. > @@ -0,0 +1,276 @@ > +-- test-run result file version 2 > +test_run = require('test_run').new() > + | --- > + | ... > +engine = test_run:get_cfg('engine') > + | --- > + | ... > +_ = box.space._session_settings:update('sql_default_engine', {{'=', 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 29. Worth checking if it works. Not just created and dropped right away. > + | ... > + > +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 30. The same. > + | ... > +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. > +-- > +_ = box.schema.space.create('WITHOUT') > + | --- > + | ... > +box.execute("ALTER TABLE WITHOUT ADD a INTEGER;") 31. No need to caps table names in SQL. 32. Need to check if it actually worked. The same below. > + | --- > + | - row_count: 0 > + | ... > +box.execute("DROP TABLE WITHOUT;") > + | --- > + | - row_count: 1 > + | ... > + > +s = box.schema.space.create('NOSQL') > + | --- > + | ... > +s:format{{name = 'A', type = '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] 33. What if I add a column with UNIQUE constraint?