Tarantool development patches archive
 help / color / mirror / Atom feed
From: "v.shpilevoy@tarantool.org" <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Kirill Yukhin <kyukhin@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH 1/2] sql: refactor initialization routines
Date: Fri, 16 Mar 2018 23:32:23 +0300	[thread overview]
Message-ID: <3F4C5B6B-9887-4A4A-9DE2-5D53A9EFC06A@tarantool.org> (raw)
In-Reply-To: <0bf2e1909c97298d450fc4e0612cf10a1e4403d0.1521137308.git.kyukhin@tarantool.org>

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

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

  reply	other threads:[~2018-03-16 20:32 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-15 18:21 [tarantool-patches] [PATCH 0/2] Refactor SQL initialization, block triggers in transaction Kirill Yukhin
2018-03-15 18:21 ` [tarantool-patches] [PATCH 1/2] sql: refactor initialization routines Kirill Yukhin
2018-03-16 20:32   ` v.shpilevoy [this message]
2018-03-20  8:28     ` [tarantool-patches] " Kirill Yukhin
2018-03-20  8:35       ` [tarantool-patches] [PATCH 1/2] sql: Refactor " Kirill Yukhin
2018-03-20 10:56         ` [tarantool-patches] " v.shpilevoy
2018-03-15 18:21 ` [tarantool-patches] [PATCH 2/2] sql: block trigger creation inside a transaction Kirill Yukhin
2018-03-16 20:36   ` [tarantool-patches] " v.shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3F4C5B6B-9887-4A4A-9DE2-5D53A9EFC06A@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=kyukhin@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH 1/2] sql: refactor initialization routines' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox