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 30BAB218AE for ; Wed, 22 Aug 2018 02:39:47 -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 PHT4Rq3z8llh for ; Wed, 22 Aug 2018 02:39:47 -0400 (EDT) Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 7A48E2044D for ; Wed, 22 Aug 2018 02:39:46 -0400 (EDT) Date: Wed, 22 Aug 2018 09:39:43 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH 1/2] sql: after table rename properly update indexes Message-ID: <20180822063943.r5qplbqkgtao7fyr@tarantool.org> References: <8A23ACEC-DCAA-47A2-8311-F46D75C9F9C0@tarantool.org> <20180821122038.zlyjqxk5nzpj755l@tarantool.org> <5C92CDEF-15F0-4394-970F-608B9B59F333@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5C92CDEF-15F0-4394-970F-608B9B59F333@tarantool.org> 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: "n.pettik" Cc: tarantool-patches@freelists.org Hello Nikita, My answers are inlined. Updated patch in the bottom. Branch force-pushed. On 21 авг 23:38, n.pettik wrote: > > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > > index dc5146f..821c7a9 100644 > > --- a/src/box/sql/vdbe.c > > +++ b/src/box/sql/vdbe.c > > @@ -4625,13 +4625,35 @@ case OP_RenameTable: { > > db->init.busy = 1; > > init.rc = SQLITE_OK; > > sql_init_callback(&init, zNewTableName, space_id, 0, zSqlStmt); > > - db->init.busy = 0; > > rc = init.rc; > > - if (rc) { > > + if (rc != SQLITE_OK) { > > sqlite3CommitInternalChanges(); > > + db->init.busy = 0; > > goto abort_due_to_error; > > } > > > > + /* Space was altered, refetch the pointer. */ > > + space = space_by_id(space_id); > > + for (uint32_t i = 0; i < space->index_count; ++i) { > > + struct index_def *def = space->index[i]->def; > > + if (def->opts.sql == NULL) > > + continue; > > + char *sql_stmt; > > + rc = sql_index_update_table_name(def, zNewTableName, &sql_stmt); > > Where do you free memory for sql_stmt? table_rename allocates new > string using sqlite3DbMalloc AFAIK. Yes, it is. This is a clear leak. Fixed. > > + if (rc != SQLITE_OK) > > rc != 0 (or better: if (sql_index_update_table_name() != 0)). > > > + goto abort_due_to_error; > > + space = space_by_id(space_id); > > + sql_init_callback(&init, zNewTableName, space_id, > > + space->index[i]->def->iid, sql_stmt); > > + rc = init.rc; > > + if (rc != SQLITE_OK) { > > rc != 0 (or better: if (init.rc != 0) Fixed. Here's iterative diff: 1 file changed, 4 insertions(+), 3 deletions(-) src/box/sql/vdbe.c | 7 ++++--- modified src/box/sql/vdbe.c @@ -4663,14 +4663,15 @@ case OP_RenameTable: { continue; char *sql_stmt; rc = sql_index_update_table_name(def, zNewTableName, &sql_stmt); - if (rc != SQLITE_OK) + if (rc != 0) goto abort_due_to_error; space = space_by_id(space_id); sql_init_callback(&init, zNewTableName, space_id, space->index[i]->def->iid, sql_stmt); - rc = init.rc; - if (rc != SQLITE_OK) { + sqlite3DbFree(db, sql_stmt); + if (init.rc != 0) { sqlite3CommitInternalChanges(); + rc = init.rc; db->init.busy = 0; goto abort_due_to_error; } > > The rest seems to be ok. > Thanks! I'll check updated patch into 2.0 branch. -- Regards, Kirill Yukhin