Re: [Tarantool-patches] [PATCH v2 2/2] sql: support column addition

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

Hi! Thanks for the patch!

See 33 comments below.

> commit 82c448dc66f6233faeb40dda353652c2fd5a3d29
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> Date:   Thu Jan 2 19:06:14 2020 +0300
> 
>     sql: support column addition
>     
>     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
>     <ALTER TABLE table_name ADD [COLUMN] column_name column_type ...>
>     statement. The column definition is the same as in <CREATE TABLE>
>     statement.
>     
>     For example:
>     
>     tarantool> box.execute([[CREATE TABLE test (
>                                a INTEGER PRIMARY KEY
>                           );]])
>     ---
>     - row_count: 1
>     ...
>     
>     tarantool> box.execute([[ALTER TABLE test ADD COLUMN
>                                b TEXT
>                                NOT NULL
>                                DEFAULT ('a')
>                                COLLATE "unicode_ci"
>                            ;]])
>     ---
>     - row_count: 0
>     ...
>     ---

1. The commit message is different from what I see on the branch. On the
branch there is no 'Closes #2349'. What is the most actual version?

2. The example from the doc request does not work:

	tarantool> box.execute([[CREATE TABLE test (a INTEGER PRIMARY KEY);]])
	---
	- row_count: 1
	...

	tarantool> box.execute([[ALTER TABLE test ADD COLUMN b TEXT NOT NULL DEFAULT ('a') COLLATE "unicode_ci";]])
	---
	- null
	- 'At line 1 at or near position 22: keyword ''COLUMN'' is reserved. Please use double
	  quotes if ''COLUMN'' is an identifier.'
	...

> diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
> index 486b6b30d..dea047241 100644
> --- a/extra/mkkeywordhash.c
> +++ b/extra/mkkeywordhash.c
> @@ -76,7 +76,7 @@ static Keyword aKeywordTable[] = {
>    { "CHECK",                  "TK_CHECK",       true  },
>    { "COLLATE",                "TK_COLLATE",     true  },
>    { "COLUMN_NAME",            "TK_COLUMN_NAME", true  },
> -  { "COLUMN",                 "TK_STANDARD",    true  },
> +  { "COLUMN",                 "TK_COLUMN",      true  },

3. This is how the same hunk looks on the branch:

====================
diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index 006285622..51c8cbb18 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -75,11 +75,7 @@ static Keyword aKeywordTable[] = {
   { "CAST",                   "TK_CAST",        false },
   { "CHECK",                  "TK_CHECK",       true  },
   { "COLLATE",                "TK_COLLATE",     true  },
-  /* gh-3075: Reserved until ALTER ADD COLUMN is implemeneted.
-   * Move it back to ALTER when done.
-   */
-  /* { "COLUMN",              "TK_COLUMNKW",    true  }, */
-  { "COLUMN",                 "TK_STANDARD",    true  },
+  { "COLUMN",                 "TK_COLUMN",      true  },
   { "COMMIT",                 "TK_COMMIT",      true  },
   { "CONFLICT",               "TK_CONFLICT",    false },
   { "CONSTRAINT",             "TK_CONSTRAINT",  true  },
====================

It is very different. Why?

> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index d1e4d02a9..3e94bee7a 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -266,6 +266,8 @@ struct errcode_record {
>  	/*211 */_(ER_WRONG_QUERY_ID,		"Prepared statement with id %u does not exist") \
>  	/*212 */_(ER_SEQUENCE_NOT_STARTED,		"Sequence '%s' is not started") \
>  	/*213 */_(ER_NO_SUCH_SESSION_SETTING,	"Session setting %s doesn't exist") \
> +	/*214 */_(ER_SQL_CANT_ADD_COLUMN_TO_VIEW,	"Can't add column '%s'. '%s' is a view") \
> +	/*215 */_(ER_SQL_CANT_ADD_AUTOINC,	"Can't add AUTOINCREMENT: the space '%s' already has one auto-incremented field") \

4. The same here:

====================
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -264,6 +264,7 @@ struct errcode_record {
        /*209 */_(ER_SESSION_SETTING_INVALID_VALUE,     "Session setting %s expected a value of type %s") \
        /*210 */_(ER_SQL_PREPARE,               "Failed to prepare SQL statement: %s") \
        /*211 */_(ER_WRONG_QUERY_ID,            "Prepared statement with id %u does not exist") \
+       /*212 */_(ER_SQL_CANT_ADD_COLUMN_TO_VIEW,       "Can't add column '%s' to the view '%s'") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
====================

From this point I will review the email, not the branch.
Generally the patch still looks very raw. I hope this is mostly
because I couldn't review it properly locally.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index ac42fe842..45fb90d38 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -285,48 +285,112 @@ 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 the existing space with
> + * <ALTER TABLE ADD COLUMN>. Copy info about indexes and
> + * definition to create constraints appeared in the statement.

5. I don't think I understood anything from the comment. Why is it needed
to create a copy (I remember why, a bit, but I mostly forgot it)?

>   */
> +static struct space *
> +sql_shallow_space_copy(struct Parse *parse, struct space *space)
> +{
> +	assert(space->def != NULL);
> +	struct space *ret = sql_ephemeral_space_new(parse, space->def->name);
> +	if (ret == NULL)
> +		return NULL;
> +	ret->index_count = space->index_count;
> +	ret->index_id_max = space->index_id_max;
> +	uint32_t indexes_sz = sizeof(struct index *) * (ret->index_count);
> +	ret->index = (struct index **) malloc(indexes_sz);

6. When is this array freed?

> +	if (ret->index == NULL) {
> +		diag_set(OutOfMemory, indexes_sz, "realloc", "ret->index");
> +		return NULL;
> +	}
> +	for (uint32_t i = 0; i < ret->index_count; i++)
> +		ret->index[i] = space->index[i];

7. What is the problem to make memcpy() instead of the cycle?

> +	memcpy(ret->def, space->def, sizeof(struct space_def));
> +	ret->def->opts.is_temporary = true;
> +	ret->def->opts.is_ephemeral = true;
> +	uint32_t fields_size = sizeof(struct field_def) * ret->def->field_count;
> +	ret->def->fields = region_alloc(&parse->region, fields_size);

8. Use region_alloc_array. Otherwise it will crash in ASAN build. It
should be visible in CI.

> +	if (ret->def->fields == NULL) {
> +		diag_set(OutOfMemory, fields_size, "region_alloc",
> +			 "ret->def->fields");

9. index array leaks here.

> +		return NULL;
> +	}
> +	memcpy(ret->def->fields, space->def->fields, fields_size);
> +
> +	return ret;
> +}
> +
>  void
> -sqlAddColumn(Parse * pParse, Token * pName, struct type_def *type_def)
> +sql_create_column_start(struct Parse *parse)
>  {
> -	assert(type_def != NULL);
> -	char *z;
> -	sql *db = pParse->db;
> -	if (pParse->create_table_def.new_space == NULL)
> -		return;
> -	struct space_def *def = pParse->create_table_def.new_space->def;
> +	struct create_column_def *create_column_def = &parse->create_column_def;
> +	struct alter_entity_def *alter_entity_def =
> +		&create_column_def->base.base;
> +	assert(alter_entity_def->entity_type == ENTITY_TYPE_COLUMN);
> +	assert(alter_entity_def->alter_action == ALTER_ACTION_CREATE);
> +	struct space *space = parse->create_table_def.new_space;
> +	bool is_alter = space == NULL;
> +	struct sql *db = parse->db;
> +	if (is_alter) {
> +		const char *space_name =
> +			alter_entity_def->entity_name->a[0].zName;
> +		space = space_by_name(space_name);
> +		if (space == NULL) {
> +			diag_set(ClientError, ER_NO_SUCH_SPACE, space_name);
> +			goto tnt_error;
> +		}
> +		space = sql_shallow_space_copy(parse, space);
> +		if (space == NULL)
> +			goto tnt_error;
> +	}
> +	create_column_def->space = space;
> +	struct space_def *def = space->def;
> +	assert(def->opts.is_ephemeral);
>  
>  #if SQL_MAX_COLUMN
>  	if ((int)def->field_count + 1 > db->aLimit[SQL_LIMIT_COLUMN]) {
>  		diag_set(ClientError, ER_SQL_COLUMN_COUNT_MAX, def->name,
>  			 def->field_count + 1, db->aLimit[SQL_LIMIT_COLUMN]);
> -		pParse->is_aborted = true;
> -		return;
> +		goto tnt_error;

10. Was there a leak before your patch? Because of not
called sqlSrcListDelete(db, alter_entity_def->entity_name);

>  	}
>  #endif
> +
> +	struct region *region = &parse->region;
> +	struct Token *name = &create_column_def->base.name;
> +	char *column_name =
> +		sql_normalized_name_region_new(region, name->z, name->n);
> +	if (column_name == NULL)
> +		goto tnt_error;
> +
> +	if (parse->create_table_def.new_space == NULL && def->opts.is_view) {

11. You have is_alter flag for that.

> +		diag_set(ClientError, ER_SQL_CANT_ADD_COLUMN_TO_VIEW,
> +			 column_name, def->name);
> +		goto tnt_error;
> +	}
> +
>  	/*
> -	 * As sql_field_retrieve will allocate memory on region
> -	 * ensure that def is also temporal and would be dropped.
> +	 * Format can be set in Lua, then exact_field_count can be
> +	 * zero, but field_count is not.
>  	 */
> -	assert(def->opts.is_ephemeral);
> -	if (sql_field_retrieve(pParse, def, def->field_count) == NULL)
> -		return;
> -	struct region *region = &pParse->region;
> -	z = sql_normalized_name_region_new(region, pName->z, pName->n);
> -	if (z == NULL) {
> -		pParse->is_aborted = true;
> +	if (def->exact_field_count == 0)
> +		def->exact_field_count = def->field_count;
> +	if (sql_field_retrieve(parse, def, def->field_count) == NULL)
>  		return;
> +
> +	for (uint32_t i = 0; i < def->field_count; i++) {
> +		if (strcmp(column_name, def->fields[i].name) == 0) {
> +			diag_set(ClientError, ER_SPACE_FIELD_IS_DUPLICATE,
> +				 column_name);
> +			goto tnt_error;
> +		}

12. I remember this code was deliberately removed, because the same check is
already done in box. Why did you return it?

>  	}
>  	struct field_def *column_def = &def->fields[def->field_count];
> -	memcpy(column_def, &field_def_default, sizeof(field_def_default));
> -	column_def->name = z;
> +	memcpy(column_def, &field_def_default, sizeof(struct field_def));

13. Unnecessary change of memcpy().

> +	column_def->name = column_name;
>  	/*
>  	 * Marker ON_CONFLICT_ACTION_DEFAULT is used to detect
>  	 * attempts to define NULL multiple time or to detect
> @@ -334,19 +398,100 @@ sqlAddColumn(Parse * pParse, Token * pName, struct type_def *type_def)
>  	 */
>  	column_def->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
>  	column_def->is_nullable = true;
> -	column_def->type = type_def->type;
> +	column_def->type = create_column_def->type_def->type;
>  	def->field_count++;
> +
> +	sqlSrcListDelete(db, alter_entity_def->entity_name);
> +	return;
> +tnt_error:
> +	parse->is_aborted = true;
> +	sqlSrcListDelete(db, alter_entity_def->entity_name);
> +}
> +
> +static void
> +sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id);
> +
> +void
> +sql_create_column_end(struct Parse *parse)
> +{
> +	struct create_column_def *create_column_def = &parse->create_column_def;
> +	struct space *space = parse->create_table_def.new_space;
> +	bool is_alter = space == NULL;
> +	space = create_column_def->space;
> +	struct space_def *def = space->def;
> +	if (is_alter) {
> +		struct field_def *field = &def->fields[def->field_count - 1];
> +		if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) {
> +			if (create_column_def->is_pk) {
> +				field->nullable_action =
> +					ON_CONFLICT_ACTION_ABORT;
> +				field->is_nullable = false;
> +			} else {
> +				field->nullable_action =
> +					ON_CONFLICT_ACTION_NONE;
> +				field->is_nullable = true;
> +			}
> +		}
> +		/*
> +		 * Encode the format array and emit code to update _space.
> +		 */
> +		uint32_t table_stmt_sz = 0;
> +		struct region *region = &parse->region;
> +		char *table_stmt = sql_encode_table(region, def,
> +						    &table_stmt_sz);
> +		char *raw = sqlDbMallocRaw(parse->db, table_stmt_sz);
> +		if (table_stmt == NULL || raw == NULL) {
> +			parse->is_aborted = true;
> +			return;
> +		}
> +		memcpy(raw, table_stmt, table_stmt_sz);
> +
> +		struct Vdbe *v = sqlGetVdbe(parse);
> +		assert(v != NULL);
> +
> +		struct space *system_space = space_by_id(BOX_SPACE_ID);
> +		assert(system_space != NULL);
> +		int cursor = parse->nTab++;
> +		vdbe_emit_open_cursor(parse, cursor, 0, system_space);
> +		sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
> +
> +		int key_reg = ++parse->nMem;
> +		sqlVdbeAddOp2(v, OP_Integer, def->id, key_reg);
> +		int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 1);
> +		sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
> +		sqlVdbeJumpHere(v, addr);
> +
> +		int tuple_reg = sqlGetTempRange(parse, box_space_field_MAX + 1);
> +		for (int i = 0; i < box_space_field_MAX - 1; ++i)
> +			sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i);
> +		sqlVdbeAddOp1(v, OP_Close, cursor);
> +
> +		sqlVdbeAddOp2(v, OP_Integer, def->field_count, tuple_reg + 4);
> +		sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz, tuple_reg + 6,
> +			      SQL_SUBTYPE_MSGPACK, raw, P4_DYNAMIC);
> +		sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, box_space_field_MAX,
> +			      tuple_reg + box_space_field_MAX);
> +		sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + box_space_field_MAX,
> +			      0, 0, (char *) system_space, P4_SPACEPTR);
> +		sql_vdbe_create_constraints(parse, key_reg);
> +	}
> +
> +	memset(create_column_def, 0, sizeof(struct create_column_def));
> +	create_column_def->nullable_action = ON_CONFLICT_ACTION_DEFAULT;

14. Why do you touch column creation def after its usage?

>  }
>  
>  void
>  sql_column_add_nullable_action(struct Parse *parser,
>  			       enum on_conflict_action nullable_action)
>  {
> -	struct space *space = parser->create_table_def.new_space;
> -	if (space == NULL || NEVER(space->def->field_count < 1))
> +	struct space_def *def = NULL;
> +	struct field_def *field = NULL;
> +	struct space *space = parser->create_column_def.space;
> +	assert (space != NULL);

15. Please, don't put whitespace after macro/function name and its arguments.

> +	def = space->def;
> +	if (NEVER(def->field_count < 1))
>  		return;
> -	struct space_def *def = space->def;
> -	struct field_def *field = &def->fields[def->field_count - 1];
> +	field = &def->fields[def->field_count - 1];

16. Why did you do that change about struct field_def? It seems not to
be needed.

>  	if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
>  	    nullable_action != field->nullable_action) {
>  		/* Prevent defining nullable_action many times. */
> @@ -364,51 +509,46 @@ sql_column_add_nullable_action(struct Parse *parser,
>  }
>  
>  /*
> - * The expression is the default value for the most recently added column
> - * of the table currently under construction.
> + * The expression is the default value for the most recently added
> + * column.
>   *
>   * Default value expressions must be constant.  Raise an exception if this
>   * is not the case.
>   *
>   * This routine is called by the parser while in the middle of
> - * parsing a CREATE TABLE statement.
> + * parsing a <CREATE TABLE> or a <ALTER TABLE ADD COLUMN>
> + * statement.
>   */
>  void
>  sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan)
>  {
>  	sql *db = pParse->db;
> -	struct space *p = pParse->create_table_def.new_space;
> -	if (p != NULL) {
> -		assert(p->def->opts.is_ephemeral);
> -		struct space_def *def = p->def;
> -		if (!sqlExprIsConstantOrFunction
> -		    (pSpan->pExpr, db->init.busy)) {
> -			const char *column_name =
> -				def->fields[def->field_count - 1].name;
> -			diag_set(ClientError, ER_CREATE_SPACE, def->name,
> -				 tt_sprintf("default value of column '%s' is "\
> -					    "not constant", column_name));
> +	struct space_def *def = NULL;
> +	struct field_def *field = NULL;
> +	struct space *space = pParse->create_column_def.space;
> +	assert (space != NULL);
> +	def = space->def;

17. Why can't you declare and initialize space_def in one line? The same
for field.

> +	field = &def->fields[def->field_count - 1];
> +	if (!sqlExprIsConstantOrFunction(pSpan->pExpr, db->init.busy)) {
> +		diag_set(ClientError, ER_CREATE_SPACE, def->name,
> +			 tt_sprintf("default value of column '%s' is not "
> +				    "constant", field->name));
> +		pParse->is_aborted = true;
> +	} else {
> +		assert(def != NULL);
> +		struct region *region = &pParse->region;
> +		uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
> +		field->default_value = region_alloc(region, default_length + 1);
> +		if (field->default_value == NULL) {
> +			diag_set(OutOfMemory, default_length + 1,
> +				 "region_alloc", "field->default_value");
>  			pParse->is_aborted = true;
> -		} else {
> -			assert(def != NULL);
> -			struct field_def *field =
> -				&def->fields[def->field_count - 1];
> -			struct region *region = &pParse->region;
> -			uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart);
> -			field->default_value = region_alloc(region,
> -							    default_length + 1);
> -			if (field->default_value == NULL) {
> -				diag_set(OutOfMemory, default_length + 1,
> -					 "region_alloc",
> -					 "field->default_value");
> -				pParse->is_aborted = true;
> -				return;
> -			}
> -			strncpy(field->default_value, pSpan->zStart,
> -				default_length);
> -			field->default_value[default_length] = '\0';
> +			goto add_default_value_exit;
>  		}
> +		strncpy(field->default_value, pSpan->zStart, default_length);
> +		field->default_value[default_length] = '\0';
>  	}
> +add_default_value_exit:
>  	sql_expr_delete(db, pSpan->pExpr, false);
>  }
>  
> @@ -447,6 +587,8 @@ sqlAddPrimaryKey(struct Parse *pParse)
>  	int nTerm;
>  	struct ExprList *pList = pParse->create_index_def.cols;
>  	struct space *space = pParse->create_table_def.new_space;
> +	if (space == NULL)
> +		space = pParse->create_column_def.space;
>  	if (space == NULL)
>  		goto primary_key_exit;
>  	if (sql_space_primary_key(space) != NULL) {
> @@ -574,8 +716,10 @@ sql_create_check_contraint(struct Parse *parser)
>  		(struct alter_entity_def *) create_ck_def;
>  	assert(alter_def->entity_type == ENTITY_TYPE_CK);
>  	(void) alter_def;
> -	struct space *space = parser->create_table_def.new_space;
> -	bool is_alter = space == NULL;
> +	struct space *space = parser->create_column_def.space;
> +	if (space == NULL)
> +		space = parser->create_table_def.new_space;
> +	bool is_alter_add_constr = space == NULL;

18. Why is it called 'is_alter' in other functions, but 'is_alter_add_constr'
here?

>  
>  	/* Prepare payload for ck constraint definition. */
>  	struct region *region = &parser->region;
> @@ -589,9 +733,18 @@ sql_create_check_contraint(struct Parse *parser)
>  			return;
>  		}
>  	} else {
> -		assert(! is_alter);
> -		uint32_t ck_idx = ++parser->create_table_def.check_count;
> -		name = tt_sprintf("ck_unnamed_%s_%d", space->def->name, ck_idx);
> +		assert(!is_alter_add_constr);
> +		uint32_t idx = ++parser->check_count;
> +		if (parser->create_table_def.new_space == NULL) {
> +			struct space *original_space =
> +				space_by_name(space->def->name);
> +			assert(original_space != NULL);
> +			struct rlist *checks = &original_space->ck_constraint;
> +			struct ck_constraint *ck;
> +			rlist_foreach_entry(ck, checks, link)
> +				idx++;
> +		}
> +		name = tt_sprintf("ck_unnamed_%s_%d", space->def->name, idx);
>  	}
>  	size_t name_len = strlen(name);
>  
> @@ -634,9 +787,9 @@ 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);
> +		space = space_by_name(space_name);

19. Why this change?

>  		if (space == NULL) {
>  			diag_set(ClientError, ER_NO_SUCH_SPACE, space_name);
>  			parser->is_aborted = true;
> @@ -652,8 +805,7 @@ sql_create_check_contraint(struct Parse *parser)
>  		sqlVdbeCountChanges(v);
>  		sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
>  	} else {
> -		rlist_add_entry(&parser->create_table_def.new_check, ck_parse,
> -				link);
> +		rlist_add_entry(&parser->checks, ck_parse, link);
>  	}
>  }
>  
> @@ -664,18 +816,19 @@ sql_create_check_contraint(struct Parse *parser)
>  void
>  sqlAddCollateType(Parse * pParse, Token * pToken)
>  {
> -	struct space *space = pParse->create_table_def.new_space;
> -	if (space == NULL)
> -		return;
> +	struct space *space = pParse->create_column_def.space;
> +	uint32_t *coll_id = NULL;
> +	assert(space != NULL);
>  	uint32_t i = space->def->field_count - 1;
> +	coll_id = &space->def->fields[i].coll_id;

20. Why not declare and initialize coll_id in one line?

>  	sql *db = pParse->db;
>  	char *coll_name = sql_name_from_token(db, pToken);
>  	if (coll_name == NULL) {
>  		pParse->is_aborted = true;
>  		return;
>  	}
> -	uint32_t *coll_id = &space->def->fields[i].coll_id;
> -	if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL) {
> +	if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL &&
> +	    space != NULL) {

21. You have assert(space != NULL) above. Why do you need this check again?

>  		/* If the column is declared as "<name> PRIMARY KEY COLLATE <type>",
>  		 * then an index may have been created on this column before the
>  		 * collation type was added. Correct this if it is the case.
> @@ -692,6 +845,7 @@ sqlAddCollateType(Parse * pParse, Token * pToken)
>  	sqlDbFree(db, coll_name);
>  }
>  
> +

22. ???

>  struct coll *
>  sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id)
>  {
> @@ -1147,6 +1304,105 @@ resolve_link(struct Parse *parse_context, const struct space_def *def,
>  	return -1;
>  }
>  
> +/**
> + * Emit code to create sequences, indexes, check and foreign
> + * constraints appeared in <CREATE TABLE> or
> + * <ALTER TABLE ADD COLUMN> statement.
> + */
> +static void
> +sql_vdbe_create_constraints(struct Parse *parse, int reg_space_id)

23. Such huge code movements better do in a separate commit. I can't
tell whether you changed anything here, or just moved the code.

> +{
> +	assert(reg_space_id != 0);
> +	struct space *space = parse->create_table_def.new_space;
> +	bool is_alter = space == NULL;
> +	uint32_t i = 0;
> +	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,
> +				       reg_space_id, idx->def->iid);
> +	}
> +
> +	/*
> +	 * Check to see if we need to create an _sequence table
> +	 * for keeping track of autoincrement keys.
> +	 */
> +	if (parse->has_autoinc) {
> +		/* Do an insertion into _sequence. */
> +		int reg_seq_id = ++parse->nMem;
> +		struct Vdbe *v = sqlGetVdbe(parse);
> +		assert(v != NULL);
> +		sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id);
> +		int reg_seq_record =
> +			emitNewSysSequenceRecord(parse, reg_seq_id,
> +						 space->def->name);
> +		const char *error_msg =
> +			tt_sprintf(tnt_errcode_desc(ER_SQL_CANT_ADD_AUTOINC),
> +				   space->def->name);
> +		if (vdbe_emit_halt_with_presence_test(parse,
> +						      BOX_SEQUENCE_ID, 2,
> +						      reg_seq_record + 3, 1,
> +						      ER_SQL_CANT_ADD_AUTOINC,
> +						      error_msg, false,
> +						      OP_NoConflict) != 0)
> +			return;
> +		sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record);
> +		/* Do an insertion into _space_sequence. */
> +		int reg_space_seq_record =
> +			emitNewSysSpaceSequenceRecord(parse, reg_space_id,
> +						      reg_seq_id);
> +		sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
> +			      reg_space_seq_record);
> +	}
> +
> +	/* Code creation of FK constraints, if any. */
> +	struct fk_constraint_parse *fk_parse;
> +	rlist_foreach_entry(fk_parse, &parse->fkeys, link) {
> +		struct fk_constraint_def *fk_def = fk_parse->fk_def;
> +		if (fk_parse->selfref_cols != NULL) {
> +			struct ExprList *cols = fk_parse->selfref_cols;
> +			for (uint32_t i = 0; i < fk_def->field_count; ++i) {
> +				if (resolve_link(parse, space->def,
> +						 cols->a[i].zName,
> +						 &fk_def->links[i].parent_field,
> +						 fk_def->name) != 0)
> +					return;
> +			}
> +			fk_def->parent_id = reg_space_id;
> +		} else if (fk_parse->is_self_referenced) {
> +			struct key_def *pk_key_def =
> +				sql_space_primary_key(space)->def->key_def;
> +			if (pk_key_def->part_count != fk_def->field_count) {
> +				diag_set(ClientError, ER_CREATE_FK_CONSTRAINT,
> +					 fk_def->name, "number of columns in "\
> +					 "foreign key does not match the "\
> +					 "number of columns in the primary "\
> +					 "index of referenced table");
> +				parse->is_aborted = true;
> +				return;
> +			}
> +			for (uint32_t i = 0; i < fk_def->field_count; ++i) {
> +				fk_def->links[i].parent_field =
> +					pk_key_def->parts[i].fieldno;
> +			}
> +			fk_def->parent_id = reg_space_id;
> +		}
> +		fk_def->child_id = reg_space_id;
> +		vdbe_emit_fk_constraint_create(parse, fk_def, space->def->name);
> +	}
> +
> +	/* Code creation of CK constraints, if any. */
> +	struct ck_constraint_parse *ck_parse;
> +	rlist_foreach_entry(ck_parse, &parse->checks, link) {
> +		vdbe_emit_ck_constraint_create(parse, ck_parse->ck_def,
> +					       reg_space_id, space->def->name);
> +	}
> +}
> +
>  /*
>   * This routine is called to report the final ")" that terminates
>   * a CREATE TABLE statement.
> @@ -1213,73 +1469,7 @@ sqlEndTable(struct Parse *pParse)
>  
>  	int reg_space_id = getNewSpaceId(pParse);
>  	vdbe_emit_space_create(pParse, reg_space_id, name_reg, new_space);
> -	for (uint32_t i = 0; i < new_space->index_count; ++i) {
> -		struct index *idx = new_space->index[i];
> -		vdbe_emit_create_index(pParse, new_space->def, idx->def,
> -				       reg_space_id, idx->def->iid);
> -	}
> -
> -	/*
> -	 * Check to see if we need to create an _sequence table
> -	 * for keeping track of autoincrement keys.
> -	 */
> -	if (pParse->create_table_def.has_autoinc) {
> -		assert(reg_space_id != 0);
> -		/* Do an insertion into _sequence. */
> -		int reg_seq_id = ++pParse->nMem;
> -		sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id);
> -		int reg_seq_record =
> -			emitNewSysSequenceRecord(pParse, reg_seq_id,
> -						 new_space->def->name);
> -		sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record);
> -		/* Do an insertion into _space_sequence. */
> -		int reg_space_seq_record =
> -			emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
> -						      reg_seq_id);
> -		sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
> -			      reg_space_seq_record);
> -	}
> -	/* Code creation of FK constraints, if any. */
> -	struct fk_constraint_parse *fk_parse;
> -	rlist_foreach_entry(fk_parse, &pParse->create_table_def.new_fkey,
> -			    link) {
> -		struct fk_constraint_def *fk_def = fk_parse->fk_def;
> -		if (fk_parse->selfref_cols != NULL) {
> -			struct ExprList *cols = fk_parse->selfref_cols;
> -			for (uint32_t i = 0; i < fk_def->field_count; ++i) {
> -				if (resolve_link(pParse, new_space->def,
> -						 cols->a[i].zName,
> -						 &fk_def->links[i].parent_field,
> -						 fk_def->name) != 0)
> -					return;
> -			}
> -			fk_def->parent_id = reg_space_id;
> -		} else if (fk_parse->is_self_referenced) {
> -			struct index *pk = sql_space_primary_key(new_space);
> -			if (pk->def->key_def->part_count != fk_def->field_count) {
> -				diag_set(ClientError, ER_CREATE_FK_CONSTRAINT,
> -					 fk_def->name, "number of columns in "\
> -					 "foreign key does not match the "\
> -					 "number of columns in the primary "\
> -					 "index of referenced table");
> -				pParse->is_aborted = true;
> -				return;
> -			}
> -			for (uint32_t i = 0; i < fk_def->field_count; ++i) {
> -				fk_def->links[i].parent_field =
> -					pk->def->key_def->parts[i].fieldno;
> -			}
> -			fk_def->parent_id = reg_space_id;
> -		}
> -		fk_def->child_id = reg_space_id;
> -		vdbe_emit_fk_constraint_create(pParse, fk_def, space_name_copy);
> -	}
> -	struct ck_constraint_parse *ck_parse;
> -	rlist_foreach_entry(ck_parse, &pParse->create_table_def.new_check,
> -			    link) {
> -		vdbe_emit_ck_constraint_create(pParse, ck_parse->ck_def,
> -					       reg_space_id, space_name_copy);
> -	}
> +	sql_vdbe_create_constraints(pParse, reg_space_id);
>  }
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 995875566..5904414a3 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -314,6 +320,7 @@ ccons ::= cconsname(N) PRIMARY KEY sortorder(Z). {
>    create_index_def_init(&pParse->create_index_def, NULL, &N, NULL,
>                          SQL_INDEX_TYPE_CONSTRAINT_PK, Z, false);
>    sqlAddPrimaryKey(pParse);
> +  pParse->create_column_def.is_pk = true;

24. Why can't this be done in sqlAddPrimaryKey(pParse);? What if this
is CREATE TABLE, and there are many columns in PK?

>  }
>  ccons ::= cconsname(N) UNIQUE. {
>    create_index_def_init(&pParse->create_index_def, NULL, &N, NULL,
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index a5a258805..b31bac437 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -197,9 +197,13 @@ sql_parser_create(struct Parse *parser, struct sql *db, uint32_t sql_flags)
>  {
>  	memset(parser, 0, sizeof(struct Parse));
>  	parser->db = db;
> +	parser->create_column_def.nullable_action = ON_CONFLICT_ACTION_DEFAULT;

25. Why isn't there a proper constructor for create_column_def structure?

>  	parser->sql_flags = sql_flags;
>  	parser->line_count = 1;
>  	parser->line_pos = 1;
> +	rlist_create(&parser->fkeys);
> +	rlist_create(&parser->checks);

26. Why did you merge these fields into struct Parse? They were specially
moved out of here for the purpose of some kind of isolation, into parse_def.h
structures.

> +	parser->has_autoinc = false;
>  	region_create(&parser->region, &cord()->slabc);
>  }
>  
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index aa6a470f8..3143ec521 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -2249,12 +2249,26 @@ struct Parse {
>  		struct enable_entity_def enable_entity_def;
>  	};
>  	/**
> -	 * Table def is not part of union since information
> -	 * being held must survive till the end of parsing of
> -	 * whole CREATE TABLE statement (to pass it to
> -	 * sqlEndTable() function).
> +	 * Table def or column def is not part of union since
> +	 * information being held must survive till the end of
> +	 * parsing of whole <CREATE TABLE> or
> +	 * <ALTER TABLE ADD COLUMN> statement (to pass it to
> +	 * sqlEndTable() sql_create_column_end() function).
>  	 */
>  	struct create_table_def create_table_def;
> +	struct create_column_def create_column_def;
> +	/**
> +	 * FK and CK constraints appeared in a <CREATE TABLE> or
> +	 * a <ALTER TABLE ADD COLUMN> statement.
> +	 */
> +	struct rlist fkeys;
> +	struct rlist checks;
> +	uint32_t fkey_count;
> +	uint32_t check_count;
> +	/** True, if column to be created has <AUTOINCREMENT>. */
> +	bool has_autoinc;

27. What column? This is struct Parse, it is not a column.

> +	/** Id of field with <AUTOINCREMENT>. */
> +	uint32_t autoinc_fieldno;
>  	bool initiateTTrans;	/* Initiate Tarantool transaction */
>  	/** If set - do not emit byte code at all, just parse.  */
>  	bool parse_only;
> @@ -2844,15 +2858,31 @@ struct space *sqlResultSetOfSelect(Parse *, Select *);
>  
>  struct space *
>  sqlStartTable(Parse *, Token *);
> -void sqlAddColumn(Parse *, Token *, struct type_def *);
> +
> +/**
> + * Add new field to the format of ephemeral space in
> + * create_table_def. If it is <ALTER TABLE> create shallow copy of
> + * the existing space and add field to its format.
> + */
> +void
> +sql_create_column_start(struct Parse *parse);
> +
> +/**
> + * Nullify create_column_def. If it is <ALTER TABLE> emit code to
> + * update entry in _space and code to create constraints (entries
> + * in _index, _ck_constraint, _fk_constraint) described with this
> + * column.
> + */
> +void
> +sql_create_column_end(struct Parse *parse);
>  
>  /**
>   * This routine is called by the parser while in the middle of
> - * parsing a CREATE TABLE statement.  A "NOT NULL" constraint has
> - * been seen on a column.  This routine sets the is_nullable flag
> - * on the column currently under construction.
> - * If nullable_action has been already set, this function raises
> - * an error.
> + * parsing a <CREATE TABLE> or a <ALTER TABLE ADD COLUMN>
> + * statement. A "NOT NULL" constraint has been seen on a column.
> + * This routine sets the is_nullable flag on the column currently
> + * under construction. If nullable_action has been already set,
> + * this function raises an error.
>   *
>   * @param parser SQL Parser object.
>   * @param nullable_action on_conflict_action value.
> diff --git a/test/box/error.result b/test/box/error.result
> index 2196fa541..eb55f9cdf 100644
> --- a/test/box/error.result
> +++ b/test/box/error.result
> @@ -432,6 +432,8 @@ t;
>   |   211: box.error.WRONG_QUERY_ID
>   |   212: box.error.SEQUENCE_NOT_STARTED
>   |   213: box.error.NO_SUCH_SESSION_SETTING
> + |   214: box.error.SQL_CANT_ADD_COLUMN_TO_VIEW
> + |   215: box.error.SQL_CANT_ADD_AUTOINC
>   | ...
>  
>  test_run:cmd("setopt delimiter ''");
> diff --git a/test/sql/add-column.result b/test/sql/add-column.result
> new file mode 100644
> index 000000000..06c95facb
> --- /dev/null
> +++ b/test/sql/add-column.result

28. You may want to look at the original SQLite tests. They probably
have some.

> @@ -0,0 +1,276 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +engine = test_run:get_cfg('engine')
> + | ---
> + | ...
> +_ = box.space._session_settings:update('sql_default_engine', {{'=', 2, engine}})
> + | ---
> + | ...
> +
> +--
> +-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
> +--
> +box.execute('CREATE TABLE t1 (a INTEGER PRIMARY KEY);')
> + | ---
> + | - row_count: 1
> + | ...
> +--
> +-- COLUMN keyword is optional. Check it here, but omit it below.
> +--
> +box.execute('ALTER TABLE t1 ADD COLUMN b INTEGER;')
> + | ---
> + | - row_count: 0
> + | ...
> +
> +--
> +-- Can't add column to a view.
> +--
> +box.execute('CREATE VIEW v AS SELECT * FROM t1;')
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute('ALTER TABLE v ADD b INTEGER;')
> + | ---
> + | - null
> + | - Can't add column 'B'. 'V' is a view
> + | ...
> +box.execute('DROP VIEW v;')
> + | ---
> + | - row_count: 1
> + | ...
> +
> +--
> +-- Check column constraints typing and work.
> +--
> +box.execute('CREATE TABLE t2 (a INTEGER CONSTRAINT pk_constr PRIMARY KEY);')
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute('ALTER TABLE t2 DROP CONSTRAINT pk_constr')
> + | ---
> + | - row_count: 1
> + | ...
> +test_run:cmd("setopt delimiter ';'");
> + | ---
> + | - true
> + | ...
> +box.execute([[ALTER TABLE t2 ADD b INTEGER CONSTRAINT pk_constr PRIMARY KEY
> +                                   CHECK (b > 0)
> +                                   REFERENCES t1(a)
> +                                   CONSTRAINT u_constr UNIQUE]])
> +test_run:cmd("setopt delimiter ''");
> + | ---
> + | ...
> +box.execute('INSERT INTO t1 VALUES (1, 1);')
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute('INSERT INTO t2 VALUES (1, 1);')
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute('INSERT INTO t2 VALUES (1, 1);')
> + | ---
> + | - null
> + | - Duplicate key exists in unique index 'PK_CONSTR' in space 'T2'
> + | ...
> +
> +box.execute('INSERT INTO t1 VALUES (0, 1);')
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute('INSERT INTO t2 VALUES (2, 0);')
> + | ---
> + | - null
> + | - 'Check constraint failed ''ck_unnamed_T2_1'': b > 0'
> + | ...
> +
> +box.execute('INSERT INTO t2 VALUES (2, 3);')
> + | ---
> + | - null
> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
> + | ...
> +
> +box.execute('DROP TABLE t2;')
> + | ---
> + | - row_count: 1
> + | ...
> +
> +--
> +-- Check self-referenced FK creation.
> +--
> +box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY);')
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute('ALTER TABLE t2 ADD b INT REFERENCES t1')
> + | ---
> + | - row_count: 0

29. Worth checking if it works. Not just created and dropped right away.

> + | ...
> +
> +box.execute('DROP TABLE t2;')
> + | ---
> + | - row_count: 1
> + | ...
> +
> +--
> +-- Check AUTOINCREMENT work.
> +--
> +box.execute("CREATE TABLE t2(a INTEGER CONSTRAINT pk PRIMARY KEY);")
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute("ALTER TABLE t2 DROP CONSTRAINT pk;")
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute("ALTER TABLE t2 ADD b INTEGER PRIMARY KEY AUTOINCREMENT;")
> + | ---
> + | - row_count: 0

30. The same.

> + | ...
> +box.execute("ALTER TABLE t2 ADD c INTEGER AUTOINCREMENT;")
> + | ---
> + | - null
> + | - 'Can''t add AUTOINCREMENT: the space ''T2'' already has one auto-incremented field'
> + | ...
> +
> +box.execute('DROP TABLE t2;')
> + | ---
> + | - row_count: 1
> + | ...
> +
> +--
> +-- Check clauses after column typing and work.
> +--
> +box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY, b INTEGER);')
> + | ---
> + | - row_count: 1
> + | ...
> +test_run:cmd("setopt delimiter ';'");
> + | ---
> + | - true
> + | ...
> +box.execute([[ALTER TABLE t2 ADD c TEXT NOT NULL DEFAULT ('a')
> +                                   COLLATE "unicode_ci";]]);
> + | ---
> + | - row_count: 0
> + | ...
> +test_run:cmd("setopt delimiter ''");
> + | ---
> + | - true
> + | ...
> +box.execute('INSERT INTO t2(a, b) VALUES (1, 1);')
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute('SELECT * FROM t2;')
> + | ---
> + | - metadata:
> + |   - name: A
> + |     type: integer
> + |   - name: B
> + |     type: integer
> + |   - name: C
> + |     type: string
> + |   rows:
> + |   - [1, 1, 'a']
> + | ...
> +box.execute('INSERT INTO t2 VALUES (2, 2, NULL);')
> + | ---
> + | - null
> + | - 'Failed to execute SQL statement: NOT NULL constraint failed: T2.C'
> + | ...
> +box.execute('SELECT * FROM t2 WHERE c LIKE \'A\';')
> + | ---
> + | - metadata:
> + |   - name: A
> + |     type: integer
> + |   - name: B
> + |     type: integer
> + |   - name: C
> + |     type: string
> + |   rows:
> + |   - [1, 1, 'a']
> + | ...
> +
> +--
> +-- Try to add to a non-empty space a [non-]nullable field.
> +--
> +box.execute('ALTER TABLE t2 ADD d INTEGER;')
> + | ---
> + | - null
> + | - Tuple field count 3 does not match space field count 4
> + | ...
> +box.execute('ALTER TABLE t2 ADD d TEXT NOT NULL');
> + | ---
> + | - null
> + | - Tuple field count 3 does not match space field count 4
> + | ...
> +box.execute('ALTER TABLE t2 ADD e TEXT NULL');
> + | ---
> + | - null
> + | - Tuple field count 3 does not match space field count 4
> + | ...
> +
> +--
> +-- Add to a space with no-SQL adjusted or without format.
> +--
> +_ = box.schema.space.create('WITHOUT')
> + | ---
> + | ...
> +box.execute("ALTER TABLE WITHOUT ADD a INTEGER;")

31. No need to caps table names in SQL.

32. Need to check if it actually worked. The same below.

> + | ---
> + | - row_count: 0
> + | ...
> +box.execute("DROP TABLE WITHOUT;")
> + | ---
> + | - row_count: 1
> + | ...
> +
> +s = box.schema.space.create('NOSQL')
> + | ---
> + | ...
> +s:format{{name = 'A', type = 'unsigned'}}
> + | ---
> + | ...
> +box.execute("ALTER TABLE NOSQL ADD b INTEGER")
> + | ---
> + | - row_count: 0
> + | ...
> +
> +box.execute('DROP TABLE t2;')
> + | ---
> + | - row_count: 1
> + | ...
> +--
> +-- Add multiple columns inside a transaction.
> +--
> +box.execute('CREATE TABLE t2 (a INTEGER PRIMARY KEY)')
> + | ---
> + | - row_count: 1
> + | ...
> +box.begin()                                                                     \
> +box.execute('ALTER TABLE t2 ADD b INT')                                         \
> +box.execute('ALTER TABLE t2 ADD c INT')                                         \
> +box.commit()
> + | ---
> + | ...
> +
> +box.execute('INSERT INTO t2 VALUES (1, 1, 1)')
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute('SELECT * FROM t2;')
> + | ---
> + | - metadata:
> + |   - name: A
> + |     type: integer
> + |   - name: B
> + |     type: integer
> + |   - name: C
> + |     type: integer
> + |   rows:
> + |   - [1, 1, 1]

33. What if I add a column with UNIQUE constraint?