Re: [Tarantool-patches] [PATCH v4 6/6] sql: support column addition

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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
>> <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
> 
> Space emptyness is required by ansi or it is tarantool's restriction?

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.

Ephemeral spaces can't be used at least because of vinyl, because may
not fit into the memory. With Roman we decided to implement non-empty
alter later as a separate issue.

>> 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.

> 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
>> + * <ALTER TABLE ADD COLUMN> 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.