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 3FDF52C1D8 for ; Fri, 16 Mar 2018 16:32:27 -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 eCelIIiuInUr for ; Fri, 16 Mar 2018 16:32:27 -0400 (EDT) Received: from smtp18.mail.ru (smtp18.mail.ru [94.100.176.155]) (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 7FED82B230 for ; Fri, 16 Mar 2018 16:32:26 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: [tarantool-patches] Re: [PATCH 1/2] sql: refactor initialization routines From: "v.shpilevoy@tarantool.org" In-Reply-To: <0bf2e1909c97298d450fc4e0612cf10a1e4403d0.1521137308.git.kyukhin@tarantool.org> Date: Fri, 16 Mar 2018 23:32:23 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <3F4C5B6B-9887-4A4A-9DE2-5D53A9EFC06A@tarantool.org> References: <0bf2e1909c97298d450fc4e0612cf10a1e4403d0.1521137308.git.kyukhin@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: tarantool-patches@freelists.org Cc: Kirill Yukhin See below 6 comments. > 15 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2018 =D0=B3., =D0=B2 21:21, Kirill = Yukhin =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(= =D0=B0): >=20 > 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. >=20 > 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. >=20 > 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; >=20 > - /* Read the database schema. If an error occurs, leave an error = message > - * and code in pParse and return NULL. > - */ > - if (SQLITE_OK !=3D sqlite3ReadSchema(pParse)) { > - return; > - } > + assert(db->pSchema); 1. Please, use explicit "!=3D NULL". >=20 > if (pName =3D=3D 0) { > /* Form 1: Analyze everything */ > 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 =3D=3D 0) { > sqlite3ExpirePreparedStatements(db); > sqlite3ResetAllSchemasOfConnection(db); > + > + db->init.busy =3D 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? > + db->pSchema =3D sqlite3SchemaCreate(db); > + int rc =3D sqlite3InitDatabase(db); > + if (rc !=3D SQLITE_OK) > + sqlite3SchemaClear(db); > + db->init.busy =3D 0; > + if (rc =3D=3D SQLITE_OK) > + sqlite3CommitInternalChanges(); > } >=20 > /* Any deferred constraint violations have now been resolved. */ > @@ -2246,122 +2255,62 @@ sqlite3ParseUri(const char *zDefaultVfs, = /* VFS to use if no "vfs=3Dxxx" query opt > return rc; > } >=20 > -/* > - * 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. >=20 > + loc_db->pVfs =3D sqlite3_vfs_find(0); 4. What is sqlite3_vfs_find? I do not see this in the original code. > + > + assert(sizeof(loc_db->aLimit) =3D=3D sizeof(aHardLimit)); > + memcpy(loc_db->aLimit, aHardLimit, sizeof(loc_db->aLimit)); > + loc_db->aLimit[SQLITE_LIMIT_WORKER_THREADS] =3D = SQLITE_DEFAULT_WORKER_THREADS; 5. Please, try to fit code in 80 symbols. > @@ -2422,27 +2371,42 @@ openDatabase(const char *zFilename, /* = Database filename UTF-8 encoded */ >=20 > - if (db) { > - assert(db->mutex !=3D 0 || isThreadsafe =3D=3D 0 > + if (rc =3D=3D SQLITE_OK) { > + struct session *user_session =3D current_session(); > + int commit_internal =3D !(user_session->sql_flags > + & SQLITE_InternChanges); > + > + assert(loc_db->init.busy =3D=3D 0); > + loc_db->init.busy =3D 1; > + loc_db->pSchema =3D sqlite3SchemaCreate(loc_db); > + rc =3D sqlite3InitDatabase(loc_db); > + if (rc !=3D SQLITE_OK) > + sqlite3SchemaClear(loc_db); > + loc_db->init.busy =3D 0; 6. Why do you do it both in rollback and here? >=20 >=20