[tarantool-patches] Re: [PATCH 1/2] sql: remove obsolete SQLite routine

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: Nikita Pettik <korablev@xxxxxxxxxxxxx>, tarantool-patches@xxxxxxxxxxxxx
  • Date: Tue, 3 Apr 2018 20:54:56 +0300

Hello. See 7 comments below.

03.04.2018 19:14, Nikita Pettik пишет:

Some of legacy functions seem to be useless, since they serve as
wrappers around others; the rest rely on capabilities which are no
longer relevant. This patch provides slight refactoring of such
functions.

Removed entities:
  - sqlite3LocateTableItem() - replaced with sqlite3LocateTable();
  - sqlite3FindTable() - replaced with sqlite3HashFind();
  - sqlite3ColumnOfIndex() - in Tarantool order of columns always the same;
  - sqlite3FindIndex() - replaced with sqlite3LocateIndex();
  - sqlite3CodeVerifySchema();
  - sqlite3SchemaToIndex();
  - sqlite3MultiWrite();
  - Parse->cookieMast;
  - Vdbe->usesStmtJournal;
---
  src/box/sql/alter.c     |  13 ++-
  src/box/sql/analyze.c   |  25 +++---
  src/box/sql/build.c     | 213 ++++++++++--------------------------------------
  src/box/sql/delete.c    |   5 +-
  src/box/sql/expr.c      |   9 +-
  src/box/sql/fkey.c      |   3 +-
  src/box/sql/insert.c    |  10 +--
  src/box/sql/pragma.c    |  18 ++--
  src/box/sql/prepare.c   |  29 -------
  src/box/sql/select.c    |   4 +-
  src/box/sql/sqliteInt.h |  11 +--
  src/box/sql/trigger.c   |   7 +-
  src/box/sql/update.c    |   2 +-
  src/box/sql/vdbe.c      |   1 -
  src/box/sql/vdbeInt.h   |   1 -
  src/box/sql/vdbeaux.c   |   1 -
  src/box/sql/where.c     |   2 -
  src/box/sql/wherecode.c |   7 +-
  18 files changed, 83 insertions(+), 278 deletions(-)

diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 054c0856c..a19324ed2 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -122,7 +121,7 @@ sqlite3AlterRenameTable(Parse * pParse,     /* Parser 
context. */
        if (v == 0) {
                goto exit_rename_table;
        }
-       sqlite3BeginWriteOperation(pParse, false);
+       sql_set_multi_write(pParse, false);
1. Can you please alongside with this patch make sql_set_multi_write take boolean as the last
argument? Now it sometimes gets true/false, sometimes 1/0. It is strange.
/* Drop and reload the internal table schema. */
        reloadTableSchema(pParse, pTab, zName);

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5e3ed0f39..61194e06b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -93,23 +93,16 @@ sqlite3FinishCoding(Parse * pParse)
                 * transaction on each used database and to verify the schema 
cookie
                 * on each used database.
                 */
-               if (db->mallocFailed == 0
-                   && (DbMaskNonZero(pParse->cookieMask) || pParse->pConstExpr)
-                   ) {
+               if (db->mallocFailed == 0 || pParse->pConstExpr) {
2. Please, together with cookie mask checking remove or refactor a comment above.
@@ -2240,7 +2164,6 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, int 
isView)
v = sqlite3GetVdbe(pParse);
        assert(v != 0);
-       sqlite3BeginWriteOperation(pParse, 1);
3. Why did you delete it with no replacement by sql_set_multi_write(false) ?
@@ -2376,15 +2299,12 @@ sqlite3DropTable(Parse * pParse, SrcList * pName, int 
isView, int noErr)
        if (noErr)
                db->suppressErr++;
        assert(isView == 0 || isView == LOCATE_VIEW);
-       pTab = sqlite3LocateTableItem(pParse, isView, &pName->a[0]);
+       pTab = sqlite3LocateTable(pParse, isView, pName->a[0].zName);
        if (noErr)
                db->suppressErr--;
- if (pTab == 0) {
-               if (noErr)
-                       sqlite3CodeVerifySchema(pParse);
+       if (pTab == 0)
4. Lets use ==/!= NULL in all new code. Same about checking sqlite3HashFind results. And if it is
possible with not huge diff, can you please rename sqlite3HashFind to sql_hash_find ?

5. sqlite3MultiWrite in commit message is listed among deleted functions, but its declaration still exists.

6. cookieMast - typo in commit message. And how about do not list deleted functions in a commit body?
I can not imagine, that somebody except me will search for any of these functions. And the list is deprecated -
for example, sqlite3BeginWriteOperation is deleted too, but does not presence in the list.

7. How about remove DbMaskTest, DbMaskZero and other dbmask shit?


Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 04-2018