Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Roman Khabibov <roman.habibov@tarantool.org>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] sql: support column addition
Date: Mon, 9 Mar 2020 23:16:34 +0100	[thread overview]
Message-ID: <61fa3221-1be2-0895-311a-88e9e151360a@tarantool.org> (raw)
In-Reply-To: <7F7A0DA7-C5F2-43FD-AFF4-F6F4C700D603@tarantool.org>

Hi! Sure.

On 09/03/2020 16:08, Roman Khabibov wrote:
> Vlad, can you take it, please?
> 
>> On Jan 31, 2020, at 18:05, Roman Khabibov <roman.habibov@tarantool.org> wrote:
>>
>> Enable to add column to existing space with
>> <ALTER TABLE ADD [COLUMN]> statement. Column definition can be
>> supplemented with <DEFAULT>, <COLLATE> clauses and <[NOT] NULL>
>> column constraint.
>>
>> Closes #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, except that table constraints (PRIMARY KEY, UNIQUE,
>> REFERENCES, CHECK) cannot be specified yet.
>>
>> 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
>> ...
>> ---
>> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3075-add-column
>> Issue: https://github.com/tarantool/tarantool/issues/3075
>>
>> P.S.: I didn't implement column constraints in ADD COLUMN statement,
>> because of the sql_create_index(), sql_create_foreign_key() and
>> sql_create_check_contraint() functions. These functions have
>> branching inside:
>>
>> 1) CREATE TABLE (parse->create_table_def.new_space != NULL)
>> We work with the ephemeral space. Use its def, indexes array,
>> ck and fk rlists for checks, opcode emitting, etc and modify it on
>> parsing level.
>>
>> 2) ALTER TABLE (parse->create_table_def.new_space == NULL)
>> We work with existing space = space_by_name(), but don't modify it.
>> This space will be modified after parsing by alter.cc triggers.
>>
>> In the case of
>> ALTER TABLE test ADD a INT PRIMARY KEY CONSTRAINT c UNIQUE CONSTRAINT d UNIQUE ...
>> we need to modifiy the existing space, for example UNIQUE constraint
>> creation/opcode emitting requires new space def with new modified
>> fields array and new index_id_max. The same is for REFERENCES
>> constraint. With CHECK it seems easier, but I didn't implement it,
>> because I want to do patch in the futue: "Enable constraints in ADD COLUMN",
>> if it will be needed.
>>
>> There is obvious way to implement constraints parsing during
>> ADD COLUMN. To do partial copy of space = space_by_name() on region,
>> to keep it in parse->create_column_>def and to work with it. Then
>> we will have to change the current code a bit, but it is necessary
>> to write API to create an ephemeral space based on the existing.
>>
>> I would like to hear your opinion. Maybe I'm generally wrong in
>> thinking about all this, and it could be made easier.
>>
>> P.P.S.: Could you, please, help me with COLUMN keyword support.
>> I couldn't add it as I wanted, i.e., the line in parse.y near the
>> line 1754:
>> column_name(N) ::= COLUMN nm(A). { N = A; }
>> because I had:
>> /Users/r.khabibov/tarantool/src/box/sql/parse.h:171:9: error: 'TK_COLUMN' macro redefined
>> #define TK_COLUMN
>>
>> I don't understand why it is so? For example, the definition of
>> CONSTRAINT keyword in mkkeywordhash.c is the same, but there is no
>> error about this #define during compilation.
>>
>>  { "COLUMN",                 "TK_COLUMN",      true  },
>>  { "CONSTRAINT",             "TK_CONSTRAINT",  true  },
>>
>> extra/mkkeywordhash.c       |   6 +-
>> src/box/errcode.h           |   1 +
>> src/box/sql.c               |  28 +++++---
>> src/box/sql/alter.c         | 125 ++++++++++++++++++++++++++++++++++++
>> src/box/sql/build.c         | 106 ++++++++++++++++++------------
>> src/box/sql/parse.y         |  59 +++++++++++++++--
>> src/box/sql/parse_def.h     |  23 +++++++
>> src/box/sql/sqlInt.h        |  40 ++++++++++++
>> src/box/sql/tarantoolInt.h  |   6 +-
>> test/box/misc.result        |   1 +
>> test/sql-tap/alter.test.lua |  76 +++++++++++++++++++++-
>> 11 files changed, 405 insertions(+), 66 deletions(-)
>>
>> 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  },
>> diff --git a/src/box/errcode.h b/src/box/errcode.h
>> index 3065b1948..e076edf02 100644
>> --- 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
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index 1256df856..7157cabe6 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -923,7 +923,8 @@ cursor_advance(BtCursor *pCur, int *pRes)
>>  */
>>
>> char *
>> -sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> +sql_encode_table_format(struct region *region, struct field_def *fields,
>> +			uint32_t field_count, uint32_t *size)
>> {
>> 	size_t used = region_used(region);
>> 	struct mpstream stream;
>> @@ -931,12 +932,11 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> 	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
>> 		      set_encode_error, &is_error);
>>
>> -	assert(def != NULL);
>> -	uint32_t field_count = def->field_count;
>> +	assert(fields != NULL);
>> 	mpstream_encode_array(&stream, field_count);
>> 	for (uint32_t i = 0; i < field_count && !is_error; i++) {
>> -		uint32_t cid = def->fields[i].coll_id;
>> -		struct field_def *field = &def->fields[i];
>> +		struct field_def *field = &fields[i];
>> +		uint32_t cid = field->coll_id;
>> 		const char *default_str = field->default_value;
>> 		int base_len = 4;
>> 		if (cid != COLL_NONE)
>> @@ -947,16 +947,16 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> 		mpstream_encode_str(&stream, "name");
>> 		mpstream_encode_str(&stream, field->name);
>> 		mpstream_encode_str(&stream, "type");
>> -		assert(def->fields[i].is_nullable ==
>> -		       action_is_nullable(def->fields[i].nullable_action));
>> +		assert(field->is_nullable ==
>> +		       action_is_nullable(field->nullable_action));
>> 		mpstream_encode_str(&stream, field_type_strs[field->type]);
>> 		mpstream_encode_str(&stream, "is_nullable");
>> -		mpstream_encode_bool(&stream, def->fields[i].is_nullable);
>> +		mpstream_encode_bool(&stream, field->is_nullable);
>> 		mpstream_encode_str(&stream, "nullable_action");
>>
>> -		assert(def->fields[i].nullable_action < on_conflict_action_MAX);
>> +		assert(field->nullable_action < on_conflict_action_MAX);
>> 		const char *action =
>> -			on_conflict_action_strs[def->fields[i].nullable_action];
>> +			on_conflict_action_strs[field->nullable_action];
>> 		mpstream_encode_str(&stream, action);
>> 		if (cid != COLL_NONE) {
>> 			mpstream_encode_str(&stream, "collation");
>> @@ -980,6 +980,14 @@ sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> 	return raw;
>> }
>>
>> +char *
>> +sql_encode_table(struct region *region, struct space_def *def, uint32_t *size)
>> +{
>> +	assert(def != NULL);
>> +	return sql_encode_table_format(region, def->fields, def->field_count,
>> +				       size);
>> +}
>> +
>> char *
>> sql_encode_table_opts(struct region *region, struct space_def *def,
>> 		      uint32_t *size)
>> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
>> index 973b420cf..c12d9373a 100644
>> --- a/src/box/sql/alter.c
>> +++ b/src/box/sql/alter.c
>> @@ -36,6 +36,7 @@
>> #include "sqlInt.h"
>> #include "box/box.h"
>> #include "box/schema.h"
>> +#include "tarantoolInt.h"
>>
>> void
>> sql_alter_table_rename(struct Parse *parse)
>> @@ -209,3 +210,127 @@ rename_trigger(sql *db, char const *sql_stmt, char const *table_name,
>> 				      table_name, tname.z + tname.n);
>> 	return new_sql_stmt;
>> }
>> +
>> +void
>> +sql_alter_add_column_start(struct Parse *parse)
>> +{
>> +	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);
>> +
>> +	const char *space_name =
>> +		alter_entity_def->entity_name->a[0].zName;
>> +	struct space *space = space_by_name(space_name);
>> +	struct sql *db = parse->db;
>> +	if (space == NULL) {
>> +		diag_set(ClientError, ER_NO_SUCH_SPACE, space_name);
>> +		goto tnt_error;
>> +	}
>> +
>> +	struct space_def *def = space->def;
>> +	create_column_def->space_def = def;
>> +	if (sql_space_def_check_format(def) != 0)
>> +		goto tnt_error;
>> +	uint32_t new_field_count = def->field_count + 1;
>> +	create_column_def->new_field_count = new_field_count;
>> +
>> +#if SQL_MAX_COLUMN
>> +	if ((int)new_field_count > db->aLimit[SQL_LIMIT_COLUMN]) {
>> +		diag_set(ClientError, ER_SQL_COLUMN_COUNT_MAX, def->name,
>> +			 new_field_count, db->aLimit[SQL_LIMIT_COLUMN]);
>> +		goto tnt_error;
>> +	}
>> +#endif
>> +	/*
>> +	 * Create new fields array and add the new column's
>> +	 * field_def to its end. During copying don't make
>> +	 * duplicates of the non-scalar fields (name,
>> +	 * default_value, default_value_expr), because they will
>> +	 * be just copied and encoded to the msgpack array on sql
>> +	 * allocator in sql_alter_add_column_end().
>> +	 */
>> +	uint32_t size = sizeof(struct field_def) * new_field_count;
>> +	struct region *region = &parse->region;
>> +	struct field_def *new_fields = region_alloc(region, size);
>> +	if (new_fields == NULL) {
>> +		diag_set(OutOfMemory, size, "region_alloc", "new_fields");
>> +		goto tnt_error;
>> +	}
>> +	memcpy(new_fields, def->fields,
>> +	       sizeof(struct field_def) * def->field_count);
>> +	create_column_def->new_fields = new_fields;
>> +	struct field_def *new_column_def = &new_fields[def->field_count];
>> +
>> +	memcpy(new_column_def, &field_def_default, sizeof(struct field_def));
>> +	struct Token *name = &create_column_def->base.name;
>> +	new_column_def->name =
>> +		sql_normalized_name_region_new(region, name->z, name->n);
>> +	if (new_column_def->name == NULL)
>> +		goto tnt_error;
>> +	if (def->opts.is_view) {
>> +		diag_set(ClientError, ER_SQL_CANT_ADD_COLUMN_TO_VIEW,
>> +			 new_column_def->name, def->name);
>> +		goto tnt_error;
>> +	}
>> +
>> +	new_column_def->type = create_column_def->type_def->type;
>> +
>> +exit_alter_add_column_start:
>> +	sqlSrcListDelete(db, alter_entity_def->entity_name);
>> +	return;
>> +tnt_error:
>> +	parse->is_aborted = true;
>> +	goto exit_alter_add_column_start;
>> +}
>> +
>> +void
>> +sql_alter_add_column_end(struct Parse *parse)
>> +{
>> +	struct create_column_def *create_column_def = &parse->create_column_def;
>> +	uint32_t new_field_count = create_column_def->new_field_count;
>> +	uint32_t table_stmt_sz = 0;
>> +	char *table_stmt =
>> +		sql_encode_table_format(&parse->region,
>> +					create_column_def->new_fields,
>> +					new_field_count, &table_stmt_sz);
>> +	if (table_stmt == NULL) {
>> +		parse->is_aborted = true;
>> +		return;
>> +	}
>> +	char *raw = sqlDbMallocRaw(parse->db, table_stmt_sz);
>> +	if (raw == NULL){
>> +		parse->is_aborted = true;
>> +		return;
>> +	}
>> +	memcpy(raw, table_stmt, table_stmt_sz);
>> +
>> +	struct space *system_space = space_by_id(BOX_SPACE_ID);
>> +	assert(system_space != NULL);
>> +	int cursor = parse->nTab++;
>> +	struct Vdbe *v = sqlGetVdbe(parse);
>> +	assert(v != NULL);
>> +	vdbe_emit_open_cursor(parse, cursor, 0, system_space);
>> +	sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
>> +
>> +	int key_reg = sqlGetTempReg(parse);
>> +	sqlVdbeAddOp2(v, OP_Integer, create_column_def->space_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);
>> +
>> +	const int system_space_field_count = 7;
>> +	int tuple_reg = sqlGetTempRange(parse, system_space_field_count + 1);
>> +	for (int i = 0; i < system_space_field_count - 1; ++i)
>> +		sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i);
>> +	sqlVdbeAddOp1(v, OP_Close, cursor);
>> +
>> +	sqlVdbeAddOp2(v, OP_Integer, new_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, system_space_field_count,
>> +		      tuple_reg + system_space_field_count);
>> +	sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + system_space_field_count, 0,
>> +		      0, (char *)system_space, P4_SPACEPTR);
>> +}
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index bc50ecbfa..2f51b55df 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -368,11 +368,21 @@ 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;
>> +	if (parser->create_table_def.new_space != NULL) {
>> +		def = parser->create_table_def.new_space->def;
>> +		if (NEVER(def->field_count < 1))
>> +			return;
>> +		field = &def->fields[def->field_count - 1];
>> +	} else if (parser->create_column_def.space_def != NULL) {
>> +		def = parser->create_column_def.space_def;
>> +		uint32_t field_count =
>> +			parser->create_column_def.new_field_count;
>> +		field = &parser->create_column_def.new_fields[field_count - 1];
>> +	} else {
>> 		return;
>> -	struct space_def *def = space->def;
>> -	struct field_def *field = &def->fields[def->field_count - 1];
>> +	}
>> 	if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
>> 	    nullable_action != field->nullable_action) {
>> 		/* Prevent defining nullable_action many times. */
>> @@ -390,51 +400,53 @@ 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 <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;
>> +	if (pParse->create_table_def.new_space != NULL) {
>> +		def = pParse->create_table_def.new_space->def;
>> +		assert(def->opts.is_ephemeral);
>> +		field = &def->fields[def->field_count - 1];
>> +	} else if (pParse->create_column_def.space_def != NULL) {
>> +		def = pParse->create_column_def.space_def;
>> +		uint32_t field_count =
>> +			pParse->create_column_def.new_field_count;
>> +		field = &pParse->create_column_def.new_fields[field_count - 1];
>> +	} else {
>> +		goto add_default_value_exit;
>> +	}
>> +	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';
>> +			return;
>> 		}
>> +		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);
>> }
>>
>> @@ -686,17 +698,27 @@ void
>> sqlAddCollateType(Parse * pParse, Token * pToken)
>> {
>> 	struct space *space = pParse->create_table_def.new_space;
>> -	if (space == NULL)
>> +	uint32_t *coll_id = NULL;
>> +	if (space != NULL) {
>> +		uint32_t i = space->def->field_count - 1;
>> +		coll_id = &space->def->fields[i].coll_id;
>> +	} else if (pParse->create_column_def.space_def != NULL) {
>> +		uint32_t field_count =
>> +			pParse->create_column_def.new_field_count;
>> +		struct field_def *field =
>> +			&pParse->create_column_def.new_fields[field_count - 1];
>> +		coll_id = &field->coll_id;
>> +	} else {
>> 		return;
>> -	uint32_t i = space->def->field_count - 1;
>> +	}
>> 	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) {
>> 		/* 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.
>> @@ -858,7 +880,9 @@ vdbe_emit_space_create(struct Parse *pParse, int space_id_reg,
>> 	if (table_opts_stmt == NULL)
>> 		goto error;
>> 	uint32_t table_stmt_sz = 0;
>> -	char *table_stmt = sql_encode_table(region, space->def, &table_stmt_sz);
>> +	char *table_stmt = sql_encode_table_format(region, space->def->fields,
>> +						   space->def->field_count,
>> +						   &table_stmt_sz);
>> 	if (table_stmt == NULL)
>> 		goto error;
>> 	char *raw = sqlDbMallocRaw(pParse->db,
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index cfe1c0012..2eba72ac3 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -289,10 +289,14 @@ carglist ::= .
>> %type cconsname { struct Token }
>> cconsname(N) ::= CONSTRAINT nm(X). { N = X; }
>> cconsname(N) ::= . { N = Token_nil; }
>> -ccons ::= DEFAULT term(X).            {sqlAddDefaultValue(pParse,&X);}
>> -ccons ::= DEFAULT LP expr(X) RP.      {sqlAddDefaultValue(pParse,&X);}
>> -ccons ::= DEFAULT PLUS term(X).       {sqlAddDefaultValue(pParse,&X);}
>> -ccons ::= DEFAULT MINUS(A) term(X).      {
>> +ccons ::= default_clause_1.
>> +default_clause_1 ::= DEFAULT term(X).           {sqlAddDefaultValue(pParse,&X);}
>> +ccons ::= default_clause_2.
>> +default_clause_2 ::= DEFAULT LP expr(X) RP.     {sqlAddDefaultValue(pParse,&X);}
>> +ccons ::= default_clause_3.
>> +default_clause_3 ::= DEFAULT PLUS term(X).      {sqlAddDefaultValue(pParse,&X);}
>> +ccons ::= default_clause_4.
>> +default_clause_4 ::= DEFAULT MINUS(A) term(X).      {
>>   ExprSpan v;
>>   v.pExpr = sqlPExpr(pParse, TK_UMINUS, X.pExpr, 0);
>>   v.zStart = A.z;
>> @@ -303,13 +307,18 @@ ccons ::= DEFAULT MINUS(A) term(X).      {
>> // In addition to the type name, we also care about the primary key and
>> // UNIQUE constraints.
>> //
>> -ccons ::= NULL onconf(R).        {
>> +ccons ::= null_cons.
>> +null_cons ::= NULL onconf(R).        {
>>     sql_column_add_nullable_action(pParse, ON_CONFLICT_ACTION_NONE);
>>     /* Trigger nullability mismatch error if required. */
>>     if (R != ON_CONFLICT_ACTION_ABORT)
>>         sql_column_add_nullable_action(pParse, R);
>> }
>> -ccons ::= NOT NULL onconf(R).    {sql_column_add_nullable_action(pParse, R);}
>> +ccons ::= not_null_cons.
>> +not_null_cons ::= NOT NULL onconf(R). {
>> +  sql_column_add_nullable_action(pParse, R);
>> +}
>> +
>> 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);
>> @@ -335,7 +344,8 @@ ccons ::= cconsname(N) REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R).
>>   sql_create_foreign_key(pParse);
>> }
>> ccons ::= defer_subclause(D).    {fk_constraint_change_defer_mode(pParse, D);}
>> -ccons ::= COLLATE id(C).        {sqlAddCollateType(pParse, &C);}
>> +ccons ::= collate_clause.
>> +collate_clause ::= COLLATE id(C).        {sqlAddCollateType(pParse, &C);}
>>
>> // The optional AUTOINCREMENT keyword
>> %type autoinc {int}
>> @@ -1729,11 +1739,46 @@ alter_table_start(A) ::= ALTER TABLE fullname(T) . { A = T; }
>>
>> %type alter_add_constraint {struct alter_args}
>> alter_add_constraint(A) ::= alter_table_start(T) ADD CONSTRAINT nm(N). {
>> +   A.table_name = T;
>> +   A.name = N;
>> +   pParse->initiateTTrans = true;
>> + }
>> +
>> +%type alter_add_column {struct alter_args}
>> +alter_add_column(A) ::= alter_table_start(T) ADD column_name(N). {
>>   A.table_name = T;
>>   A.name = N;
>>   pParse->initiateTTrans = true;
>> }
>>
>> +column_name(N) ::= nm(A). { N = A; }
>> +
>> +cmd ::= alter_column_def alter_carglist add_column_end.
>> +
>> +alter_column_def ::= alter_add_column(N) typedef(Y). {
>> +  create_column_def_init(&pParse->create_column_def, N.table_name, &N.name, &Y);
>> +  sql_alter_add_column_start(pParse);
>> +}
>> +
>> +/*
>> + * "alter_carglist" is a list of additional constraints and
>> + * clauses that come after the column name and column type in a
>> + * ALTER TABLE ADD [COLUMN] statement.
>> + */
>> +alter_carglist ::= alter_carglist alter_ccons.
>> +alter_carglist ::= .
>> +alter_ccons ::= default_clause_1.
>> +alter_ccons ::= default_clause_2.
>> +alter_ccons ::= default_clause_3.
>> +alter_ccons ::= default_clause_4.
>> +alter_ccons ::= null_cons.
>> +alter_ccons ::= not_null_cons.
>> +alter_ccons ::= collate_clause.
>> +
>> +add_column_end ::= . {
>> +  sql_alter_add_column_end(pParse);
>> +}
>> +
>> cmd ::= alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP REFERENCES
>>         nm(T) eidlist_opt(TA) matcharg(M) refargs(R) defer_subclause_opt(D). {
>>   create_fk_def_init(&pParse->create_fk_def, N.table_name, &N.name, FA, &T, TA,
>> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
>> index 2f433e4c0..56887efa7 100644
>> --- a/src/box/sql/parse_def.h
>> +++ b/src/box/sql/parse_def.h
>> @@ -154,6 +154,7 @@ enum sql_index_type {
>>
>> enum entity_type {
>> 	ENTITY_TYPE_TABLE = 0,
>> +	ENTITY_TYPE_COLUMN,
>> 	ENTITY_TYPE_VIEW,
>> 	ENTITY_TYPE_INDEX,
>> 	ENTITY_TYPE_TRIGGER,
>> @@ -227,6 +228,18 @@ struct create_table_def {
>> 	uint32_t autoinc_fieldno;
>> };
>>
>> +struct create_column_def {
>> +	struct create_entity_def base;
>> +	/** Column type. */
>> +	struct type_def *type_def;
>> +	/** Already existing space def. */
>> +	struct space_def *space_def;
>> +	/** New format array with the new column. */
>> +	struct field_def *new_fields;
>> +	/** Old field count + 1. */
>> +	uint32_t new_field_count;
>> +};
>> +
>> struct create_view_def {
>> 	struct create_entity_def base;
>> 	/**
>> @@ -486,6 +499,16 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name,
>> 	table_def->autoinc_fieldno = 0;
>> }
>>
>> +static inline void
>> +create_column_def_init(struct create_column_def *column_def,
>> +		       struct SrcList *table_name, struct Token *name,
>> +		       struct type_def *type_def)
>> +{
>> +	create_entity_def_init(&column_def->base, ENTITY_TYPE_COLUMN,
>> +			       table_name, name, false);
>> +	column_def->type_def = type_def;
>> +}
>> +
>> static inline void
>> create_view_def_init(struct create_view_def *view_def, struct Token *name,
>> 		     struct Token *create, struct ExprList *aliases,
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index d1fcf4761..46d3d6d2a 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> @@ -2231,6 +2231,7 @@ struct Parse {
>> 	 * from parse.y
>> 	 */
>> 	union {
>> +		struct create_column_def create_column_def;
>> 		struct create_ck_def create_ck_def;
>> 		struct create_fk_def create_fk_def;
>> 		struct create_index_def create_index_def;
>> @@ -2842,6 +2843,18 @@ struct space *
>> sqlStartTable(Parse *, Token *);
>> void sqlAddColumn(Parse *, Token *, struct type_def *);
>>
>> +/**
>> + * Set the is_nullable flag on the column @a field.
>> + *
>> + * @param field           Column to set.
>> + * @param nullable_action on_conflict_action value.
>> + * @param space_name Name of the space to which the column
>> + *                   belongs. Needed to set error.
>> + *
>> + * @retval 0  Success.
>> + * @retval -1 nullable_action has been already set.
>> + */
>> +
>> /**
>>  * This routine is called by the parser while in the middle of
>>  * parsing a CREATE TABLE statement.  A "NOT NULL" constraint has
>> @@ -2868,6 +2881,19 @@ sqlAddPrimaryKey(struct Parse *parse);
>> void
>> sql_create_check_contraint(Parse *parser);
>>
>> +/**
>> + * Add default value to the column @a field.
>> + *
>> + * @param parse      Parsing context.
>> + * @param field      Column to set.
>> + * @param span       Default expression.
>> + * @param space_name Name of the space to which the column
>> + *                   belongs. Needed to set error.
>> + *
>> + * @retval 0  Success.
>> + * @retval -1 Error.
>> + */
>> +
>> void sqlAddDefaultValue(Parse *, ExprSpan *);
>> void sqlAddCollateType(Parse *, Token *);
>>
>> @@ -4031,6 +4057,20 @@ sql_alter_table_rename(struct Parse *parse);
>> void
>> sql_alter_ck_constraint_enable(struct Parse *parse);
>>
>> +/**
>> + * Process the statement: do the checks of space and column
>> + * definition. Create new format array with a new column.
>> + */
>> +void
>> +sql_alter_add_column_start(struct Parse *parse);
>> +
>> +/**
>> + * Encode new format array and emit code to update an entry in
>> + * _space.
>> + */
>> +void
>> +sql_alter_add_column_end(struct Parse *parse);
>> +
>> /**
>>  * Return the length (in bytes) of the token that begins at z[0].
>>  * Store the token type in *type before returning.
>> diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
>> index 1ded6c709..5fd433c60 100644
>> --- a/src/box/sql/tarantoolInt.h
>> +++ b/src/box/sql/tarantoolInt.h
>> @@ -118,14 +118,16 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id);
>> /**
>>  * Encode format as entry to be inserted to _space on @region.
>>  * @param region Region to use.
>> - * @param def Space definition to encode.
>> + * @param fields Space format.
>> + * @param field_count Number of fields.
>>  * @param[out] size Size of result allocation.
>>  *
>>  * @retval NULL Error.
>>  * @retval not NULL Pointer to msgpack on success.
>>  */
>> char *
>> -sql_encode_table(struct region *region, struct space_def *def, uint32_t *size);
>> +sql_encode_table_format(struct region *region, struct field_def *fields,
>> +			uint32_t field_count, uint32_t *size);
>>
>> /**
>>  * Encode "opts" dictionary for _space entry on @region.
>> diff --git a/test/box/misc.result b/test/box/misc.result
>> index 3ca4e31ac..2aaec6566 100644
>> --- a/test/box/misc.result
>> +++ b/test/box/misc.result
>> @@ -563,6 +563,7 @@ t;
>>   209: box.error.SESSION_SETTING_INVALID_VALUE
>>   210: box.error.SQL_PREPARE
>>   211: box.error.WRONG_QUERY_ID
>> +  212: box.error.SQL_CANT_ADD_COLUMN_TO_VIEW
>> ...
>> test_run:cmd("setopt delimiter ''");
>> ---
>> diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
>> index 615b9d8a6..283dc1759 100755
>> --- a/test/sql-tap/alter.test.lua
>> +++ b/test/sql-tap/alter.test.lua
>> @@ -1,6 +1,6 @@
>> #!/usr/bin/env tarantool
>> test = require("sqltester")
>> -test:plan(50)
>> +test:plan(57)
>>
>> test:do_execsql_test(
>>     "alter-1.1",
>> @@ -585,4 +585,78 @@ test:do_catchsql_test(
>> --         -- </alter-7.11>
>> --     })
>>
>> +--
>> +-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
>> +--
>> +test:do_execsql_test(
>> +    "alter-9.1",
>> +    [[
>> +        CREATE TABLE test (a INTEGER PRIMARY KEY);
>> +        ALTER TABLE test ADD b INTEGER;
>> +    ]], {
>> +        -- <alter-9.1>
>> +        -- </alter-9.1>
>> +    })
>> +
>> +test:do_catchsql_test(
>> +    "alter-9.2",
>> +    [[
>> +        CREATE VIEW v AS SELECT * FROM test;
>> +        ALTER TABLE v ADD b INTEGER;
>> +    ]], {
>> +        -- <alter-9.2>
>> +        1,"Can't add column 'B' to the view 'V'"
>> +        -- </alter-9.2>
>> +    })
>> +
>> +test:do_execsql_test(
>> +    "alter-9.3",
>> +    [[
>> +        ALTER TABLE test ADD c TEXT NOT NULL DEFAULT ('a') COLLATE "unicode_ci";
>> +    ]], {
>> +        -- <alter-9.3>
>> +        -- </alter-9.3>
>> +    })
>> +
>> +test:do_execsql_test(
>> +    "alter-9.4",
>> +    [[
>> +        INSERT INTO test(a, b) VALUES (1, 1);
>> +        SELECT * FROM test;
>> +    ]], {
>> +        -- <alter-9.4>
>> +        1,1,"a"
>> +        -- </alter-9.4>
>> +    })
>> +
>> +test:do_catchsql_test(
>> +    "alter-9.5",
>> +    [[
>> +        INSERT INTO test VALUES (2, 2, NULL);
>> +    ]], {
>> +        -- <alter-9.5>
>> +        1,"Failed to execute SQL statement: NOT NULL constraint failed: TEST.C"
>> +                -- </alter-9.5>
>> +    })
>> +
>> +test:do_execsql_test(
>> +    "alter-9.6",
>> +    [[
>> +        SELECT * FROM test WHERE c LIKE 'A';
>> +    ]], {
>> +        -- <alter-9.6>
>> +        1,1,"a"
>> +        -- </alter-9.6>
>> +    })
>> +
>> +test:do_catchsql_test(
>> +    "alter-9.7",
>> +    [[
>> +        ALTER TABLE test ADD d INTEGER;
>> +    ]], {
>> +        -- <alter-9.7>
>> +        1,"Tuple field count 3 does not match space field count 4"
>> +        -- </alter-9.7>
>> +    })
>> +
>> test:finish_test()
>> -- 
>> 2.21.0 (Apple Git-122)
>>
> 

  reply	other threads:[~2020-03-09 22:16 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-31 15:05 Roman Khabibov
2020-03-09 15:08 ` Roman Khabibov
2020-03-09 22:16   ` Vladislav Shpilevoy [this message]
2020-03-14 17:17 ` Vladislav Shpilevoy
2020-07-06 13:37   ` Roman Khabibov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=61fa3221-1be2-0895-311a-88e9e151360a@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=roman.habibov@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] sql: support column addition' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox