[tarantool-patches] Re: [PATCH 1/2] sql: refactor initialization routines
Kirill Yukhin
kyukhin at tarantool.org
Tue Mar 20 11:28:49 MSK 2018
Hello Vlad,
I've fixed/commented your inputs. Could you pls take a look?
On 16 мар 23:32, v.shpilevoy at tarantool.org wrote:
> See below 6 comments.
>
> > 15 марта 2018 г., в 21:21, Kirill Yukhin <kyukhin at tarantool.org> написал(а):
> >
> > 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
More information about the Tarantool-patches
mailing list