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

[Nikita Pettik <korablev@tarantool.org>]

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

> +{
>  	}
>  	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 <CREATE TABLE ...> statement, we don't have child
> -	 * id, but we know register where it will be stored.
> +	 * of <CREATE TABLE ...> or <ALER TABLE ADD COLUMN ...>

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 <CREATE TABLE> or
> + * <ALTER TABLE ADD COLUMN>.
>   */
>  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 <ALTER TABLE ADD COLUMN>, 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 <CREATE TABLE> or <ALTER TABLE ... ADD
> +	 * 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.