[Tarantool-patches] [PATCH v2 2/2] sql: support column addition
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun Jul 12 19:45:46 MSK 2020
Hi! Thanks for the patch!
See 33 comments below.
> commit 82c448dc66f6233faeb40dda353652c2fd5a3d29
> Author: Roman Khabibov <roman.habibov at 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.
>
> 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
> + * <ALTER TABLE ADD COLUMN>. 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 <CREATE TABLE> or a <ALTER TABLE ADD COLUMN>
> + * 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 "<name> PRIMARY KEY COLLATE <type>",
> * 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 <CREATE TABLE> or
> + * <ALTER TABLE ADD COLUMN> 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 <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> or
> + * a <ALTER TABLE ADD COLUMN> statement.
> + */
> + struct rlist fkeys;
> + struct rlist checks;
> + uint32_t fkey_count;
> + uint32_t check_count;
> + /** True, if column to be created has <AUTOINCREMENT>. */
> + bool has_autoinc;
27. What column? This is struct Parse, it is not a column.
> + /** Id of field with <AUTOINCREMENT>. */
> + 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 <ALTER TABLE> 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 <ALTER TABLE> 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/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 <ALTER TABLE table ADD COLUMN column> 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?
More information about the Tarantool-patches
mailing list