From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 8755944643C for ; Sat, 12 Sep 2020 00:51:44 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\)) From: Roman Khabibov In-Reply-To: <41180845-f742-d78d-c692-2e4244705319@tarantool.org> Date: Sat, 12 Sep 2020 00:51:42 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20200811003338.45084-1-roman.habibov@tarantool.org> <20200811003338.45084-5-roman.habibov@tarantool.org> <41180845-f742-d78d-c692-2e4244705319@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3 4/4] sql: support column addition List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. > On Aug 20, 2020, at 01:20, Vladislav Shpilevoy = wrote: >=20 > Thanks for the patch! >=20 > See 20 comments below. >=20 >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index 9013bc86f..6f3d2747d 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -285,48 +285,113 @@ sql_field_retrieve(Parse *parser, struct = space_def *space_def, uint32_t id) >> return field; >> } >>=20 >> -/* >> - * Add a new column to the table currently being constructed. >> +/** >> + * Make shallow copy of @a space on region. >> * >> - * The parser calls this routine once for each column declaration >> - * in a CREATE TABLE statement. sqlStartTable() gets called >> - * first to get things going. Then this routine is called for each >> - * column. >> + * Function is used to add a new column to an existing space with >> + * statement. Copy space def and index >> + * array to create constraints appeared in the statement. The >> + * index array copy will be modified by adding new elements to it. >> + * It is necessary, because the statement may contain several >> + * index definitions (constraints). >> */ >> +static struct space * >> +sql_shallow_space_copy(struct Parse *parse, struct space *space) >> +{ >> + assert(space->def !=3D NULL); >> + struct space *ret =3D sql_ephemeral_space_new(parse, = space->def->name); >> + if (ret =3D=3D NULL) >> + return NULL; >> + ret->index_count =3D space->index_count; >> + ret->index_id_max =3D space->index_id_max; >> + uint32_t indexes_sz =3D sizeof(struct index *) * = (ret->index_count); >> + ret->index =3D (struct index **) malloc(indexes_sz); >=20 > 1. Why can't you use parser's region? I added the patch. See it in the new patch set version. >> + if (ret->index =3D=3D NULL) { >> + diag_set(OutOfMemory, indexes_sz, "realloc", = "ret->index"); >=20 > 2. It is not realloc, it is malloc. >=20 > 3. Seems you need to set parser->is_aborted =3D true in case of an = error. As > far as I understand, it is a contract of each function taking Parse > argument. Done. +static struct space * +sql_shallow_space_copy(struct Parse *parse, struct space *space) +{ + assert(space->def !=3D NULL); + struct space *ret =3D sql_ephemeral_space_new(parse, = space->def->name); + if (ret =3D=3D NULL) + goto error; + ret->index_count =3D space->index_count; + ret->index_id_max =3D space->index_id_max; + size_t size =3D 0; + ret->index =3D region_alloc_array(&parse->region, typeof(struct = index *), + ret->index_count, &size); + if (ret->index =3D=3D 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 =3D true; + ret->def->opts.is_ephemeral =3D true; + if (ret->def->field_count !=3D 0) { + uint32_t fields_size =3D 0; + ret->def->fields =3D + region_alloc_array(&parse->region, + typeof(struct field_def), + ret->def->field_count, = &fields_size); + if (ret->def->fields =3D=3D NULL) { + diag_set(OutOfMemory, fields_size, = "region_alloc", + "ret->def->fields"); + goto error; + } + memcpy(ret->def->fields, space->def->fields, = fields_size); + } + + return ret; +error: + parse->is_aborted =3D true; + return NULL; +} >> + return NULL; >> + } >> + memcpy(ret->index, space->index, indexes_sz); >> + memcpy(ret->def, space->def, sizeof(struct space_def)); >> + ret->def->opts.is_temporary =3D true; >> + ret->def->opts.is_ephemeral =3D true; >> + if (ret->def->field_count !=3D 0) { >> + uint32_t fields_size =3D 0; >> + ret->def->fields =3D >> + region_alloc_array(&parse->region, >> + typeof(struct field_def), >> + ret->def->field_count, = &fields_size); >> + if (ret->def->fields =3D=3D NULL) { >> + diag_set(OutOfMemory, fields_size, = "region_alloc", >> + "ret->def->fields"); >> + free(ret->index); >> + return NULL; >> + } >> + memcpy(ret->def->fields, space->def->fields, = fields_size); >> + } >> + >> + return ret; >> +} >> @@ -334,18 +399,86 @@ sqlAddColumn(Parse * pParse, Token * pName, = struct type_def *type_def) >> +void >> +sql_create_column_end(struct Parse *parse) >> +{ >> + struct space *space =3D parse->create_column_def.space; >> + assert(space !=3D NULL); >> + struct space_def *def =3D space->def; >> + struct field_def *field =3D &def->fields[def->field_count - 1]; >> + if (field->nullable_action =3D=3D ON_CONFLICT_ACTION_DEFAULT) { >> + field->nullable_action =3D ON_CONFLICT_ACTION_NONE; >> + field->is_nullable =3D true; >> + } >> + /* >> + * Encode the format array and emit code to update _space. >> + */ >> + uint32_t table_stmt_sz =3D 0; >> + struct region *region =3D &parse->region; >> + char *table_stmt =3D sql_encode_table(region, def, = &table_stmt_sz); >> + char *raw =3D sqlDbMallocRaw(parse->db, table_stmt_sz); >> + if (table_stmt =3D=3D NULL || raw =3D=3D NULL) { >> + parse->is_aborted =3D true; >> + return; >> + } >> + memcpy(raw, table_stmt, table_stmt_sz); >> + >> + struct Vdbe *v =3D sqlGetVdbe(parse); >> + assert(v !=3D NULL); >> + >> + struct space *system_space =3D space_by_id(BOX_SPACE_ID); >> + assert(system_space !=3D NULL); >> + int cursor =3D parse->nTab++; >> + vdbe_emit_open_cursor(parse, cursor, 0, system_space); >> + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); >> + >> + int key_reg =3D ++parse->nMem; >> + sqlVdbeAddOp2(v, OP_Integer, def->id, key_reg); >> + int addr =3D sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, = 1); >> + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT); >> + sqlVdbeJumpHere(v, addr); >> + >> + int tuple_reg =3D sqlGetTempRange(parse, box_space_field_MAX + = 1); >=20 > 4. You need to call sqlReleaseTempRange() somewhere. Done. >> + for (int i =3D 0; i < box_space_field_MAX - 1; ++i) >> + sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i); >> + sqlVdbeAddOp1(v, OP_Close, cursor); >> + >> + sqlVdbeAddOp2(v, OP_Integer, def->field_count, tuple_reg + 4); >> + sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, tuple_reg + 6, >> + SQL_SUBTYPE_MSGPACK, raw, P4_DYNAMIC); >> + sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, box_space_field_MAX, >> + tuple_reg + box_space_field_MAX); >> + sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + box_space_field_MAX, = 0, 0, >> + (char *) system_space, P4_SPACEPTR); >> + sql_vdbe_create_constraints(parse, key_reg); >> + >> + /* >> + * Clean up array allocated in sql_shallow_space_copy(). >> + */ >> + free(space->index); >=20 > 5. It may happen that sql_create_column_end() is never called. For > example, if an error is encountered somewhere inside column = definition, > after sql_create_column_start() is called. Removed with the region usage. I also added the constants to AddOp functions. +void +sql_create_column_end(struct Parse *parse) +{ + struct space *space =3D parse->create_column_def.space; + assert(space !=3D NULL); + struct space_def *def =3D space->def; + struct field_def *field =3D &def->fields[def->field_count - 1]; + if (field->nullable_action =3D=3D ON_CONFLICT_ACTION_DEFAULT) { + field->nullable_action =3D ON_CONFLICT_ACTION_NONE; + field->is_nullable =3D true; + } + /* + * Encode the format array and emit code to update _space. + */ + uint32_t table_stmt_sz =3D 0; + struct region *region =3D &parse->region; + char *table_stmt =3D sql_encode_table(region, def, = &table_stmt_sz); + char *raw =3D sqlDbMallocRaw(parse->db, table_stmt_sz); + if (table_stmt =3D=3D NULL || raw =3D=3D NULL) { + parse->is_aborted =3D true; + return; + } + memcpy(raw, table_stmt, table_stmt_sz); + + struct Vdbe *v =3D sqlGetVdbe(parse); + assert(v !=3D NULL); + + struct space *system_space =3D space_by_id(BOX_SPACE_ID); + assert(system_space !=3D NULL); + int cursor =3D parse->nTab++; + vdbe_emit_open_cursor(parse, cursor, 0, system_space); + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); + + int key_reg =3D ++parse->nMem; + sqlVdbeAddOp2(v, OP_Integer, def->id, key_reg); + int addr =3D sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, = 1); + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT); + sqlVdbeJumpHere(v, addr); + + int tuple_reg =3D sqlGetTempRange(parse, box_space_field_MAX + = 1); + for (int i =3D 0; i < box_space_field_MAX - 1; ++i) + sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i); + sqlVdbeAddOp1(v, OP_Close, cursor); + + sqlVdbeAddOp2(v, OP_Integer, def->field_count, + tuple_reg + 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 *) system_space, P4_SPACEPTR); + sqlVdbeCountChanges(v); + sqlVdbeChangeP5(v, OPFLAG_NCHANGE); + sqlReleaseTempRange(parse, tuple_reg, box_space_field_MAX + 1); + sql_vdbe_create_constraints(parse, key_reg); } >> } >>=20 >> void >> sql_column_add_nullable_action(struct Parse *parser, >> enum on_conflict_action nullable_action) >> { >> - struct space *space =3D parser->create_table_def.new_space; >> - if (space =3D=3D NULL || NEVER(space->def->field_count < 1)) >> + assert(parser->create_column_def.space !=3D NULL); >> + struct space_def *def =3D parser->create_column_def.space->def; >> + if (NEVER(def->field_count < 1)) >> return; >> - struct space_def *def =3D space->def; >> struct field_def *field =3D &def->fields[def->field_count - 1]; >> if (field->nullable_action !=3D ON_CONFLICT_ACTION_DEFAULT && >> nullable_action !=3D field->nullable_action) { >> @@ -364,51 +497,42 @@ sql_column_add_nullable_action(struct Parse = *parser, >> } >>=20 >> /* >> - * The expression is the default value for the most recently added = column >> - * of the table currently under construction. >> + * The expression is the default value for the most recently added >> + * column. >> * >> * Default value expressions must be constant. Raise an exception if = this >> * is not the case. >> * >> * This routine is called by the parser while in the middle of >> - * parsing a CREATE TABLE statement. >> + * parsing a or a >> + * statement. >> */ >> void >> sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan) >> { >> sql *db =3D pParse->db; >> - struct space *p =3D pParse->create_table_def.new_space; >> - if (p !=3D NULL) { >> - assert(p->def->opts.is_ephemeral); >> - struct space_def *def =3D p->def; >> - if (!sqlExprIsConstantOrFunction >> - (pSpan->pExpr, db->init.busy)) { >> - const char *column_name =3D >> - def->fields[def->field_count - 1].name; >> - diag_set(ClientError, ER_CREATE_SPACE, = def->name, >> - tt_sprintf("default value of column = '%s' is "\ >> - "not constant", = column_name)); >> + assert(pParse->create_column_def.space !=3D NULL); >> + struct space_def *def =3D pParse->create_column_def.space->def; >> + struct field_def *field =3D &def->fields[def->field_count - 1]; >> + if (!sqlExprIsConstantOrFunction(pSpan->pExpr, db->init.busy)) { >> + diag_set(ClientError, ER_CREATE_SPACE, def->name, >> + tt_sprintf("default value of column '%s' is not = " >> + "constant", field->name)); >> + pParse->is_aborted =3D true; >> + } else { >> + struct region *region =3D &pParse->region; >> + uint32_t default_length =3D (int)(pSpan->zEnd - = pSpan->zStart); >> + field->default_value =3D region_alloc(region, = default_length + 1); >> + if (field->default_value =3D=3D NULL) { >> + diag_set(OutOfMemory, default_length + 1, >> + "region_alloc", = "field->default_value"); >> pParse->is_aborted =3D true; >> - } else { >> - assert(def !=3D NULL); >> - struct field_def *field =3D >> - &def->fields[def->field_count - 1]; >> - struct region *region =3D &pParse->region; >> - uint32_t default_length =3D (int)(pSpan->zEnd - = pSpan->zStart); >> - field->default_value =3D region_alloc(region, >> - = default_length + 1); >> - if (field->default_value =3D=3D NULL) { >> - diag_set(OutOfMemory, default_length + = 1, >> - "region_alloc", >> - "field->default_value"); >> - pParse->is_aborted =3D true; >> - return; >> - } >> - strncpy(field->default_value, pSpan->zStart, >> - default_length); >> - field->default_value[default_length] =3D '\0'; >> + goto add_default_value_exit; >> } >> + strncpy(field->default_value, pSpan->zStart, = default_length); >> + field->default_value[default_length] =3D '\0'; >> } >> +add_default_value_exit: >> sql_expr_delete(db, pSpan->pExpr, false); >=20 > 6. Was it necessary to make so many changes? Wouldn't it work if you > would just replace >=20 > struct space *p =3D pParse->create_table_def.new_space; >=20 > with >=20 > struct space *p =3D pParse->create_column_def.space; >=20 > ? Yes. Done. void sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan) { sql *db =3D pParse->db; - struct space *p =3D pParse->create_table_def.new_space; + struct space *p =3D pParse->create_column_def.space; if (p !=3D NULL) { assert(p->def->opts.is_ephemeral); struct space_def *def =3D p->def; >> } >>=20 >> @@ -574,8 +700,10 @@ sql_create_check_contraint(struct Parse *parser) >> (struct alter_entity_def *) create_ck_def; >> assert(alter_def->entity_type =3D=3D ENTITY_TYPE_CK); >> (void) alter_def; >> - struct space *space =3D parser->create_table_def.new_space; >> - bool is_alter =3D space =3D=3D NULL; >> + struct space *space =3D parser->create_column_def.space; >> + if (space =3D=3D NULL) >> + space =3D parser->create_table_def.new_space; >=20 > 7. Why in some places you check create_table_def.new_space !=3D NULL > first, and in some places create_column_def.space !=3D NULL first? Is > the order important somehow? The order is not important. >> + bool is_alter_add_constr =3D space =3D=3D NULL; >> /* Prepare payload for ck constraint definition. */ >> struct region *region =3D &parser->region;> @@ -704,8 +845,7 @@ = sql_column_collation(struct space_def *def, uint32_t column, uint32_t = *coll_id) >> * >> * In cases mentioned above collation is fetched by id. >> */ >> - if (space =3D=3D NULL) { >> - assert(def->opts.is_ephemeral); >> + if (def->opts.is_ephemeral) { >=20 > 8. space_by_id() above is not needed, if the definition is ephemeral = now. It > can be moved below this condition so as not to call it when the result = is > not going to be used anyway. Yes. @@ -704,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 =3D=3D NULL) { - assert(def->opts.is_ephemeral); + if (def->opts.is_ephemeral) { assert(column < (uint32_t)def->field_count); *coll_id =3D def->fields[column].coll_id; struct coll_id *collation =3D coll_by_id(*coll_id); return collation !=3D NULL ? collation->coll : NULL; } + struct space *space =3D space_by_id(def->id); struct tuple_field *field =3D tuple_format_field(space->format, = column); *coll_id =3D field->coll_id; return field->coll; >> assert(column < (uint32_t)def->field_count); >> *coll_id =3D def->fields[column].coll_id; >> struct coll_id *collation =3D coll_by_id(*coll_id); >> @@ -1148,15 +1292,21 @@ resolve_link(struct Parse *parse_context, = const struct space_def *def, >>=20 >> /** >> * Emit code to create sequences, indexes, check and foreign key >> - * constraints appeared in . >> + * constraints appeared in or >> + * . >> */ >> static void >> sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id) >> { >> assert(reg_space_id !=3D 0); >> struct space *space =3D parse->create_table_def.new_space; >> - assert(space !=3D NULL); >> + bool is_alter =3D space =3D=3D NULL; >> uint32_t i =3D 0; >> + if (is_alter) { >> + space =3D parse->create_column_def.space; >> + i =3D space_by_name(space->def->name)->index_count; >=20 > 9. Why do you need the original space for that? = sql_shallow_space_copy() > copies index_count as well. Now, explained this step in the comment. /** * Emit code to create sequences, indexes, check and foreign key - * constraints appeared in . + * constraints appeared in or + * . */ static void sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id) { assert(reg_space_id !=3D 0); struct space *space =3D parse->create_table_def.new_space; - assert(space !=3D NULL); + bool is_alter =3D space =3D=3D NULL; uint32_t i =3D 0; + /* + * If it is an , 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 =3D parse->create_column_def.space; + i =3D space_by_name(space->def->name)->index_count; + } + assert(space !=3D NULL); >> + } >> + assert(space !=3D NULL); >> for (; i < space->index_count; ++i) { >> struct index *idx =3D space->index[i]; >> vdbe_emit_create_index(parse, space->def, idx->def, >> @@ -1908,6 +2077,8 @@ sql_create_foreign_key(struct Parse = *parse_context) >> goto tnt_error; >> } >> memset(fk_parse, 0, sizeof(*fk_parse)); >> + if (parse_context->create_column_def.space !=3D NULL) >> + child_space =3D space; >=20 > 10. Why? Fixed. @@ -1908,6 +2091,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 + * . + */ + if (table_def->new_space =3D=3D NULL) + child_space =3D space; >> rlist_add_entry(&parse_context->fkeys, fk_parse, link); >> } >> struct Token *parent =3D create_fk_def->parent_name; >> @@ -1920,28 +2091,45 @@ sql_create_foreign_key(struct Parse = *parse_context) >> struct space *parent_space =3D space_by_name(parent_name); >> - if (parent_space =3D=3D NULL) { >> - if (is_self_referenced) { >> - struct fk_constraint_parse *fk =3D >> - rlist_first_entry(&parse_context->fkeys, >> - struct = fk_constraint_parse, >> - link); >> - fk->selfref_cols =3D parent_cols; >> - fk->is_self_referenced =3D true; >> - } else { >> - diag_set(ClientError, ER_NO_SUCH_SPACE, = parent_name);; >> - goto tnt_error; >> - } >> + if (parent_space =3D=3D NULL && !is_self_referenced) { >> + diag_set(ClientError, ER_NO_SUCH_SPACE, parent_name); >> + goto tnt_error; >> + } >> + if (is_self_referenced) { >> + struct fk_constraint_parse *fk =3D >> + rlist_first_entry(&parse_context->fkeys, >> + struct fk_constraint_parse, >> + link); >> + fk->selfref_cols =3D parent_cols; >> + fk->is_self_referenced =3D true; >> } >=20 > ^^^ > 11. This refactoring seems unnecessary. What changed here = functionally? No. I need to execute this branch, when parent_space !=3D NULL (ADD = COLUMN case). Tests fail without this change. + if (is_self_referenced) { + struct fk_constraint_parse *fk =3D + = rlist_first_entry(&parse_context->fkeys_def.fkeys, + struct fk_constraint_parse, = link); + fk->selfref_cols =3D parent_cols; + fk->is_self_referenced =3D true; >> - if (!is_alter) { >> + if (!is_alter_add_constr) { >> if (create_def->name.n =3D=3D 0) { >> - constraint_name =3D >> - sqlMPrintf(db, "fk_unnamed_%s_%d", >> - space->def->name, >> - ++parse_context->fkey_count); >> + uint32_t idx =3D ++parse_context->fkey_count; >> + /* >> + * If it is we >> + * should count the existing FK >> + * constraints in the space and form a >> + * name based on this. >> + */ >> + if (table_def->new_space =3D=3D NULL) { >> + struct space *original_space =3D >> + space_by_name(space->def->name); >> + assert(original_space !=3D NULL); >> + struct rlist *child_fk =3D >> + = &original_space->child_fk_constraint; >> + if (!rlist_empty(child_fk)) { >=20 > 12. You don't need to check for emptiness. rlist_foreach_entry() works = fine > with an empty list. Ok. Removed. >> + struct fk_constraint *fk; >> + rlist_foreach_entry(fk, = child_fk, >> + = in_child_space) >> + idx++; >> + } >> + } >> + constraint_name =3D sqlMPrintf(db, = "fk_unnamed_%s_%d", >> + space->def->name, = idx); >> } else { >> constraint_name =3D >> sql_name_from_token(db, = &create_def->name); >> @@ -2001,7 +2189,8 @@ sql_create_foreign_key(struct Parse = *parse_context) >> } >> int actions =3D create_fk_def->actions; >> fk_def->field_count =3D child_cols_count; >> - fk_def->child_id =3D child_space !=3D NULL ? = child_space->def->id : 0; >> + fk_def->child_id =3D table_def->new_space =3D=3D NULL ? >> + child_space->def->id : 0; >=20 > 13. Why? Removed. >> fk_def->parent_id =3D parent_space !=3D NULL ? = parent_space->def->id : 0; >> fk_def->is_deferred =3D create_constr_def->is_deferred; >> fk_def->match =3D (enum fk_constraint_match) = (create_fk_def->match); >> @@ -2420,10 +2613,8 @@ sql_create_index(struct Parse *parse) { >> } >> goto exit_create_index; >> } >> - } else { >> - if (parse->create_table_def.new_space =3D=3D NULL) >> - goto exit_create_index; >> - space =3D parse->create_table_def.new_space; >> + } else if (space =3D=3D NULL) { >=20 > 14. Why not !is_create_table_or_add_col? Fixed. @@ -2439,10 +2646,8 @@ sql_create_index(struct Parse *parse) { } goto exit_create_index; } - } else { - if (parse->create_table_def.new_space =3D=3D NULL) - goto exit_create_index; - space =3D parse->create_table_def.new_space; + } else if (!is_create_table_or_add_col) { + goto exit_create_index; >> + goto exit_create_index; >> } >> struct space_def *def =3D space->def; >>=20 >> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y >> index 995875566..0c9887851 100644 >> --- a/src/box/sql/parse.y >> +++ b/src/box/sql/parse.y >> @@ -281,9 +286,11 @@ nm(A) ::=3D id(A). { >> } >> } >>=20 >> -// "carglist" is a list of additional constraints that come after = the >> -// column name and column type in a CREATE TABLE statement. >> -// >> +/* >=20 > 15. Out-of-function comments are started from /**. Fixed. >> + * "carglist" is a list of additional constraints and clauses that >> + * come after the column name and column type in a >> + * or statement. >> + */ >> carglist ::=3D carglist ccons. >> carglist ::=3D . >> %type cconsname { struct Token } >> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h >> index 1105fda6e..336914c57 100644 >> --- a/src/box/sql/parse_def.h >> +++ b/src/box/sql/parse_def.h >> @@ -207,6 +209,14 @@ struct create_table_def { >> struct space *new_space; >> }; >>=20 >> +struct create_column_def { >> + struct create_entity_def base; >> + /** Shallow space_def copy. */ >=20 > 16. Not space_def. Fixed. >> + struct space *space; >> + /** Column type. */ >> + struct type_def *type_def; >> +}; >> + >> struct create_view_def { >> struct create_entity_def base; >> /** >> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h >> index fa87e7bd2..32142a871 100644 >> --- a/src/box/sql/sqlInt.h >> +++ b/src/box/sql/sqlInt.h >> @@ -2860,15 +2864,30 @@ struct space *sqlResultSetOfSelect(Parse *, = Select *); >>=20 >> struct space * >> sqlStartTable(Parse *, Token *); >> -void sqlAddColumn(Parse *, Token *, struct type_def *); >> + >> +/** >> + * Add new field to the format of ephemeral space in >> + * create_table_def. If it is create shallow copy of >> + * the existing space and add field to its format. >=20 > 17. It fills create_column_def, not create_table_def. -void sqlAddColumn(Parse *, Token *, struct type_def *); + +/** + * Add new field to the format of ephemeral space in + * create_column_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); >> + */ >> +void >> +sql_create_column_start(struct Parse *parse); >> + >> diff --git a/test/sql/add-column.result b/test/sql/add-column.result >> new file mode 100644 >> index 000000000..f86259105 >> --- /dev/null >> +++ b/test/sql/add-column.result >> @@ -0,0 +1,471 @@ >> +-- test-run result file version 2 >> +-- >> +-- gh-3075: Check statement. >> +-- >> +CREATE TABLE t1 (a INT PRIMARY KEY); >> + | --- >> + | - row_count: 1 >> + | ... >> + >> +-- >> +-- COLUMN keyword is optional. Check it here, but omit it below. >> +-- >> +ALTER TABLE t1 ADD COLUMN b INT; >> + | --- >> + | - row_count: 0 >=20 > 18. It seems you need to return row_count 1. To be consistent with > other ALTER TABLE expressions. Done. I added opcode emission. >> + | ... >> + >> +-- >> +-- A column with the same name already exists. >> +-- >> +ALTER TABLE t1 ADD b SCALAR; >> + | --- >> + | - null >> + | - Space field 'B' is duplicate >> + | ... >> + >> +-- >> +-- Can't add column to a view. >> +-- >> +CREATE VIEW v AS SELECT * FROM t1; >> + | --- >> + | - row_count: 1 >> + | ... >> +ALTER TABLE v ADD b INT; >> + | --- >> + | - null >> + | - Can't add column 'B'. 'V' is a view >=20 > 19. What if I do the same via direct replace into _space in Lua? See the new patch in the patch set. >> + | ... >> +DROP VIEW v; >> + | --- >> + | - row_count: 1 >> + | ... >> + >> +-- >> +-- Check PRIMARY KEY constraint works with an added column. >> +-- >> +CREATE TABLE pk_check (a INT CONSTRAINT pk PRIMARY KEY); >> + | --- >> + | - row_count: 1 >> + | ... >> +ALTER TABLE pk_check DROP CONSTRAINT pk; >> + | --- >> + | - row_count: 1 >> + | ... >> +ALTER TABLE pk_check ADD b INT PRIMARY KEY; >> + | --- >> + | - row_count: 0 >> + | ... >> +INSERT INTO pk_check VALUES (1, 1); >> + | --- >> + | - row_count: 1 >> + | ... >> +INSERT INTO pk_check VALUES (1, 1); >> + | --- >> + | - null >> + | - Duplicate key exists in unique index 'pk_unnamed_PK_CHECK_1' in = space 'PK_CHECK' >> + | ... >> +DROP TABLE pk_check; >> + | --- >> + | - row_count: 1 >> + | ... >> + >> +-- >> +-- Check UNIQUE constraint works with an added column. >> +-- >> +CREATE TABLE unique_check (a INT PRIMARY KEY); >> + | --- >> + | - row_count: 1 >> + | ... >> +ALTER TABLE unique_check ADD b INT UNIQUE; >> + | --- >> + | - row_count: 0 >> + | ... >> +INSERT INTO unique_check VALUES (1, 1); >> + | --- >> + | - row_count: 1 >> + | ... >> +INSERT INTO unique_check VALUES (2, 1); >> + | --- >> + | - null >> + | - Duplicate key exists in unique index = 'unique_unnamed_UNIQUE_CHECK_2' in space 'UNIQUE_CHECK' >> + | ... >> +DROP TABLE unique_check; >> + | --- >> + | - row_count: 1 >> + | ... >> + >> +-- >> +-- Check CHECK constraint works with an added column. >> +-- >> +CREATE TABLE ck_check (a INT PRIMARY KEY); >> + | --- >> + | - row_count: 1 >> + | ... >> +ALTER TABLE ck_check ADD b INT CHECK (b > 0); >> + | --- >> + | - row_count: 0 >> + | ... >> +INSERT INTO ck_check VALUES (1, 0); >> + | --- >> + | - null >> + | - 'Check constraint failed ''ck_unnamed_CK_CHECK_1'': b > 0' >> + | ... >> +DROP TABLE ck_check; >> + | --- >> + | - row_count: 1 >> + | ... >> + >> +-- >> +-- Check FOREIGN KEY constraint works with an added column. >> +-- >> +CREATE TABLE fk_check (a INT PRIMARY KEY); >> + | --- >> + | - row_count: 1 >> + | ... >> +ALTER TABLE fk_check ADD b INT REFERENCES t1(a); >> + | --- >> + | - row_count: 0 >> + | ... >> +INSERT INTO fk_check VALUES (0, 1); >> + | --- >> + | - null >> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint = failed' >> + | ... >> +INSERT INTO fk_check VALUES (2, 0); >> + | --- >> + | - null >> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint = failed' >> + | ... >> +INSERT INTO fk_check VALUES (2, 1); >> + | --- >> + | - null >> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint = failed' >=20 > 20. It is worth adding one more test with a successfull insertion. +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 + | =E2=80=A6 +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] + | =E2=80=A6 +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 + | =E2=80=A6