[Tarantool-patches] [PATCH v4 6/6] sql: support column addition
Nikita Pettik
korablev at tarantool.org
Wed Sep 16 23:18:23 MSK 2020
On 12 Sep 00:51, Roman Khabibov wrote:
> 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
Space emptyness is required by ansi or it is tarantool's restriction?
In Postgres for instance, one can add column for non-empty table and
it is considered to be ok (new column is filled with default value).
We can use ephemeral tables as temporary holder while rebuilding space.
Below some comments, I didn't review patch carefully tho.
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index 3c21375f5..ec16399a0 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -271,6 +271,7 @@ struct errcode_record {
> /*216 */_(ER_SYNC_QUORUM_TIMEOUT, "Quorum collection for a synchronous transaction is timed out") \
> /*217 */_(ER_SYNC_ROLLBACK, "A rollback for a synchronous transaction is received") \
> /*218 */_(ER_TUPLE_METADATA_IS_TOO_BIG, "Can't create tuple: metadata size %u is too big") \
> + /*219 */_(ER_SQL_CANT_ADD_AUTOINC, "Can't add AUTOINCREMENT: the space '%s' already has one auto incremented field") \
Consider reusing sql_add_autoincrement() func. Or re-phrase err msg and use it
in sql_add_autoinc. For example:
space %s can't feature more than one AUTOINCREMENT field.
> /*
> * !IMPORTANT! Please follow instructions at start of the file
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index d1d240315..677099b48 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;
> }
>
> -/*
> - * 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)
Why do you need whole space *? Def is likely to be enough.
> +{
> }
> size_t name_len = strlen(name);
> @@ -634,7 +783,7 @@ 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);
> if (space == NULL) {
> sqlVdbeAddOp2(v, OP_Integer, idx_def->iid, entry_reg + 1);
> } else {
> @@ -1032,18 +1180,21 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
> P4_DYNAMIC);
> /*
> * In case we are adding FK constraints during execution
> - * of <CREATE TABLE ...> statement, we don't have child
> - * id, but we know register where it will be stored.
> + * of <CREATE TABLE ...> or <ALER TABLE ADD COLUMN ...>
ALER -> ALTER
Don't get why we don't have child id for ALTER TABLE...
> + * statement, we don't have child id, but we know register
> + * where it will be stored.
> */
> - if (parse_context->create_table_def.new_space != NULL) {
> + bool is_alter_add_constr =
> + parse_context->create_table_def.new_space == NULL &&
> + parse_context->create_column_def.space == NULL;
> + if (!is_alter_add_constr) {
> sqlVdbeAddOp2(vdbe, OP_SCopy, fk->child_id,
> constr_tuple_reg + 1);
> } else {
> sqlVdbeAddOp2(vdbe, OP_Integer, fk->child_id,
> constr_tuple_reg + 1);
> }
> + * 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
At most one index..?
> + * 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);
> for (; i < space->index_count; ++i) {
> struct index *idx = space->index[i];
> vdbe_emit_create_index(parse, space->def, idx->def,
> memcpy(fk_def->name, constraint_name, name_len);
> fk_def->name[name_len] = '\0';
> /*
> - * In case of CREATE TABLE processing, all foreign keys
> - * constraints must be created after space itself, so
> - * lets delay it until sqlEndTable() call and simply
> - * maintain list of all FK constraints inside parser.
> + * In case of <CREATE TABLE> or <ALTER TABLE ... ADD
> + * COLUMN> processing, all foreign keys constraints must
> + * be created after space itself, so lets delay it until
Again: when we execute alter table, space itself must already exist.
More information about the Tarantool-patches
mailing list