[Tarantool-patches] [PATCH v3 4/4] sql: support column addition

Roman Khabibov roman.habibov at tarantool.org
Sat Sep 12 00:51:42 MSK 2020


Hi! Thanks for the review.

> On Aug 20, 2020, at 01:20, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> 
> Thanks for the patch!
> 
> See 20 comments below.
> 
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 9013bc86f..6f3d2747d 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -285,48 +285,113 @@ 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)
>> +{
>> +	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);
> 
> 1. Why can't you use parser's region?
I added the patch. See it in the new patch set version.

>> +	if (ret->index == NULL) {
>> +		diag_set(OutOfMemory, indexes_sz, "realloc", "ret->index");
> 
> 2. It is not realloc, it is malloc.
> 
> 3. Seems you need to set parser->is_aborted = true in case of an error. As
> far as I understand, it is a contract of each function taking Parse
> argument.
Done.

+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)
+		goto error;
+	ret->index_count = space->index_count;
+	ret->index_id_max = space->index_id_max;
+	size_t size = 0;
+	ret->index = region_alloc_array(&parse->region, typeof(struct index *),
+					ret->index_count, &size);
+	if (ret->index == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc_array", "ret->index");
+		goto error;
+	}
+	memcpy(ret->index, space->index, size);
+	memcpy(ret->def, space->def, sizeof(struct space_def));
+	ret->def->opts.is_temporary = true;
+	ret->def->opts.is_ephemeral = true;
+	if (ret->def->field_count != 0) {
+		uint32_t fields_size = 0;
+		ret->def->fields =
+			region_alloc_array(&parse->region,
+					   typeof(struct field_def),
+					   ret->def->field_count, &fields_size);
+		if (ret->def->fields == NULL) {
+			diag_set(OutOfMemory, fields_size, "region_alloc",
+				 "ret->def->fields");
+			goto error;
+		}
+		memcpy(ret->def->fields, space->def->fields, fields_size);
+	}
+
+	return ret;
+error:
+	parse->is_aborted = true;
+	return NULL;
+}

>> +		return NULL;
>> +	}
>> +	memcpy(ret->index, space->index, indexes_sz);
>> +	memcpy(ret->def, space->def, sizeof(struct space_def));
>> +	ret->def->opts.is_temporary = true;
>> +	ret->def->opts.is_ephemeral = true;
>> +	if (ret->def->field_count != 0) {
>> +		uint32_t fields_size = 0;
>> +		ret->def->fields =
>> +			region_alloc_array(&parse->region,
>> +					   typeof(struct field_def),
>> +					   ret->def->field_count, &fields_size);
>> +		if (ret->def->fields == NULL) {
>> +			diag_set(OutOfMemory, fields_size, "region_alloc",
>> +				 "ret->def->fields");
>> +			free(ret->index);
>> +			return NULL;
>> +		}
>> +		memcpy(ret->def->fields, space->def->fields, fields_size);
>> +	}
>> +
>> +	return ret;
>> +}
>> @@ -334,18 +399,86 @@ sqlAddColumn(Parse * pParse, Token * pName, struct type_def *type_def)
>> +void
>> +sql_create_column_end(struct Parse *parse)
>> +{
>> +	struct space *space = parse->create_column_def.space;
>> +	assert(space != NULL);
>> +	struct space_def *def = space->def;
>> +	struct field_def *field = &def->fields[def->field_count - 1];
>> +	if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) {
>> +		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);
> 
> 4. You need to call sqlReleaseTempRange() somewhere.
Done.

>> +	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);
>> +
>> +	/*
>> +	 * Clean up array allocated in sql_shallow_space_copy().
>> +	 */
>> +	free(space->index);
> 
> 5. It may happen that sql_create_column_end() is never called. For
> example, if an error is encountered somewhere inside column definition,
> after sql_create_column_start() is called.
Removed with the region usage.

I also added the constants to AddOp functions.

+void
+sql_create_column_end(struct Parse *parse)
+{
+	struct space *space = parse->create_column_def.space;
+	assert(space != NULL);
+	struct space_def *def = space->def;
+	struct field_def *field = &def->fields[def->field_count - 1];
+	if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) {
+		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 + BOX_SPACE_FIELD_FIELD_COUNT);
+	sqlVdbeAddOp4(v, OP_Blob, table_stmt_sz,
+		      tuple_reg + BOX_SPACE_FIELD_FORMAT,
+		      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);
+	sqlVdbeCountChanges(v);
+	sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
+	sqlReleaseTempRange(parse, tuple_reg, box_space_field_MAX + 1);
+	sql_vdbe_create_constraints(parse, key_reg);
 }

>> }
>> 
>> 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))
>> +	assert(parser->create_column_def.space != NULL);
>> +	struct space_def *def = parser->create_column_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];
>> 	if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
>> 	    nullable_action != field->nullable_action) {
>> @@ -364,51 +497,42 @@ 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));
>> +	assert(pParse->create_column_def.space != NULL);
>> +	struct space_def *def = pParse->create_column_def.space->def;
>> +	struct field_def *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 {
>> +		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);
> 
> 6. Was it necessary to make so many changes? Wouldn't it work if you
> would just replace
> 
> 	struct space *p = pParse->create_table_def.new_space;
> 
> with
> 
> 	struct space *p = pParse->create_column_def.space;
> 
> ?
Yes. Done.

void
 sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan)
 {
 	sql *db = pParse->db;
-	struct space *p = pParse->create_table_def.new_space;
+	struct space *p = pParse->create_column_def.space;
 	if (p != NULL) {
 		assert(p->def->opts.is_ephemeral);
 		struct space_def *def = p->def;

>> }
>> 
>> @@ -574,8 +700,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;
> 
> 7. Why in some places you check create_table_def.new_space != NULL
> first, and in some places create_column_def.space != NULL first? Is
> the order important somehow?
The order is not important.

>> +	bool is_alter_add_constr = space == NULL;
>> 	/* Prepare payload for ck constraint definition. */
>> 	struct region *region = &parser->region;> @@ -704,8 +845,7 @@ sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id)
>> 	 *
>> 	 * In cases mentioned above collation is fetched by id.
>> 	 */
>> -	if (space == NULL) {
>> -		assert(def->opts.is_ephemeral);
>> +	if (def->opts.is_ephemeral) {
> 
> 8. space_by_id() above is not needed, if the definition is ephemeral now. It
> can be moved below this condition so as not to call it when the result is
> not going to be used anyway.
Yes.

@@ -704,13 +851,13 @@ sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id)
 	 *
 	 * In cases mentioned above collation is fetched by id.
 	 */
-	if (space == NULL) {
-		assert(def->opts.is_ephemeral);
+	if (def->opts.is_ephemeral) {
 		assert(column < (uint32_t)def->field_count);
 		*coll_id = def->fields[column].coll_id;
 		struct coll_id *collation = coll_by_id(*coll_id);
 		return collation != NULL ? collation->coll : NULL;
 	}
+	struct space *space = space_by_id(def->id);
 	struct tuple_field *field = tuple_format_field(space->format, column);
 	*coll_id = field->coll_id;
 	return field->coll;

>> 		assert(column < (uint32_t)def->field_count);
>> 		*coll_id = def->fields[column].coll_id;
>> 		struct coll_id *collation = coll_by_id(*coll_id);
>> @@ -1148,15 +1292,21 @@ resolve_link(struct Parse *parse_context, const struct space_def *def,
>> 
>> /**
>>  * Emit code to create sequences, indexes, check and foreign key
>> - * constraints appeared in <CREATE TABLE>.
>> + * 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 (is_alter) {
>> +		space = parse->create_column_def.space;
>> +		i = space_by_name(space->def->name)->index_count;
> 
> 9. Why do you need the original space for that? sql_shallow_space_copy()
> copies index_count as well.
Now, explained this step in the comment.

 /**
  * Emit code to create sequences, indexes, check and foreign key
- * constraints appeared in <CREATE TABLE>.
+ * 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
+	 * 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);

>> +	}
>> +	assert(space != NULL);
>> 	for (; i < space->index_count; ++i) {
>> 		struct index *idx = space->index[i];
>> 		vdbe_emit_create_index(parse, space->def, idx->def,
>> @@ -1908,6 +2077,8 @@ sql_create_foreign_key(struct Parse *parse_context)
>> 			goto tnt_error;
>> 		}
>> 		memset(fk_parse, 0, sizeof(*fk_parse));
>> +		if (parse_context->create_column_def.space != NULL)
>> +			child_space = space;
> 
> 10. Why?
Fixed.

@@ -1908,6 +2091,12 @@ sql_create_foreign_key(struct Parse *parse_context)
 			goto tnt_error;
 		}
 		memset(fk_parse, 0, sizeof(*fk_parse));
+		/*
+		* Child space already exists if it is
+		 * <ALTER TABLE ADD COLUMN>.
+		 */
+		if (table_def->new_space == NULL)
+			child_space = space;

>> 		rlist_add_entry(&parse_context->fkeys, fk_parse, link);
>> 	}
>> 	struct Token *parent = create_fk_def->parent_name;
>> @@ -1920,28 +2091,45 @@ sql_create_foreign_key(struct Parse *parse_context)
>> 	struct space *parent_space = space_by_name(parent_name);
>> -	if (parent_space == NULL) {
>> -		if (is_self_referenced) {
>> -			struct fk_constraint_parse *fk =
>> -				rlist_first_entry(&parse_context->fkeys,
>> -						  struct fk_constraint_parse,
>> -						  link);
>> -			fk->selfref_cols = parent_cols;
>> -			fk->is_self_referenced = true;
>> -		} else {
>> -			diag_set(ClientError, ER_NO_SUCH_SPACE, parent_name);;
>> -			goto tnt_error;
>> -		}
>> +	if (parent_space == NULL && !is_self_referenced) {
>> +		diag_set(ClientError, ER_NO_SUCH_SPACE, parent_name);
>> +		goto tnt_error;
>> +	}
>> +	if (is_self_referenced) {
>> +		struct fk_constraint_parse *fk =
>> +			rlist_first_entry(&parse_context->fkeys,
>> +					  struct fk_constraint_parse,
>> +					  link);
>> +		fk->selfref_cols = parent_cols;
>> +		fk->is_self_referenced = true;
>> 	}
> 
>    ^^^
> 11. This refactoring seems unnecessary. What changed here functionally?
No. I need to execute this branch, when parent_space != NULL (ADD COLUMN case).
Tests fail without this change.

+	if (is_self_referenced) {
+		struct fk_constraint_parse *fk =
+			rlist_first_entry(&parse_context->fkeys_def.fkeys,
+					  struct fk_constraint_parse, link);
+		fk->selfref_cols = parent_cols;
+		fk->is_self_referenced = true;


>> -	if (!is_alter) {
>> +	if (!is_alter_add_constr) {
>> 		if (create_def->name.n == 0) {
>> -			constraint_name =
>> -				sqlMPrintf(db, "fk_unnamed_%s_%d",
>> -					   space->def->name,
>> -					   ++parse_context->fkey_count);
>> +			uint32_t idx = ++parse_context->fkey_count;
>> +			/*
>> +			 * If it is <ALTER TABLE ADD COLUMN> we
>> +			 * should count the existing FK
>> +			 * constraints in the space and form a
>> +			 * name based on this.
>> +			 */
>> +			if (table_def->new_space == NULL) {
>> +				struct space *original_space =
>> +					space_by_name(space->def->name);
>> +				assert(original_space != NULL);
>> +				struct rlist *child_fk =
>> +					&original_space->child_fk_constraint;
>> +				if (!rlist_empty(child_fk)) {
> 
> 12. You don't need to check for emptiness. rlist_foreach_entry() works fine
> with an empty list.
Ok. Removed.

>> +					struct fk_constraint *fk;
>> +					rlist_foreach_entry(fk, child_fk,
>> +							    in_child_space)
>> +						idx++;
>> +				}
>> +			}
>> +			constraint_name = sqlMPrintf(db, "fk_unnamed_%s_%d",
>> +						     space->def->name, idx);
>> 		} else {
>> 			constraint_name =
>> 				sql_name_from_token(db, &create_def->name);
>> @@ -2001,7 +2189,8 @@ sql_create_foreign_key(struct Parse *parse_context)
>> 	}
>> 	int actions = create_fk_def->actions;
>> 	fk_def->field_count = child_cols_count;
>> -	fk_def->child_id = child_space != NULL ? child_space->def->id : 0;
>> +	fk_def->child_id = table_def->new_space == NULL ?
>> +		child_space->def->id : 0;
> 
> 13. Why?
Removed.

>> 	fk_def->parent_id = parent_space != NULL ? parent_space->def->id : 0;
>> 	fk_def->is_deferred = create_constr_def->is_deferred;
>> 	fk_def->match = (enum fk_constraint_match) (create_fk_def->match);
>> @@ -2420,10 +2613,8 @@ sql_create_index(struct Parse *parse) {
>> 			}
>> 			goto exit_create_index;
>> 		}
>> -	} else {
>> -		if (parse->create_table_def.new_space == NULL)
>> -			goto exit_create_index;
>> -		space = parse->create_table_def.new_space;
>> +	} else if (space == NULL) {
> 
> 14. Why not !is_create_table_or_add_col?
Fixed.

@@ -2439,10 +2646,8 @@ sql_create_index(struct Parse *parse) {
 			}
 			goto exit_create_index;
 		}
-	} else {
-		if (parse->create_table_def.new_space == NULL)
-			goto exit_create_index;
-		space = parse->create_table_def.new_space;
+	} else if (!is_create_table_or_add_col) {
+		goto exit_create_index;

>> +		goto exit_create_index;
>> 	}
>> 	struct space_def *def = space->def;
>> 
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index 995875566..0c9887851 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -281,9 +286,11 @@ nm(A) ::= id(A). {
>>   }
>> }
>> 
>> -// "carglist" is a list of additional constraints that come after the
>> -// column name and column type in a CREATE TABLE statement.
>> -//
>> +/*
> 
> 15. Out-of-function comments are started from /**.
Fixed.

>> + * "carglist" is a list of additional constraints and clauses that
>> + * come after the column name and column type in a <CREATE TABLE>
>> + * or <ALTER TABLE ADD COLUMN> statement.
>> + */
>> carglist ::= carglist ccons.
>> carglist ::= .
>> %type cconsname { struct Token }
>> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
>> index 1105fda6e..336914c57 100644
>> --- a/src/box/sql/parse_def.h
>> +++ b/src/box/sql/parse_def.h
>> @@ -207,6 +209,14 @@ struct create_table_def {
>> 	struct space *new_space;
>> };
>> 
>> +struct create_column_def {
>> +	struct create_entity_def base;
>> +	/** Shallow space_def copy. */
> 
> 16. Not space_def.
Fixed.

>> +	struct space *space;
>> +	/** Column type. */
>> +	struct type_def *type_def;
>> +};
>> +
>> struct create_view_def {
>> 	struct create_entity_def base;
>> 	/**
>> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
>> index fa87e7bd2..32142a871 100644
>> --- a/src/box/sql/sqlInt.h
>> +++ b/src/box/sql/sqlInt.h
>> @@ -2860,15 +2864,30 @@ 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.
> 
> 17. It fills create_column_def, not create_table_def.
-void sqlAddColumn(Parse *, Token *, struct type_def *);
+
+/**
+ * Add new field to the format of ephemeral space in
+ * create_column_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);

>> + */
>> +void
>> +sql_create_column_start(struct Parse *parse);
>> +
>> diff --git a/test/sql/add-column.result b/test/sql/add-column.result
>> new file mode 100644
>> index 000000000..f86259105
>> --- /dev/null
>> +++ b/test/sql/add-column.result
>> @@ -0,0 +1,471 @@
>> +-- test-run result file version 2
>> +--
>> +-- gh-3075: Check <ALTER TABLE table ADD COLUMN column> statement.
>> +--
>> +CREATE TABLE t1 (a INT PRIMARY KEY);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +
>> +--
>> +-- COLUMN keyword is optional. Check it here, but omit it below.
>> +--
>> +ALTER TABLE t1 ADD COLUMN b INT;
>> + | ---
>> + | - row_count: 0
> 
> 18. It seems you need to return row_count 1. To be consistent with
> other ALTER TABLE expressions.
Done. I added opcode emission.

>> + | ...
>> +
>> +--
>> +-- A column with the same name already exists.
>> +--
>> +ALTER TABLE t1 ADD b SCALAR;
>> + | ---
>> + | - null
>> + | - Space field 'B' is duplicate
>> + | ...
>> +
>> +--
>> +-- Can't add column to a view.
>> +--
>> +CREATE VIEW v AS SELECT * FROM t1;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE v ADD b INT;
>> + | ---
>> + | - null
>> + | - Can't add column 'B'. 'V' is a view
> 
> 19. What if I do the same via direct replace into _space in Lua?
See the new patch in the patch set.

>> + | ...
>> +DROP VIEW v;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +
>> +--
>> +-- Check PRIMARY KEY constraint works with an added column.
>> +--
>> +CREATE TABLE pk_check (a INT CONSTRAINT pk PRIMARY KEY);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE pk_check DROP CONSTRAINT pk;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE pk_check ADD b INT PRIMARY KEY;
>> + | ---
>> + | - row_count: 0
>> + | ...
>> +INSERT INTO pk_check VALUES (1, 1);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +INSERT INTO pk_check VALUES (1, 1);
>> + | ---
>> + | - null
>> + | - Duplicate key exists in unique index 'pk_unnamed_PK_CHECK_1' in space 'PK_CHECK'
>> + | ...
>> +DROP TABLE pk_check;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +
>> +--
>> +-- Check UNIQUE constraint works with an added column.
>> +--
>> +CREATE TABLE unique_check (a INT PRIMARY KEY);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE unique_check ADD b INT UNIQUE;
>> + | ---
>> + | - row_count: 0
>> + | ...
>> +INSERT INTO unique_check VALUES (1, 1);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +INSERT INTO unique_check VALUES (2, 1);
>> + | ---
>> + | - null
>> + | - Duplicate key exists in unique index 'unique_unnamed_UNIQUE_CHECK_2' in space 'UNIQUE_CHECK'
>> + | ...
>> +DROP TABLE unique_check;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +
>> +--
>> +-- Check CHECK constraint works with an added column.
>> +--
>> +CREATE TABLE ck_check (a INT PRIMARY KEY);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE ck_check ADD b INT CHECK (b > 0);
>> + | ---
>> + | - row_count: 0
>> + | ...
>> +INSERT INTO ck_check VALUES (1, 0);
>> + | ---
>> + | - null
>> + | - 'Check constraint failed ''ck_unnamed_CK_CHECK_1'': b > 0'
>> + | ...
>> +DROP TABLE ck_check;
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +
>> +--
>> +-- Check FOREIGN KEY constraint works with an added column.
>> +--
>> +CREATE TABLE fk_check (a INT PRIMARY KEY);
>> + | ---
>> + | - row_count: 1
>> + | ...
>> +ALTER TABLE fk_check ADD b INT REFERENCES t1(a);
>> + | ---
>> + | - row_count: 0
>> + | ...
>> +INSERT INTO fk_check VALUES (0, 1);
>> + | ---
>> + | - null
>> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
>> + | ...
>> +INSERT INTO fk_check VALUES (2, 0);
>> + | ---
>> + | - null
>> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
>> + | ...
>> +INSERT INTO fk_check VALUES (2, 1);
>> + | ---
>> + | - null
>> + | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
> 
> 20. It is worth adding one more test with a successfull insertion.
+INSERT INTO fk_check VALUES (2, 1);
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: FOREIGN KEY constraint failed'
+ | ...
+INSERT INTO t1 VALUES (1, 1);
+ | ---
+ | - row_count: 1
+ | ...
+INSERT INTO fk_check VALUES (2, 1);
+ | ---
+ | - row_count: 1
+ | …


+INSERT INTO null_check(a) VALUES (1);
+ | ---
+ | - row_count: 1
+ | ...
+SELECT * FROM null_check;
+ | ---
+ | - metadata:
+ |   - name: A
+ |     type: integer
+ |   - name: B
+ |     type: string
+ |   rows:
+ |   - [1, null]
+ | …


+INSERT INTO notnull_check(a) VALUES (1);
+ | ---
+ | - null
+ | - 'Failed to execute SQL statement: NOT NULL constraint failed: NOTNULL_CHECK.B'
+ | ...
+INSERT INTO notnull_check VALUES (1, 'not null');
+ | ---
+ | - row_count: 1
+ | ...
+DROP TABLE notnull_check;
+ | ---
+ | - row_count: 1
+ | …




More information about the Tarantool-patches mailing list