From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 35F7F469719 for ; Fri, 6 Nov 2020 18:57:20 +0300 (MSK) Date: Fri, 6 Nov 2020 15:57:19 +0000 From: Nikita Pettik Message-ID: <20201106155719.GA13328@tarantool.org> References: <20201009134529.13212-1-roman.habibov@tarantool.org> <20201009134529.13212-6-roman.habibov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201009134529.13212-6-roman.habibov@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 5/5] sql: support column addition List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov Cc: tarantool-patches@dev.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 . > + * constraints appeared in or > + * . > */ > 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 , 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?