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

[Nikita Pettik <korablev@tarantool.org>]

On 09 Oct 16:45, Roman Khabibov wrote:
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index e6957d612..5a59f477f 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -273,6 +273,8 @@ struct errcode_record {
>  	/*218 */_(ER_TUPLE_METADATA_IS_TOO_BIG,	"Can't create tuple: metadata size %u is too big") \
>  	/*219 */_(ER_XLOG_GAP,			"%s") \
>  	/*220 */_(ER_TOO_EARLY_SUBSCRIBE,	"Can't subscribe non-anonymous replica %s until join is done") \
> +	/*221 */_(ER_SQL_CANT_ADD_AUTOINC,	"Can't add AUTOINCREMENT: space %s can't feature more than one AUTOINCREMENT field") \
> +

Extra empty line.
  
>  /*
>   * !IMPORTANT! Please follow instructions at start of the file
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index fd8e7ed1e..597608bea 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -285,48 +285,110 @@ sql_field_retrieve(Parse *parser, struct space_def *space_def, uint32_t id)
>  	return field;
>  }
>  
> +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;
> +	}


You don't need all indexes from space..What you only need from space -          
count of indexes (to generate name if it is required). The same for
field defs. Mb it is worth avoiding copying space at all - just
store in create_column_def list of ck/fk constraints, single
field_def, index/field count - these objects seem to be enough.
Please consider this suggestion.
> +
> +	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;
> +
>  	/*
> -	 * 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)
> +	if (def->exact_field_count == 0)
> +		def->exact_field_count = def->field_count;

assert(is_alter);

> +	struct Vdbe *v = sqlGetVdbe(parse);
> +	assert(v != NULL);
> +
> +	struct space *system_space = space_by_id(BOX_SPACE_ID);

-> you can call it simply _space or s_space

> +	assert(system_space != NULL);
> +	int cursor = parse->nTab++;
> +	vdbe_emit_open_cursor(parse, cursor, 0, system_space);
> +	sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
> +
>  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;

Let's use standard facilities: assert(def->field_count > 0);

>  	if (space == NULL)
>  		goto primary_key_exit;
>  	if (sql_space_primary_key(space) != NULL) {
> @@ -574,8 +707,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;

Consider tiny refactoring:

is_alter = space == NULL;
if (is_alter)
	space = ...
assert(space != NULL);

> +	if (space == NULL)
> +		space = parser->create_table_def.new_space;
> +	bool is_alter_add_constr = space == NULL;
>  
> -		assert(! is_alter);
> +		assert(!is_alter_add_constr);

What's the point of this renaming?

>  		uint32_t ck_idx = ++parser->create_checks_def.count;
> +		/*
> @@ -1149,15 +1302,28 @@ 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
>  vdbe_emit_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

AFAIK only one index (corresponding to unique constraint).                      

> +	 * 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;
> +	}
> @@ -1176,6 +1342,21 @@ vdbe_emit_create_constraints(struct Parse *parse, int reg_space_id)
>  		sqlVdbeAddOp2(v, OP_NextSequenceId, 0, reg_seq_id);
>  		int reg_seq_rec = emitNewSysSequenceRecord(parse, reg_seq_id,
>  							   space->def->name);
> +		if (is_alter) {
> +			int errcode = ER_SQL_CANT_ADD_AUTOINC;
> +			const char *error_msg =
> +				tt_sprintf(tnt_errcode_desc(errcode),
> +					   space->def->name);
> +			if (vdbe_emit_halt_with_presence_test(parse,
> +							      BOX_SEQUENCE_ID,
> +							      2,
> +							      reg_seq_rec + 3,
> +							      1, errcode,
> +							      error_msg, false,
> +							      OP_NoConflict)
> +			    != 0)

Oh please leave !=0 cond on previous line. This way looks disgusting.

> +				return;
> +		}
>  		sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_rec);
>  		/* Do an insertion into _space_sequence. */
>  		int reg_space_seq_record =
> +	 * CONSTRAINT> statement handling.
>  	 */
> -	bool is_alter = space == NULL;
> +	bool is_alter_add_constr = space == NULL;

Again, what's the point of this renaming?