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 775FA2B54F for ; Tue, 20 Mar 2018 04:28:53 -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 nrtWrXj-jx45 for ; Tue, 20 Mar 2018 04:28:53 -0400 (EDT) Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (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 E26452B522 for ; Tue, 20 Mar 2018 04:28:52 -0400 (EDT) Date: Tue, 20 Mar 2018 11:28:49 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH 1/2] sql: refactor initialization routines Message-ID: <20180320082848.pf5ebs6rlkrxfxj7@tarantool.org> References: <0bf2e1909c97298d450fc4e0612cf10a1e4403d0.1521137308.git.kyukhin@tarantool.org> <3F4C5B6B-9887-4A4A-9DE2-5D53A9EFC06A@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <3F4C5B6B-9887-4A4A-9DE2-5D53A9EFC06A@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: "v.shpilevoy@tarantool.org" Cc: tarantool-patches@freelists.org Hello Vlad, I've fixed/commented your inputs. Could you pls take a look? On 16 мар 23:32, v.shpilevoy@tarantool.org wrote: > See below 6 comments. > > > 15 марта 2018 г., в 21:21, Kirill Yukhin написал(а): > > > > Make SQL load schema during initialization. It was > > initialized on first query before the patch. Replace > > check that schema is loaded w/ assert arouns the code base. > > > > As far as DDL is prohibited inside the transaction, schema > > cannot be changed inside a transaction and hence, no need to > > reload it during rollback. No internal changes > > (SQLITE_InernChanges) are possible. > > > > Part of #3235 > > --- > > src/box/sql.c | 16 +--- > > src/box/sql/analyze.c | 7 +- > > src/box/sql/build.c | 33 ++------ > > src/box/sql/main.c | 204 +++++++++++++++--------------------------------- > > src/box/sql/pragma.c | 3 +- > > src/box/sql/prepare.c | 53 ------------- > > src/box/sql/sqlite3.h | 7 +- > > src/box/sql/sqliteInt.h | 2 - > > src/box/sql/trigger.c | 5 +- > > 9 files changed, 80 insertions(+), 250 deletions(-) > > > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > > index 130e87665..38f48c5a0 100644 > > --- a/src/box/sql/analyze.c > > +++ b/src/box/sql/analyze.c > > @@ -1176,12 +1176,7 @@ sqlite3Analyze(Parse * pParse, Token * pName) > > Table *pTab; > > Vdbe *v; > > > > - /* Read the database schema. If an error occurs, leave an error message > > - * and code in pParse and return NULL. > > - */ > > - if (SQLITE_OK != sqlite3ReadSchema(pParse)) { > > - return; > > - } > > + assert(db->pSchema); > > 1. Please, use explicit "!= NULL". Fixed. > > diff --git a/src/box/sql/main.c b/src/box/sql/main.c > > index 540570dc8..5a0f372b3 100644 > > --- a/src/box/sql/main.c > > +++ b/src/box/sql/main.c > > @@ -981,6 +981,15 @@ sqlite3RollbackAll(Vdbe * pVdbe, int tripCode) > > && db->init.busy == 0) { > > sqlite3ExpirePreparedStatements(db); > > sqlite3ResetAllSchemasOfConnection(db); > > + > > + db->init.busy = 1; > > 2. Why do you initialise DB in sqlite3RollbackAll? And do you really need to add place this code here, if in a next commit you remove it? I hope we'll have bisect up and running. So, all patches are pass testing. This is actual re-read in memory DD representation in case schema id (aka cookie) was changed during transaction. This might happen if trigger was added or deleted. Next patch blocks this explicitly. > > @@ -2246,122 +2255,62 @@ sqlite3ParseUri(const char *zDefaultVfs, /* VFS to use if no "vfs=xxx" query opt > > return rc; > > } > > > > -/* > > - * This routine does the work of opening a database on behalf of > > - * sqlite3_open() and database filename "zFilename" > > - * is UTF-8 encoded. > > +/** > > + * This routine does the work of initialization of main > > + * SQL connection instance. > > + * > > + * @param[out] db returned database handle. > > + * @return error status code. > > */ > > -static int > > -openDatabase(const char *zFilename, /* Database filename UTF-8 encoded */ > > - sqlite3 ** ppDb, /* OUT: Returned database handle */ > > - unsigned int flags, /* Operational flags */ > > - const char *zVfs) /* Name of the VFS to use */ > > +int > > +sql_init_db(sqlite3 **db) > > { > > - sqlite3 *db; /* Store allocated handle here */ > > + sqlite3 *loc_db; > > 3. If you call a parameter 'db' as 'out_db', and save the local name as 'db', then the diff becomes smaller. You can fix or not fix it at your convenience. Fixed. > > > > + loc_db->pVfs = sqlite3_vfs_find(0); > > 4. What is sqlite3_vfs_find? I do not see this in the original code. This is OS callback, which should be removed in future. It is used for now to get date/time from operating system. Hypotetically, there can be multiple different OS connectors. It was called in openDatabase, passing zero as backend requested. > > + > > + assert(sizeof(loc_db->aLimit) == sizeof(aHardLimit)); > > + memcpy(loc_db->aLimit, aHardLimit, sizeof(loc_db->aLimit)); > > + loc_db->aLimit[SQLITE_LIMIT_WORKER_THREADS] = SQLITE_DEFAULT_WORKER_THREADS; > 5. Please, try to fit code in 80 symbols. Fixed w/ new name :) > > @@ -2422,27 +2371,42 @@ openDatabase(const char *zFilename, /* Database filename UTF-8 encoded */ > > > > - if (db) { > > - assert(db->mutex != 0 || isThreadsafe == 0 > > + if (rc == SQLITE_OK) { > > + struct session *user_session = current_session(); > > + int commit_internal = !(user_session->sql_flags > > + & SQLITE_InternChanges); > > + > > + assert(loc_db->init.busy == 0); > > + loc_db->init.busy = 1; > > + loc_db->pSchema = sqlite3SchemaCreate(loc_db); > > + rc = sqlite3InitDatabase(loc_db); > > + if (rc != SQLITE_OK) > > + sqlite3SchemaClear(loc_db); > > + loc_db->init.busy = 0; > > 6. Why do you do it both in rollback and here? Since this was part of sqlite4Init, which I was erased. Next patch makes this copy paste single. -- Kirill