From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 19DEA28F94 for ; Tue, 21 Aug 2018 06:26:45 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id QvN4jfrUhokz for ; Tue, 21 Aug 2018 06:26:45 -0400 (EDT) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 4F1AB28F91 for ; Tue, 21 Aug 2018 06:26:44 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes From: "n.pettik" In-Reply-To: Date: Tue, 21 Aug 2018 13:26:38 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <8A23ACEC-DCAA-47A2-8311-F46D75C9F9C0@tarantool.org> References: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Kirill Yukhin > diff --git a/src/box/sql.c b/src/box/sql.c > index ae12cae..9485899 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -840,6 +840,46 @@ rename_fail: > return SQL_TARANTOOL_ERROR; > } >=20 > +int > +sql_update_index_table_name(struct index_def *def, const char = *new_tbl_name, > + char **sql_stmt) I guess name should be like (according to naming convention): sql_index_update_table_name().. > +{ > + assert(new_tbl_name !=3D NULL); > + uint32_t iid =3D def->iid; Do you really need this shortcut for single (or two) usage(s)? > + > + bool is_quoted =3D false; > + *sql_stmt =3D rename_table(db, def->opts.sql, new_tbl_name, = &is_quoted); You need here check: rename_table() may fail and return NULL. > + > + uint32_t key_len =3D mp_sizeof_uint(def->space_id) + = mp_sizeof_uint(iid) + > + mp_sizeof_array(2); > + uint32_t new_opts_sz =3D = tarantoolSqlite3MakeIdxOpts(def->opts.is_unique, > + *sql_stmt, = NULL); > + uint32_t op_sz =3D mp_sizeof_array(1) + mp_sizeof_array(3) + > + mp_sizeof_str(1) + mp_sizeof_uint(4) + = new_opts_sz; > + > + char *key_begin =3D (char*) region_alloc(&fiber()->gc, key_len + = op_sz); > + if (key_begin =3D=3D NULL) { > + diag_set(OutOfMemory, key_len, "region_alloc", = "key_begin=E2=80=9D); In diag_set you pass key_len, but allocate key_len + op_sz. > + return SQL_TARANTOOL_ERROR; > + } > + char *key =3D mp_encode_array(key_begin, 2); > + key =3D mp_encode_uint(key, def->space_id); > + key =3D mp_encode_uint(key, iid); > + > + char *op_begin =3D key; > + char *op =3D mp_encode_array(op_begin, 1); > + op =3D mp_encode_array(op, 3); > + op =3D mp_encode_str(op, "=3D", 1); > + op =3D mp_encode_uint(op, 4); Mb it would be better to use BOX_INDEX_FIELD_OPTS? At least it would be easier to understand what happen here. > + op +=3D tarantoolSqlite3MakeIdxOpts(def->opts.is_unique, = *sql_stmt, op); > + > + if (box_update(BOX_INDEX_ID, 0, key_begin, key, op_begin, op, > + 0, NULL) !=3D 0) > + return SQL_TARANTOOL_ERROR; > + > + return SQLITE_OK; AFAIR we decided to avoid using SQLITE_OK macros. Lets use instead simple return 0/-1. > +} > + > int > tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor, > struct UnpackedRecord *unpacked) > @@ -1484,23 +1524,12 @@ int tarantoolSqlite3MakeIdxParts(SqliteIndex = *pIndex, void *buf) > return p - base; > } >=20 > -/* > - * Format "opts" dictionary for _index entry. > - * Returns result size. > - * If buf=3D=3DNULL estimate result size. > - * > - * Ex: { > - * "unique": "true", > - * "sql": "CREATE INDEX student_by_name ON students(name)" > - * } > - */ > -int tarantoolSqlite3MakeIdxOpts(SqliteIndex *index, const char *zSql, = void *buf) > +int > +tarantoolSqlite3MakeIdxOpts(bool is_unique, const char *zSql, void = *buf) > { You can pass here struct index_def (or struct index_opts) instead of two args: is_unique ---> def->opst.is_unique and zSql ---> def->opts.sql. I see that you pass new zSql, but you still can reassign def->opts.sql. It is up to you, thou. > const struct Enc *enc =3D get_enc(buf); > char *base =3D buf, *p; >=20 > - (void)index; > - > p =3D enc->encode_map(base, 2); > /* Mark as unique pk and unique indexes */ > p =3D enc->encode_str(p, "unique", 6); > @@ -1511,7 +1540,7 @@ int tarantoolSqlite3MakeIdxOpts(SqliteIndex = *index, const char *zSql, void *buf) > * INSERT OR REPLACE/IGNORE uniqueness checks will be also done = by > * Tarantool. > */ > - p =3D enc->encode_bool(p, IsUniqueIndex(index)); > + p =3D enc->encode_bool(p, is_unique); > p =3D enc->encode_str(p, "sql", 3); > p =3D enc->encode_str(p, zSql, zSql ? strlen(zSql) : 0); > return (int)(p - base); > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index dddeb12..05b729d 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1246,13 +1246,14 @@ createIndex(Parse * pParse, Index * pIndex, = int iSpaceId, int iIndexId, >=20 > /* Format "opts" and "parts" for _index entry. */ > zOpts =3D sqlite3DbMallocRaw(pParse->db, > - tarantoolSqlite3MakeIdxOpts(pIndex, = zSql, > - NULL) + > + = tarantoolSqlite3MakeIdxOpts(IsUniqueIndex(pIndex), > + zSql, = NULL) + > tarantoolSqlite3MakeIdxParts(pIndex, > NULL) + = 2); > if (!zOpts) > return; > - zOptsSz =3D tarantoolSqlite3MakeIdxOpts(pIndex, zSql, zOpts); > + zOptsSz =3D tarantoolSqlite3MakeIdxOpts(IsUniqueIndex(pIndex), = zSql, > + zOpts); > zParts =3D zOpts + zOptsSz + 1; > zPartsSz =3D tarantoolSqlite3MakeIdxParts(pIndex, zParts); > #if SQLITE_DEBUG > diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h > index 94517f6..536b0e3 100644 > --- a/src/box/sql/tarantoolInt.h > +++ b/src/box/sql/tarantoolInt.h > @@ -89,6 +89,20 @@ int tarantoolSqlite3ClearTable(struct space = *space); > int > sql_rename_table(uint32_t space_id, const char *new_name, char = **sql_stmt); >=20 > +/** > + * Update CREATE INDEX field (def->opt.sql) replacing table name > + * w/ new one in _index space. > + * > + * @param idef Index definition. > + * @param new_tbl_name new name of table Nitpicking: put dot at the end of sentence and start sentence with = capital letter. > + * @param[out] sql_stmt New CREATE INDEX statement. > + * > + * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise. > + */ > +int > +sql_update_index_table_name(struct index_def *idef, const char = *new_tbl_name, > + char **sql_stmt); > + > /* Alter trigger statement after rename table. */ > int tarantoolSqlite3RenameTrigger(const char *zTriggerName, > const char *zOldName, const char = *zNewName); > @@ -171,12 +185,17 @@ fkey_encode_links(const struct fkey_def *def, = int type, char *buf); > */ > int tarantoolSqlite3MakeIdxParts(Index * index, void *buf); >=20 > -/* > +/** > * Format "opts" dictionary for _index entry. > * Returns result size. > * If buf=3D=3DNULL estimate result size. > + * > + * @param is_unique Flag if index is unique. > + * @param zSql SQL create stmt. > + * @param[out] buf destination buffer. You didn=E2=80=99t specify @retval for this function. > /** > * Extract next id from _sequence space. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index dc5146f..4516ee9 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4625,13 +4625,35 @@ case OP_RenameTable: { > db->init.busy =3D 1; > init.rc =3D SQLITE_OK; > sql_init_callback(&init, zNewTableName, space_id, 0, zSqlStmt); > - db->init.busy =3D 0; > rc =3D init.rc; > - if (rc) { > + if (rc !=3D SQLITE_OK) { > sqlite3CommitInternalChanges(); > + db->init.busy =3D 0; > goto abort_due_to_error; > } >=20 > + /* Space was altered, refetch the pointer. */ > + space =3D space_by_id(space_id); > + for (uint32_t i =3D 0; i < space->index_count; ++i) { > + struct index_def *def =3D space->index[i]->def; > + if (def->opts.sql =3D=3D NULL) > + continue; > + char *sql_stmt; > + rc =3D sql_update_index_table_name(def, zNewTableName, = &sql_stmt); > + if (rc !=3D SQLITE_OK) > + goto abort_due_to_error; Again the same problem with RENAME: in case of fail part of indexes = would remain with old =E2=80=98create index =E2=80=A6=E2=80=99 statement and = after server=E2=80=99s restart it is likely to result in segfault/assertion. > + space =3D space_by_id(space_id); Does opts.sql update can lead to renewal of space ptr? If so, it looks quite weird: string with =E2=80=98CREATE INDEX =E2=80=A6=E2= =80=99 statement seems to be sort of static attribute which may be useful only during instance restart... > + sql_init_callback(&init, zNewTableName, space_id, > + space->index[i]->def->iid, sql_stmt); > + rc =3D init.rc; > + if (rc !=3D SQLITE_OK) { > + sqlite3CommitInternalChanges(); > + db->init.busy =3D 0; > + goto abort_due_to_error; > + } > + } > + db->init.busy =3D 0; > + > /* > * Rebuild 'CREATE TRIGGER' expressions of all triggers > * created on this table. Sure, this action is not atomic >=20 > diff --git a/test/sql/gh-3613-idx-alter-update.test.lua = b/test/sql/gh-3613-idx-alter-update.test.lua > new file mode 100644 > index 0000000..287096b > --- /dev/null > +++ b/test/sql/gh-3613-idx-alter-update.test.lua > @@ -0,0 +1,21 @@ > +test_run =3D require('test_run').new() > +engine =3D test_run:get_cfg('engine') > +box.sql.execute('pragma sql_default_engine=3D\''..engine..'\'') > + > +box.sql.execute('CREATE TABLE t (s1 INT PRIMARY KEY)') > +box.sql.execute('CREATE INDEX i ON t (s1)') > +box.sql.execute('ALTER TABLE t RENAME TO j3') > + > +-- Due to gh-3613, next stmt caused segfault > +box.sql.execute('DROP INDEX i ON j3') > + > +box.sql.execute('CREATE INDEX i ON j3 (s1)=E2=80=99) Well, actually I would also add test when instance is restarted right after dropping to make sure that no artefacts are left after = rename. Recreation may be misleading in this case.