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

[Nikita Pettik <korablev@tarantool.org>]

On 10 Nov 15:18, Roman Khabibov wrote:
> >> 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.
> Yes, I understand that. But unfortunately, the current code, namely
> the functions for creating indexes, fk, and ck constraints, are written
> in such a way that they cannot be called without struct space (ephemeral
> or not). Vlad and I discussed this and came to the conclusion that it is
> better to go in the direction of reusing the code, rather than rewriting
> a significant part of build.c.
> 
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018460.html
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019003.html

Oh, ok, I won't insist on this refactoring. But could you please
copy-paste referenced anwers from mailing list? Those letters quite
large so navigating through them takes a lot of time.
 
> >> +	/*
> >> +	 * 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).
> I can write any number of named unique constraints. They should be
> created.
> ALTER TABLE t ADD COLUMN a INT CONSTRAINT c_1 UNIQUE, CONSTRAINT c_2 UNIQUE …

Well, ok. Just wondering why somebody might want to create several unique
constraints over the same field..

Otherwise LGTM