From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 8D21F4696C1 for ; Thu, 19 Nov 2020 02:06:07 +0300 (MSK) References: <20201009134529.13212-1-roman.habibov@tarantool.org> <20201009134529.13212-6-roman.habibov@tarantool.org> <20201106155719.GA13328@tarantool.org> <8AFFA6D9-CCF7-4134-9018-0C6389542ADE@tarantool.org> <20201118161519.GA23479@tarantool.org> From: roman Message-ID: <654174e6-35b5-0888-302b-1f710e81ab7a@tarantool.org> Date: Sun, 15 Nov 2020 14:51:06 +0300 MIME-Version: 1.0 In-Reply-To: <20201118161519.GA23479@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH v4 5/5] sql: support column addition List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. On 18.11.2020 19:15, Nikita Pettik wrote: > On 10 Nov 15:18, Roman Khabibov wrote: >>>> 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 > Oh, ok, I won't insist on this refactoring. But could you please > copy-paste referenced anwers from mailing list? Those letters quite > large so navigating through them takes a lot of time. I'd rather write down the discussion. At first, we wanted to create just struct space_def with a new column and store it as a field in struct create_column_def. The modified struct space_def is needed in the index_file_def() function. We didn'twant to rewrite this function. Then it turned out that we need to count indexesandadd them to the array inside struct space. sql_create_index() modifies this array during parsing of the statement. We found it easier to create ephemeral space thanto change the code for this function. Well, put ifs -- less work. Then, I noticedthat the opcode generation for creating constants after CREATE TABLE andALTER TABLE is the same, and decided to move the code from sqlTableEnd()to a separate function, so there was a patch for refactoring. >>>> + /* >>>> + * If it is an , 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 … > Well, ok. Just wondering why somebody might want to create several unique > constraints over the same field.. Ha ha, I agree, a strange case. But, unfortunately, it is possible. > Otherwise LGTM commit ace2be0b40775b0852160a6dcb0502e85e546e04 Author: Roman Khabibov Date:   Thu Jan 2 19:06:14 2020 +0300     sql: support column addition     Enable to add column to existing space with     statement. Column definition can be     supplemented with the four types of constraints, ,     clauses and <[NOT] NULL>, AUTOINCREMENT.     Closes #2349, #3075     @TarantoolBot document     Title: Add columns to existing tables in SQL     Now, it is possible to add columns to existing empty spaces using         statement. The column definition is the same as in     statement.     * 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 bacdad9..7480c02 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 e6957d6..cfea5bc 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 7c1e248..7e5ff0e 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 + * 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) -        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; -    } +      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 or an + * 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; @@ -448,6 +578,8 @@ sqlAddPrimaryKey(struct Parse *pParse)      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) {          diag_set(ClientError, ER_CREATE_SPACE, space->def->name, @@ -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,9 +723,23 @@ sql_create_check_contraint(struct Parse *parser)              return;          }      } else { -        assert(! is_alter); +        assert(!is_alter_add_constr);          uint32_t ck_idx =              ++parser->create_ck_constraint_parse_def.count; +        /* +         * If it is we should +         * count the existing CHECK constraints in the +         * space and form a name based on this. +         */ +        if (parser->create_table_def.new_space == 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); @@ -635,7 +783,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) { @@ -665,9 +813,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); @@ -697,7 +844,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. @@ -706,13 +852,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; @@ -796,7 +942,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 { @@ -1034,18 +1181,23 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,                P4_DYNAMIC);      /*       * In case we are adding FK constraints during execution -     * of statement, we don't have child -     * id, but we know register where it will be stored. +     * of or +     * statement, we don't have child id (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 { @@ -1109,7 +1261,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);      } @@ -1150,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 . + * constraints appeared in or + * .   */  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 , 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, @@ -1177,6 +1342,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 = @@ -1879,24 +2058,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 +     * processing or shallow copy of space during . NULL for 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) { @@ -1914,6 +2097,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 == NULL) +            child_space = space;          struct rlist *fkeys = &parse_context->create_fk_constraint_parse_def.fkeys;          rlist_add_entry(fkeys, fk_parse, link); @@ -1928,29 +2117,44 @@ 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 create_fk_constraint_parse_def *parse_def = - &parse_context->create_fk_constraint_parse_def; -            struct fk_constraint_parse *fk = -                rlist_first_entry(&parse_def->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_fk_constraint_parse_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) {              struct create_fk_constraint_parse_def *parse_def = &parse_context->create_fk_constraint_parse_def;              uint32_t idx = ++parse_def->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 == 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 { @@ -2032,7 +2236,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);                  /* @@ -2061,12 +2265,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 +     * 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_fk_constraint_parse_def.fkeys;          struct fk_constraint_parse *fk_parse = @@ -2445,7 +2651,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); @@ -2458,10 +2667,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; @@ -2496,7 +2703,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) { @@ -2662,7 +2869,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; @@ -2750,7 +2957,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 9e870c8..abc3639 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 + * or 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_ck_constraint_parse_def_init(&pParse->create_ck_constraint_parse_def); + create_fk_constraint_parse_def_init(&pParse->create_fk_constraint_parse_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 7ad7496..c5e0935 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 or +     * 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_ck_constraint_parse_def {      /** List of ck_constraint_parse_def objects. */      struct rlist checks; @@ -479,6 +489,16 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name,  }  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_ck_constraint_parse_def_init(struct create_ck_constraint_parse_def *def)  {      rlist_create(&def->checks); diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 6c79a86..95a6fed 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 or +     * statement (to pass it to +     * sqlEndTable() sql_create_column_end() function).       */      struct create_table_def create_table_def; +    struct create_column_def create_column_def;      /* -     * FK and CK constraints appeared in a . +     * FK and CK constraints appeared in a or +     * an statement.       */      struct create_fk_constraint_parse_def create_fk_constraint_parse_def;      struct create_ck_constraint_parse_def create_ck_constraint_parse_def;      /* -     * True, if column within a statement to be -     * created has . +     * True, if column in a or an +     * statement to be created has +     * .       */      bool has_autoinc;      /* Id of field with . */ @@ -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 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 or a + * statement. A "NOT NULL" constraint has been seen on a column. + * This routine sets the is_nullable flag on the column currently + * under construction. If nullable_action has been already set, + * this function raises an error.   *   * @param parser SQL Parser object.   * @param nullable_action on_conflict_action value. diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 5404a78..0899cf2 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -40,6 +40,7 @@  #include  #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 4d85b9e..2f6d7fb 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 0000000..924368e --- /dev/null +++ b/test/sql/add-column.result @@ -0,0 +1,529 @@ +-- 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: 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 0000000..430c7c4 --- /dev/null +++ b/test/sql/add-column.test.sql @@ -0,0 +1,183 @@ +-- +-- gh-3075: Check statement. +-- +CREATE TABLE t1 (a INT PRIMARY KEY); + +-- +-- COLUMN keyword is optional. Check it here, but omit it below. +-- +ALTER TABLE t1 ADD COLUMN b INT; + +-- +-- A column with the same name already exists. +-- +ALTER TABLE t1 ADD b SCALAR; + +-- +-- Can't add column to a view. +-- +CREATE VIEW v AS SELECT * FROM t1; +ALTER TABLE v ADD 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 7b18e5d..3ddd52d 100644 --- a/test/sql/checks.result +++ b/test/sql/checks.result @@ -856,6 +856,26 @@ box.execute("DROP TABLE t6")  ---  - row_count: 1  ... +-- +-- gh-3075: Check the auto naming of CHECK constraints in +-- . +-- +box.execute("CREATE TABLE check_naming (a INT PRIMARY KEY CHECK (a > 0))") +--- +- row_count: 1 +... +box.execute("ALTER TABLE check_naming ADD b INT CHECK (b > 0)") +--- +- row_count: 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 301f8ea..a791314 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 +-- . +-- +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 33689a0..9ba9955 100644 --- a/test/sql/foreign-keys.result +++ b/test/sql/foreign-keys.result @@ -499,5 +499,33 @@ box.space.S:drop()  box.space.T:drop()  ---  ... +-- +-- gh-3075: Check the auto naming of FOREIGN KEY constraints in +-- . +-- +box.execute("CREATE TABLE t1 (a INT PRIMARY KEY)") +--- +- row_count: 1 +... +box.execute("CREATE TABLE check_naming (a INT PRIMARY KEY REFERENCES t1(a))") +--- +- row_count: 1 +... +box.execute("ALTER TABLE check_naming ADD b INT REFERENCES t1(a)") +--- +- row_count: 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 d2dd88d..29918c5 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 +-- . +-- +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')