From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 C6B14469719 for ; Fri, 18 Sep 2020 15:59:02 +0300 (MSK) Date: Fri, 18 Sep 2020 12:59:01 +0000 From: Nikita Pettik Message-ID: <20200918125901.GL10599@tarantool.org> References: <20200911215115.6622-1-roman.habibov@tarantool.org> <20200911215115.6622-7-roman.habibov@tarantool.org> <20200916201823.GE10599@tarantool.org> <4b304f32-379a-fb50-f427-615a91646b7d@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <4b304f32-379a-fb50-f427-615a91646b7d@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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 17 Sep 17:19, Vladislav Shpilevoy wrote: > Thanks for the comments! I tried to answer some of them. > > On 16.09.2020 22:18, Nikita Pettik wrote: > > 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? > > It is Tarantool's restriction. I mean it would be nice to explain this in the doc request.. > > 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. > > Ephemeral spaces can't be used at least because of vinyl, because may > not fit into the memory. Anyway they are used for vinyl even now. > With Roman we decided to implement non-empty > alter later as a separate issue. Ok, then please create follow-up issue with proposed approach. > >> 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. > > We can't. sql_add_autoincrement() fails only if autoinc is added more than once > inside one statements. Here Roman tries to handle an error, when autoinc was > added earlier in another statement, so we have has_autoinc false in ADD COLUMN. > Even if autoinc was specified in CREATE TABLE some time ago. Just set parse_context->create_table_def.has_autoinc before func invocation to space->sequence_fieldno. > > 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. > > We need to store indexes somewhere. UNIQUE, for example. Space_def > does not have index definitions. Hm, we need at most one index_def. Mb it is worth passing it as a separate arg?