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

[v.shpilevoy at tarantool.org]

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".

> 
> 	if (pName == 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 == 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?

> +		db->pSchema = sqlite3SchemaCreate(db);
> +		int rc = sqlite3InitDatabase(db);
> +		if (rc != SQLITE_OK)
> +			sqlite3SchemaClear(db);
> +		db->init.busy = 0;
> +		if (rc == SQLITE_OK)
> +			sqlite3CommitInternalChanges();
> 	}
> 
> 	/* Any deferred constraint violations have now been resolved. */
> @@ -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.

> 
> +	loc_db->pVfs = sqlite3_vfs_find(0);

4. What is sqlite3_vfs_find? I do not see this in the original code.

> +
> +	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.

> @@ -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?
> 
>