From: Roman Khabibov <roman.habibov@tarantool.org> To: Nikita Pettik <korablev@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v4 5/5] sql: support column addition Date: Tue, 10 Nov 2020 15:18:00 +0300 [thread overview] Message-ID: <8AFFA6D9-CCF7-4134-9018-0C6389542ADE@tarantool.org> (raw) In-Reply-To: <20201106155719.GA13328@tarantool.org> Hi! Thanks for the review. > On Nov 6, 2020, at 18:57, Nikita Pettik <korablev@tarantool.org> wrote: > > On 09 Oct 16:45, Roman Khabibov wrote: >> diff --git a/src/box/errcode.h b/src/box/errcode.h >> index e6957d612..5a59f477f 100644 >> --- a/src/box/errcode.h >> +++ b/src/box/errcode.h >> @@ -273,6 +273,8 @@ struct errcode_record { >> /*218 */_(ER_TUPLE_METADATA_IS_TOO_BIG, "Can't create tuple: metadata size %u is too big") \ >> /*219 */_(ER_XLOG_GAP, "%s") \ >> /*220 */_(ER_TOO_EARLY_SUBSCRIBE, "Can't subscribe non-anonymous replica %s until join is done") \ >> + /*221 */_(ER_SQL_CANT_ADD_AUTOINC, "Can't add AUTOINCREMENT: space %s can't feature more than one AUTOINCREMENT field") \ >> + > > Extra empty line. Removed. >> /* >> * !IMPORTANT! Please follow instructions at start of the file >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index fd8e7ed1e..597608bea 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -285,48 +285,110 @@ sql_field_retrieve(Parse *parser, struct space_def *space_def, uint32_t id) >> return field; >> } >> >> +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) >> + goto error; >> + ret->index_count = space->index_count; >> + ret->index_id_max = space->index_id_max; >> + size_t size = 0; >> + ret->index = region_alloc_array(&parse->region, typeof(struct index *), >> + ret->index_count, &size); >> + if (ret->index == NULL) { >> + diag_set(OutOfMemory, size, "region_alloc_array", "ret->index"); >> + goto error; >> + } > > > You don't need all indexes from space..What you only need from space - > count of indexes (to generate name if it is required). The same for > field defs. Mb it is worth avoiding copying space at all - just > store in create_column_def list of ck/fk constraints, single > field_def, index/field count - these objects seem to be enough. > Please consider this suggestion. Yes, I understand that. But unfortunately, the current code, namely the functions for creating indexes, fk, and ck constraints, are written in such a way that they cannot be called without struct space (ephemeral or not). Vlad and I discussed this and came to the conclusion that it is better to go in the direction of reusing the code, rather than rewriting a significant part of build.c. https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018460.html https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019003.html >> + >> + 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; >> + >> /* >> - * 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) >> + if (def->exact_field_count == 0) >> + def->exact_field_count = def->field_count; > > assert(is_alter); If you meant if (def->exact_field_count == 0) { assert(is_alter); def->exact_field_count = def->field_count; } This branch is not only for alter. >> + struct Vdbe *v = sqlGetVdbe(parse); >> + assert(v != NULL); >> + >> + struct space *system_space = space_by_id(BOX_SPACE_ID); > > -> you can call it simply _space or s_space Done. >> + assert(system_space != NULL); >> + int cursor = parse->nTab++; >> + vdbe_emit_open_cursor(parse, cursor, 0, system_space); >> + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); >> + >> 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)) >> + assert(parser->create_column_def.space != NULL); >> + struct space_def *def = parser->create_column_def.space->def; >> + if (NEVER(def->field_count < 1)) >> return; > > Let's use standard facilities: assert(def->field_count > 0); Done. 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)) - return; - struct space_def *def = space->def; + assert(parser->create_column_def.space != NULL); + struct space_def *def = parser->create_column_def.space->def; + assert(def->field_count > 0); struct field_def *field = &def->fields[def->field_count - 1]; if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT && nullable_action != field->nullable_action) { >> if (space == NULL) >> goto primary_key_exit; >> if (sql_space_primary_key(space) != NULL) { >> @@ -574,8 +707,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; > > Consider tiny refactoring: > > is_alter = space == NULL; > if (is_alter) > space = ... > assert(space != NULL); In my case it will be: struct space *space = parser->create_column_def.space; bool is_alter_add_constr = space == NULL && parser->create_table_def.new_space == NULL; if (!is_alter_add_constr) space = parser->parser->create_table_def.new_space; This results in more rows than there are now. >> + if (space == NULL) >> + space = parser->create_table_def.new_space; >> + bool is_alter_add_constr = space == NULL; >> >> - assert(! is_alter); >> + assert(!is_alter_add_constr); > > What's the point of this renaming? I need to distinguish between <CREATE TABLE>, <ALTER TABLE ADD COLUMN> and <ALTER TABLE ADD CONSTRAINT> branches. > >> uint32_t ck_idx = ++parser->create_checks_def.count; >> + /* >> @@ -1149,15 +1302,28 @@ resolve_link(struct Parse *parse_context, const struct space_def *def, >> >> /** >> * Emit code to create sequences, indexes, check and foreign key >> - * constraints appeared in <CREATE TABLE>. >> + * constraints appeared in <CREATE TABLE> or >> + * <ALTER TABLE ADD COLUMN>. >> */ >> static void >> vdbe_emit_create_constraints(struct Parse *parse, int reg_space_id) >> { >> assert(reg_space_id != 0); >> struct space *space = parse->create_table_def.new_space; >> - assert(space != NULL); >> + bool is_alter = space == NULL; >> uint32_t i = 0; >> + /* >> + * If it is an <ALTER TABLE ADD COLUMN>, then we have to >> + * create all indexes added by this statement. These > > AFAIK only one index (corresponding to unique constraint). I can write any number of named unique constraints. They should be created. ALTER TABLE t ADD COLUMN a INT CONSTRAINT c_1 UNIQUE, CONSTRAINT c_2 UNIQUE … > > >> + * indexes are in the array, starting with old index_count >> + * (inside space object) and ending with new index_count >> + * (inside ephemeral space). >> + */ >> + if (is_alter) { >> + space = parse->create_column_def.space; >> + i = space_by_name(space->def->name)->index_count; >> + } >> @@ -1176,6 +1342,21 @@ vdbe_emit_create_constraints(struct Parse *parse, int reg_space_id) >> sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id); >> int reg_seq_rec = emitNewSysSequenceRecord(parse, reg_seq_id, >> space->def->name); >> + if (is_alter) { >> + int errcode = ER_SQL_CANT_ADD_AUTOINC; >> + const char *error_msg = >> + 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) >> + != 0) > > Oh please leave !=0 cond on previous line. This way looks disgusting. Done. >> + return; >> + } >> sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_rec); >> /* Do an insertion into _space_sequence. */ >> int reg_space_seq_record = >> + * CONSTRAINT> statement handling. >> */ >> - bool is_alter = space == NULL; >> + bool is_alter_add_constr = space == NULL; > > Again, what's the point of this renaming? commit d00ac8986bd37d5234e7d3889cb6f77216c1d33c Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Thu Jan 2 19:06:14 2020 +0300 sql: support column addition Enable to add column to existing space with <ALTER TABLE ADD [COLUMN]> statement. Column definition can be supplemented with the four types of constraints, <DEFAULT>, <COLLATE> 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 <ALTER TABLE table_name ADD [COLUMN] column_name column_type ...> statement. The column definition is the same as in <CREATE TABLE> statement. * Space emptiness is Tarantool's restriction. Possibilty to add column to non empty space will be implemented later. 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 > CHECK (LENGTH(b) > 1) > NOT NULL > DEFAULT ('aa') > COLLATE "unicode_ci" > ]]) --- - row_count: 1 ... ``` diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c index bacdad9b3..7480c0211 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_REF", "TK_COLUMN_REF", 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 e6957d612..cfea5bcf2 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -273,6 +273,7 @@ struct errcode_record { /*218 */_(ER_TUPLE_METADATA_IS_TOO_BIG, "Can't create tuple: metadata size %u is too big") \ /*219 */_(ER_XLOG_GAP, "%s") \ /*220 */_(ER_TOO_EARLY_SUBSCRIBE, "Can't subscribe non-anonymous replica %s until join is done") \ + /*221 */_(ER_SQL_CANT_ADD_AUTOINC, "Can't add AUTOINCREMENT: space %s can't feature more than one AUTOINCREMENT field") \ /* * !IMPORTANT! Please follow instructions at start of the file diff --git a/src/box/sql/build.c b/src/box/sql/build.c index fd8e7ed1e..35883aa0f 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -285,48 +285,110 @@ 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 an existing space with + * <ALTER TABLE ADD COLUMN> 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 != NULL); + struct space *ret = sql_ephemeral_space_new(parse, space->def->name); + if (ret == NULL) + goto error; + ret->index_count = space->index_count; + ret->index_id_max = space->index_id_max; + size_t size = 0; + ret->index = region_alloc_array(&parse->region, typeof(struct index *), + ret->index_count, &size); + if (ret->index == NULL) { + diag_set(OutOfMemory, size, "region_alloc_array", "ret->index"); + goto error; + } + memcpy(ret->index, space->index, size); + memcpy(ret->def, space->def, sizeof(struct space_def)); + ret->def->opts.is_temporary = true; + ret->def->opts.is_ephemeral = true; + if (ret->def->field_count != 0) { + uint32_t fields_size = 0; + ret->def->fields = + region_alloc_array(&parse->region, + typeof(struct field_def), + ret->def->field_count, &fields_size); + if (ret->def->fields == NULL) { + diag_set(OutOfMemory, fields_size, "region_alloc", + "ret->def->fields"); + goto error; + } + memcpy(ret->def->fields, space->def->fields, fields_size); + } + + return ret; +error: + parse->is_aborted = true; + return NULL; +} + 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; } #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; + /* - * 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) + if (def->exact_field_count == 0) + def->exact_field_count = def->field_count; + if (sql_field_retrieve(parse, 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; - return; - } + struct field_def *column_def = &def->fields[def->field_count]; memcpy(column_def, &field_def_default, sizeof(field_def_default)); - column_def->name = z; + column_def->name = column_name; /* * Marker ON_CONFLICT_ACTION_DEFAULT is used to detect * attempts to define NULL multiple time or to detect @@ -334,18 +396,85 @@ 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 +vdbe_emit_create_constraints(struct Parse *parse, int reg_space_id); + +void +sql_create_column_end(struct Parse *parse) +{ + struct space *space = parse->create_column_def.space; + assert(space != NULL); + struct space_def *def = space->def; + struct field_def *field = &def->fields[def->field_count - 1]; + if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) { + 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 *s_space = space_by_id(BOX_SPACE_ID); + assert(s_space != NULL); + int cursor = parse->nTab++; + vdbe_emit_open_cursor(parse, cursor, 0, s_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 + BOX_SPACE_FIELD_FIELD_COUNT); + sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, + tuple_reg + BOX_SPACE_FIELD_FORMAT, + 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 *) s_space, P4_SPACEPTR); + sqlVdbeCountChanges(v); + sqlVdbeChangeP5(v, OPFLAG_NCHANGE); + sqlReleaseTempRange(parse, tuple_reg, box_space_field_MAX + 1); + vdbe_emit_create_constraints(parse, key_reg); } 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)) - return; - struct space_def *def = space->def; + assert(parser->create_column_def.space != NULL); + struct space_def *def = parser->create_column_def.space->def; + assert(def->field_count > 0); struct field_def *field = &def->fields[def->field_count - 1]; if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT && nullable_action != field->nullable_action) { @@ -364,20 +493,21 @@ 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 <CREATE TABLE> or an <ALTER TABLE ADD COLUMN> + * statement. */ void sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan) { sql *db = pParse->db; - struct space *p = pParse->create_table_def.new_space; + struct space *p = pParse->create_column_def.space; if (p != NULL) { assert(p->def->opts.is_ephemeral); struct space_def *def = p->def; @@ -447,6 +577,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 +706,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; /* Prepare payload for ck constraint definition. */ struct region *region = &parser->region; @@ -589,8 +723,22 @@ sql_create_check_contraint(struct Parse *parser) return; } } else { - assert(! is_alter); + assert(!is_alter_add_constr); uint32_t ck_idx = ++parser->create_checks_def.count; + /* + * If it is <ALTER TABLE ADD COLUMN> we should + * count the existing CHECK constraints in the + * space and form a name based on this. + */ + if (parser->create_table_def.new_space == NULL) { + struct space *original_space = + space_by_name(space->def->name); + assert(original_space != NULL); + struct ck_constraint *ck; + rlist_foreach_entry(ck, &original_space->ck_constraint, + link) + ck_idx++; + } name = tt_sprintf("ck_unnamed_%s_%u", space->def->name, ck_idx); } size_t name_len = strlen(name); @@ -634,7 +782,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] = '\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); if (space == NULL) { @@ -664,9 +812,8 @@ 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; + assert(space != NULL); uint32_t i = space->def->field_count - 1; sql *db = pParse->db; char *coll_name = sql_name_from_token(db, pToken); @@ -696,7 +843,6 @@ struct coll * sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id) { assert(def != NULL); - struct space *space = space_by_id(def->id); /* * It is not always possible to fetch collation directly * from struct space due to its absence in space cache. @@ -705,13 +851,13 @@ 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 == NULL) { - assert(def->opts.is_ephemeral); + if (def->opts.is_ephemeral) { assert(column < (uint32_t)def->field_count); *coll_id = def->fields[column].coll_id; struct coll_id *collation = coll_by_id(*coll_id); return collation != NULL ? collation->coll : NULL; } + struct space *space = space_by_id(def->id); struct tuple_field *field = tuple_format_field(space->format, column); *coll_id = field->coll_id; return field->coll; @@ -795,7 +941,8 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def, memcpy(raw, index_parts, index_parts_sz); index_parts = raw; - if (parse->create_table_def.new_space != NULL) { + if (parse->create_table_def.new_space != NULL || + parse->create_column_def.space != NULL) { sqlVdbeAddOp2(v, OP_SCopy, space_id_reg, entry_reg); sqlVdbeAddOp2(v, OP_Integer, idx_def->iid, entry_reg + 1); } else { @@ -1033,18 +1180,23 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context, P4_DYNAMIC); /* * In case we are adding FK constraints during execution - * of <CREATE TABLE ...> statement, we don't have child - * id, but we know register where it will be stored. + * of <CREATE TABLE ...> or <ALTER TABLE ADD COLUMN> + * statement, we don't have child id (we know it, but + * fk->child_id stores register because of code reuse in + * vdbe_emit_create_constraints()), but we know register + * where it will be stored. */ - if (parse_context->create_table_def.new_space != NULL) { + bool is_alter_add_constr = + parse_context->create_table_def.new_space == NULL && + parse_context->create_column_def.space == 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 != 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 { @@ -1108,7 +1260,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 == NULL) { + if (is_alter_add_constr) { sqlVdbeCountChanges(vdbe); sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE); } @@ -1149,15 +1301,28 @@ resolve_link(struct Parse *parse_context, const struct space_def *def, /** * Emit code to create sequences, indexes, check and foreign key - * constraints appeared in <CREATE TABLE>. + * constraints appeared in <CREATE TABLE> or + * <ALTER TABLE ADD COLUMN>. */ static void vdbe_emit_create_constraints(struct Parse *parse, int reg_space_id) { assert(reg_space_id != 0); struct space *space = parse->create_table_def.new_space; - assert(space != NULL); + bool is_alter = space == NULL; uint32_t i = 0; + /* + * If it is an <ALTER TABLE ADD COLUMN>, then we have to + * create all indexes added by this statement. These + * indexes are in the array, starting with old index_count + * (inside space object) and ending with new index_count + * (inside ephemeral space). + */ + 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, @@ -1176,6 +1341,20 @@ vdbe_emit_create_constraints(struct Parse *parse, int reg_space_id) sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id); int reg_seq_rec = emitNewSysSequenceRecord(parse, reg_seq_id, space->def->name); + if (is_alter) { + int errcode = ER_SQL_CANT_ADD_AUTOINC; + const char *error_msg = + 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) != 0) + return; + } sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_rec); /* Do an insertion into _space_sequence. */ int reg_space_seq_record = @@ -1874,24 +2053,28 @@ sql_create_foreign_key(struct Parse *parse_context) char *parent_name = NULL; char *constraint_name = NULL; bool is_self_referenced = false; + struct space *space = parse_context->create_column_def.space; struct create_table_def *table_def = &parse_context->create_table_def; - struct space *space = table_def->new_space; + if (space == NULL) + space = table_def->new_space; /* - * Space under construction during CREATE TABLE - * processing. NULL for ALTER TABLE statement handling. + * Space under construction during <CREATE TABLE> + * processing or shallow copy of space during <ALTER TABLE + * ... ADD COLUMN>. NULL for <ALTER TABLE ... ADD + * CONSTRAINT> statement handling. */ - bool is_alter = space == NULL; + bool is_alter_add_constr = space == NULL; uint32_t child_cols_count; struct ExprList *child_cols = create_fk_def->child_cols; if (child_cols == NULL) { - assert(!is_alter); + assert(!is_alter_add_constr); child_cols_count = 1; } else { child_cols_count = child_cols->nExpr; } struct ExprList *parent_cols = create_fk_def->parent_cols; struct space *child_space = NULL; - if (is_alter) { + if (is_alter_add_constr) { const char *child_name = alter_def->entity_name->a[0].zName; child_space = space_by_name(child_name); if (child_space == NULL) { @@ -1909,6 +2092,12 @@ sql_create_foreign_key(struct Parse *parse_context) goto tnt_error; } memset(fk_parse, 0, sizeof(*fk_parse)); + /* + * Child space already exists if it is + * <ALTER TABLE ADD COLUMN>. + */ + if (table_def->new_space == NULL) + child_space = space; rlist_add_entry(&parse_context->create_fkeys_def.fkeys, fk_parse, link); } @@ -1922,27 +2111,41 @@ 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 = !is_alter && + is_self_referenced = !is_alter_add_constr && strcmp(parent_name, space->def->name) == 0; struct space *parent_space = space_by_name(parent_name); - if (parent_space == NULL) { - if (is_self_referenced) { - struct rlist *fkeys = - &parse_context->create_fkeys_def.fkeys; - struct fk_constraint_parse *fk = - rlist_first_entry(fkeys, - struct fk_constraint_parse, - link); - fk->selfref_cols = parent_cols; - fk->is_self_referenced = true; - } else { - diag_set(ClientError, ER_NO_SUCH_SPACE, parent_name);; - goto tnt_error; - } + if (parent_space == NULL && !is_self_referenced) { + diag_set(ClientError, ER_NO_SUCH_SPACE, parent_name); + goto tnt_error; + } + if (is_self_referenced) { + struct rlist *fkeys = &parse_context->create_fkeys_def.fkeys; + struct fk_constraint_parse *fk = + rlist_first_entry(fkeys, struct fk_constraint_parse, + link); + fk->selfref_cols = parent_cols; + fk->is_self_referenced = true; } - if (!is_alter) { + if (!is_alter_add_constr) { if (create_def->name.n == 0) { uint32_t idx = ++parse_context->create_fkeys_def.count; + /* + * If it is <ALTER TABLE ADD COLUMN> we + * should count the existing FK + * constraints in the space and form a + * name based on this. + */ + if (table_def->new_space == NULL) { + struct space *original_space = + space_by_name(space->def->name); + assert(original_space != NULL); + struct rlist *child_fk = + &original_space->child_fk_constraint; + struct fk_constraint *fk; + rlist_foreach_entry(fk, child_fk, + in_child_space) + idx++; + } constraint_name = sqlMPrintf(db, "fk_unnamed_%s_%u", space->def->name, idx); } else { @@ -2024,7 +2227,7 @@ sql_create_foreign_key(struct Parse *parse_context) constraint_name) != 0) { goto exit_create_fk; } - if (!is_alter) { + if (!is_alter_add_constr) { if (child_cols == NULL) { assert(i == 0); /* @@ -2053,12 +2256,14 @@ sql_create_foreign_key(struct Parse *parse_context) memcpy(fk_def->name, constraint_name, name_len); fk_def->name[name_len] = '\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 - * maintain list of all FK constraints inside parser. + * In case of <СREATE TABLE> and <ALTER TABLE ADD COLUMN> + * processing, all foreign keys constraints must be + * created after space itself (or space altering), so let + * delay it until vdbe_emit_create_constraints() call and + * simply maintain list of all FK constraints inside + * parser. */ - if (!is_alter) { + if (!is_alter_add_constr) { struct rlist *fkeys = &parse_context->create_fkeys_def.fkeys; struct fk_constraint_parse *fk_parse = rlist_first_entry(fkeys, struct fk_constraint_parse, @@ -2435,7 +2640,10 @@ sql_create_index(struct Parse *parse) { * Find the table that is to be indexed. * Return early if not found. */ - struct space *space = NULL; + struct space *space = parse->create_table_def.new_space; + if (space == NULL) + space = parse->create_column_def.space; + bool is_create_table_or_add_col = space != NULL; struct Token token = create_entity_def->name; if (tbl_name != NULL) { assert(token.n > 0 && token.z != NULL); @@ -2448,10 +2656,8 @@ sql_create_index(struct Parse *parse) { } goto exit_create_index; } - } else { - if (parse->create_table_def.new_space == NULL) - goto exit_create_index; - space = parse->create_table_def.new_space; + } else if (!is_create_table_or_add_col) { + goto exit_create_index; } struct space_def *def = space->def; @@ -2486,7 +2692,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 == NULL) { + if (!is_create_table_or_add_col) { assert(token.z != NULL); name = sql_name_from_token(db, &token); if (name == NULL) { @@ -2652,7 +2858,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 != NULL) { + if (is_create_table_or_add_col) { for (uint32_t i = 0; i < space->index_count; ++i) { struct index *existing_idx = space->index[i]; uint32_t iid = existing_idx->def->iid; @@ -2740,7 +2946,8 @@ sql_create_index(struct Parse *parse) { sqlVdbeAddOp0(vdbe, OP_Expire); } - if (tbl_name != NULL || sql_space_add_index(parse, space, index) != 0) + if (!is_create_table_or_add_col || + sql_space_add_index(parse, space, index) != 0) goto exit_create_index; index = NULL; diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index da4e2a3ae..d7c65bb4b 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -228,19 +228,24 @@ create_table_end ::= . { sqlEndTable(pParse); } */ columnlist ::= columnlist COMMA tcons. -columnlist ::= columnlist COMMA columnname carglist autoinc(I). { - uint32_t fieldno = pParse->create_table_def.new_space->def->field_count - 1; - if (I == 1 && sql_add_autoincrement(pParse, fieldno) != 0) - return; +columnlist ::= columnlist COMMA column_def create_column_end. +columnlist ::= column_def create_column_end. + +column_def ::= column_name_and_type carglist. + +column_name_and_type ::= nm(A) typedef(Y). { + create_column_def_init(&pParse->create_column_def, NULL, &A, &Y); + sql_create_column_start(pParse); } -columnlist ::= columnname carglist autoinc(I). { - uint32_t fieldno = pParse->create_table_def.new_space->def->field_count - 1; +create_column_end ::= autoinc(I). { + uint32_t fieldno = pParse->create_column_def.space->def->field_count - 1; if (I == 1 && sql_add_autoincrement(pParse, fieldno) != 0) return; + if (pParse->create_table_def.new_space == NULL) + sql_create_column_end(pParse); } columnlist ::= tcons. -columnname(A) ::= nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);} // An IDENTIFIER can be a generic identifier, or one of several // keywords. Any non-standard keyword can also be an identifier. @@ -283,9 +288,11 @@ nm(A) ::= id(A). { } } -// "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 <CREATE TABLE> + * or <ALTER TABLE ADD COLUMN> statement. + */ carglist ::= carglist ccons. carglist ::= . %type cconsname { struct Token } @@ -1737,11 +1744,30 @@ alter_table_start(A) ::= ALTER TABLE fullname(T) . { A = T; } %type alter_add_constraint {struct alter_args} alter_add_constraint(A) ::= alter_table_start(T) ADD CONSTRAINT nm(N). { + A.table_name = T; + A.name = N; + pParse->initiateTTrans = true; + } + +%type alter_add_column {struct alter_args} +alter_add_column(A) ::= alter_table_start(T) ADD column_name(N). { A.table_name = T; A.name = N; pParse->initiateTTrans = true; } +column_name(N) ::= COLUMN nm(A). { N = A; } +column_name(N) ::= nm(A). { N = A; } + +cmd ::= alter_column_def carglist create_column_end. + +alter_column_def ::= alter_add_column(N) typedef(Y). { + create_column_def_init(&pParse->create_column_def, N.table_name, &N.name, &Y); + create_checks_def_init(&pParse->create_checks_def); + create_fkeys_def_init(&pParse->create_fkeys_def); + sql_create_column_start(pParse); +} + cmd ::= 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 21829b6f0..07230a8be 100644 --- a/src/box/sql/parse_def.h +++ b/src/box/sql/parse_def.h @@ -107,9 +107,10 @@ struct fk_constraint_parse { */ struct fk_constraint_def *fk_def; /** - * If inside CREATE TABLE statement we want to declare - * self-referenced FK constraint, we must delay their - * resolution until the end of parsing of all columns. + * If inside <CREATE TABLE> or <ALTER TABLE ADD COLUMN> + * statement we want to declare self-referenced FK + * constraint, we must delay their resolution until the + * end of parsing of all columns. * E.g.: CREATE TABLE t1(id REFERENCES t1(b), b); */ struct ExprList *selfref_cols; @@ -154,6 +155,7 @@ enum sql_index_type { enum entity_type { ENTITY_TYPE_TABLE = 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; }; +struct create_column_def { + struct create_entity_def base; + /** Shallow space copy. */ + struct space *space; + /** Column type. */ + struct type_def *type_def; +}; + struct create_checks_def { /** List of ck_constraint_parse objects. */ struct rlist checks; @@ -478,6 +488,16 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name, if_not_exists); } +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 = type_def; +} + static inline void create_checks_def_init(struct create_checks_def *checks_def) { diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 2e426600d..a54aa1d51 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2251,20 +2251,24 @@ 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 <CREATE TABLE> or + * <ALTER TABLE ADD COLUMN> 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 <CREATE TABLE>. + * FK and CK constraints appeared in a <CREATE TABLE> or + * an <ALTER TABLE ADD COLUMN> statement. */ struct create_fkeys_def create_fkeys_def; struct create_checks_def create_checks_def; /* - * True, if column within a <CREATE TABLE> statement to be - * created has <AUTOINCREMENT>. + * True, if column in a <CREATE TABLE> or an + * <ALTER TABLE ADD COLUMN> statement to be created has + * <AUTOINCREMENT>. */ bool has_autoinc; /* Id of field with <AUTOINCREMENT>. */ @@ -2858,15 +2862,30 @@ 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_column_def. If it is <ALTER TABLE> 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); /** * 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 <CREATE TABLE> or a <ALTER TABLE ADD COLUMN> + * 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/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 5404a78e9..0899cf23e 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -40,6 +40,7 @@ #include <unicode/uchar.h> #include "box/session.h" +#include "box/schema.h" #include "say.h" #include "sqlInt.h" #include "tarantoolInt.h" @@ -431,8 +432,8 @@ sql_token(const char *z, int *type, bool *is_reserved) /** * This function is called to release parsing artifacts - * during table creation. The only objects allocated using - * malloc are index defs and check constraints. + * during table creation or column addition. The only objects + * allocated using malloc are index defs. * Note that this functions can't be called on ordinary * space object. It's purpose is to clean-up parser->new_space. * @@ -445,7 +446,15 @@ parser_space_delete(struct sql *db, struct space *space) if (space == NULL || db == NULL) return; assert(space->def->opts.is_ephemeral); - for (uint32_t i = 0; i < space->index_count; ++i) + struct space *altered_space = space_by_name(space->def->name); + uint32_t i = 0; + /* + * Don't delete already existing defs and start from new + * ones. + */ + if (altered_space != NULL) + i = altered_space->index_count; + for (; i < space->index_count; ++i) index_def_delete(space->index[i]->def); } @@ -539,7 +548,7 @@ sqlRunParser(Parse * pParse, const char *zSql) sqlVdbeDelete(pParse->pVdbe); pParse->pVdbe = 0; } - parser_space_delete(db, pParse->create_table_def.new_space); + parser_space_delete(db, pParse->create_column_def.space); if (pParse->pWithToFree) sqlWithDelete(db, pParse->pWithToFree); diff --git a/test/box/error.result b/test/box/error.result index 4d85b9e55..2f6d7fb22 100644 --- a/test/box/error.result +++ b/test/box/error.result @@ -439,6 +439,7 @@ t; | 218: box.error.TUPLE_METADATA_IS_TOO_BIG | 219: box.error.XLOG_GAP | 220: box.error.TOO_EARLY_SUBSCRIBE + | 221: 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..924368ea2 --- /dev/null +++ b/test/sql/add-column.result @@ -0,0 +1,529 @@ +-- test-run result file version 2 +-- +-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> 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: 1 + | ... + +-- +-- 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 c INT; + | --- + | - null + | - 'Can''t modify space ''V'': view can not be altered' + | ... + +\set language lua + | --- + | - true + | ... +view = box.space._space.index[2]:select('V')[1]:totable() + | --- + | ... +view_format = view[7] + | --- + | ... +f = {type = 'string', nullable_action = 'none', name = 'C', is_nullable = true} + | --- + | ... +table.insert(view_format, f) + | --- + | ... +view[5] = 3 + | --- + | ... +view[7] = view_format + | --- + | ... +box.space._space:replace(view) + | --- + | - error: 'Can''t modify space ''V'': view can not be altered' + | ... +\set language sql + | --- + | - true + | ... + +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: 1 + | ... +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: 1 + | ... +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: 1 + | ... +INSERT INTO ck_check VALUES (1, 0); + | --- + | - null + | - 'Check constraint failed ''ck_unnamed_CK_CHECK_1'': b > 0' + | ... +INSERT INTO ck_check VALUES (1, 1); + | --- + | - row_count: 1 + | ... +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: 1 + | ... +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' + | ... +INSERT INTO t1 VALUES (1, 1); + | --- + | - row_count: 1 + | ... +INSERT INTO fk_check VALUES (2, 1); + | --- + | - row_count: 1 + | ... +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: 1 + | ... +INSERT INTO self(a,b) VALUES(1, 1); + | --- + | - autoincrement_ids: + | - 1 + | row_count: 1 + | ... +UPDATE self SET a = 2, b = 2; + | --- + | - row_count: 1 + | ... +UPDATE self SET b = 3; + | --- + | - null + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed' + | ... +UPDATE self SET a = 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: 1 + | ... +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: space AUTOINC_CHECK can''t feature more than one AUTOINCREMENT + | 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: 1 + | ... +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: 1 + | ... +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: 1 + | ... +INSERT INTO null_check(a) VALUES (1); + | --- + | - row_count: 1 + | ... +SELECT * FROM null_check; + | --- + | - metadata: + | - name: A + | type: integer + | - name: B + | type: string + | rows: + | - [1, null] + | ... +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: 1 + | ... +INSERT INTO notnull_check(a) VALUES (1); + | --- + | - null + | - 'Failed to execute SQL statement: NOT NULL constraint failed: NOTNULL_CHECK.B' + | ... +INSERT INTO notnull_check VALUES (1, 'not null'); + | --- + | - row_count: 1 + | ... +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 + | ... +_ = box.schema.space.create('WITHOUT_FORMAT') + | --- + | ... +\set language sql + | --- + | - true + | ... +ALTER TABLE without_format ADD a INT PRIMARY KEY; + | --- + | - row_count: 1 + | ... +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 = box.schema.space.create('WITH_FORMAT') + | --- + | ... +with_format:format{{name = 'A', type = 'unsigned'}} + | --- + | ... +\set language sql + | --- + | - true + | ... +ALTER TABLE with_format ADD b INT PRIMARY KEY; + | --- + | - row_count: 1 + | ... +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..430c7c46a --- /dev/null +++ b/test/sql/add-column.test.sql @@ -0,0 +1,183 @@ +-- +-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> 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 c INT; + +\set language lua +view = box.space._space.index[2]:select('V')[1]:totable() +view_format = view[7] +f = {type = 'string', nullable_action = 'none', name = 'C', is_nullable = true} +table.insert(view_format, f) +view[5] = 3 +view[7] = view_format +box.space._space:replace(view) +\set language sql + +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); +INSERT INTO ck_check VALUES (1, 1); +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); +INSERT INTO t1 VALUES (1, 1); +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 = 2, b = 2; +UPDATE self SET b = 3; +UPDATE self SET a = 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); +SELECT * FROM null_check; +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); +INSERT INTO notnull_check VALUES (1, 'not null'); +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 +_ = 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 = box.schema.space.create('WITH_FORMAT') +with_format:format{{name = 'A', type = '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..3ddd52d42 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 +-- <ALTER TABLE ADD COLUMN>. +-- +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: 1 +... +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") +-- +-- gh-3075: Check the auto naming of CHECK constraints in +-- <ALTER TABLE ADD COLUMN>. +-- +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..9ba995579 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 +-- <ALTER TABLE ADD COLUMN>. +-- +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: 1 +... +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=1') 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() +-- +-- gh-3075: Check the auto naming of FOREIGN KEY constraints in +-- <ALTER TABLE ADD COLUMN>. +-- +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=1')
next prev parent reply other threads:[~2020-11-10 12:18 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-09 13:45 [Tarantool-patches] [PATCH v4 0/5] Support " Roman Khabibov 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 1/5] sql: rename TK_COLUMN to TK_COLUMN_REF Roman Khabibov 2020-11-05 22:17 ` Nikita Pettik 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse Roman Khabibov 2020-11-05 22:17 ` Nikita Pettik 2020-11-10 12:17 ` Roman Khabibov 2020-11-17 20:23 ` Nikita Pettik 2020-11-15 11:51 ` roman 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 3/5] schema: add box_space_field_MAX Roman Khabibov 2020-11-05 22:18 ` Nikita Pettik 2020-10-09 13:45 ` [Tarantool-patches] [PATCH v4 4/5] sql: use parser's region of "index" array Roman Khabibov 2020-11-05 22:26 ` Nikita Pettik 2020-11-10 12:18 ` Roman Khabibov 2020-11-17 20:35 ` Nikita Pettik [not found] ` <20201009134529.13212-6-roman.habibov@tarantool.org> 2020-11-06 15:57 ` [Tarantool-patches] [PATCH v4 5/5] sql: support column addition Nikita Pettik 2020-11-10 12:18 ` Roman Khabibov [this message] 2020-11-18 16:15 ` Nikita Pettik 2020-11-15 11:51 ` roman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=8AFFA6D9-CCF7-4134-9018-0C6389542ADE@tarantool.org \ --to=roman.habibov@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v4 5/5] sql: support column addition' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox