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

[Nikita Pettik <korablev@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
> >> <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.

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

Hm, we need at most one index_def. Mb it is worth passing it
as a separate arg?