[tarantool-patches] Re: [PATCH 1/2] sql: refactor initialization routines

[Kirill Yukhin <kyukhin@tarantool.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 <kyukhin@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