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

  • From: "v.shpilevoy@xxxxxxxxxxxxx" <v.shpilevoy@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Fri, 16 Mar 2018 23:32:23 +0300

See below 6 comments.

15 марта 2018 г., в 21:21, Kirill Yukhin <kyukhin@xxxxxxxxxxxxx> написал(а):

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?




Other related posts: