From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 4F27D469719 for ; Wed, 16 Sep 2020 23:18:24 +0300 (MSK) Date: Wed, 16 Sep 2020 20:18:23 +0000 From: Nikita Pettik Message-ID: <20200916201823.GE10599@tarantool.org> References: <20200911215115.6622-1-roman.habibov@tarantool.org> <20200911215115.6622-7-roman.habibov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200911215115.6622-7-roman.habibov@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 6/6] sql: support column addition List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org On 12 Sep 00:51, Roman Khabibov wrote: > 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 Space emptyness is required by ansi or it is tarantool's restriction? In Postgres for instance, one can add column for non-empty table and it is considered to be ok (new column is filled with default value). We can use ephemeral tables as temporary holder while rebuilding space. Below some comments, I didn't review patch carefully tho. > diff --git a/src/box/errcode.h b/src/box/errcode.h > index 3c21375f5..ec16399a0 100644 > --- a/src/box/errcode.h > +++ b/src/box/errcode.h > @@ -271,6 +271,7 @@ struct errcode_record { > /*216 */_(ER_SYNC_QUORUM_TIMEOUT, "Quorum collection for a synchronous transaction is timed out") \ > /*217 */_(ER_SYNC_ROLLBACK, "A rollback for a synchronous transaction is received") \ > /*218 */_(ER_TUPLE_METADATA_IS_TOO_BIG, "Can't create tuple: metadata size %u is too big") \ > + /*219 */_(ER_SQL_CANT_ADD_AUTOINC, "Can't add AUTOINCREMENT: the space '%s' already has one auto incremented field") \ Consider reusing sql_add_autoincrement() func. Or re-phrase err msg and use it in sql_add_autoinc. For example: 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 d1d240315..677099b48 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) Why do you need whole space *? Def is likely to be enough. > +{ > } > size_t name_len = strlen(name); > @@ -634,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) { > sqlVdbeAddOp2(v, OP_Integer, idx_def->iid, entry_reg + 1); > } else { > @@ -1032,18 +1180,21 @@ 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 ALER -> ALTER Don't get why we don't have child id for ALTER TABLE... > + * statement, we don't have child id, 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); > } > + * constraints appeared in or > + * . > */ > static void > sql_vdbe_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 At most one index..? > + * 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, > 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 or + * COLUMN> processing, all foreign keys constraints must > + * be created after space itself, so lets delay it until Again: when we execute alter table, space itself must already exist.