From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id F1C792733C for ; Mon, 11 Feb 2019 08:17:21 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id UieFgJH2KdW1 for ; Mon, 11 Feb 2019 08:17:21 -0500 (EST) Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6D41627317 for ; Mon, 11 Feb 2019 08:17:21 -0500 (EST) Subject: [tarantool-patches] [PATCH v3] sql: remove useless pragmas and unused code References: <20190208101947.8945-1-szudin@tarantool.org> From: Stanislav Zudin Message-ID: <4ebcc5a8-d1e0-d7db-98b1-d830802598bf@tarantool.org> Date: Mon, 11 Feb 2019 16:17:18 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.org On 08.02.2019 22:12, n.pettik wrote: > >> On 8 Feb 2019, at 13:19, Stanislav Zudin wrote: >> >> The pragmas "query_only" and "read_uncommitted" didn't affect anything >> and were removed. >> Fixed an error in pragma index_list which caused a segmantation fault. >> pragmas index_list and index_info return an error when receive an >> invalid table or index names. >> pragma sql_default_engine accepts only strings. >> Thus pragma sql_default_engine('memtx') is a well-formed command, >> while pragma sql_default_engine(memtx) or >> pragma sql_default_engine("memtx") are considered as an ill-formed and >> raise an error. >> >> Closes #3733 > Please, fix commit subject: now it is not only about removal of useless pragmas. Ok. > >> --- >> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c >> index eef1ed931..158e6269b 100644 >> --- a/src/box/sql/pragma.c >> +++ b/src/box/sql/pragma.c >> @@ -342,15 +342,37 @@ sql_pragma_index_info(struct Parse *parse, >> MAYBE_UNUSED const PragmaName *pragma, >> const char *tbl_name, const char *idx_name) >> { >> - if (idx_name == NULL || tbl_name == NULL) >> + if (idx_name == NULL) { >> + diag_set(ClientError, ER_ILLEGAL_PARAMS, >> + "index name was not specified"); >> + parse->rc = SQL_TARANTOOL_ERROR; >> + parse->nErr++; >> + return; >> + } > I’m sorry, but it seems that I interpreted comment from > P.Gulutzan in a wrong way. We shouldn’t throw any errors, > according to his clarification. Please, check out his comment > and make sure this time I am right. If really so, please discard > changes to sql_pragma_index_info(). Done. >> + if(tbl_name == NULL) { >> + diag_set(ClientError, ER_ILLEGAL_PARAMS, >> + "table name was not specified"); >> + parse->rc = SQL_TARANTOOL_ERROR; >> + parse->nErr++; >> return; >> + } >> + >> struct space *space = space_by_name(tbl_name); >> - if (space == NULL) >> + if (space == NULL) { >> + diag_set(ClientError, ER_NO_SUCH_SPACE, >> + tbl_name); >> + parse->rc = SQL_TARANTOOL_ERROR; >> + parse->nErr++; >> return; >> + } >> + >> uint32_t iid = box_index_id_by_name(space->def->id, idx_name, >> strlen(idx_name)); >> - if (iid == BOX_ID_NIL) >> + if (iid == BOX_ID_NIL) { >> + sqlite3ErrorMsg(parse, "no such index: %s", idx_name); > Note that in this case you don’t need set sqlite error: > box_index_id_by_name will set diag message. So, you should > only set appropriate error code in parse struct. >> return; >> + } >> + >> struct index *idx = space_index(space, iid); >> assert(idx != NULL); >> parse->nMem = 6; >> @@ -389,18 +411,39 @@ sql_pragma_index_list(struct Parse *parse, const char *tbl_name) >> if (tbl_name == NULL) >> return; >> struct space *space = space_by_name(tbl_name); >> - if (space == NULL) >> + if (space == NULL) { >> + diag_set(ClientError, ER_NO_SUCH_SPACE, >> + tbl_name); >> + parse->rc = SQL_TARANTOOL_ERROR; >> + parse->nErr++; > Seems that this change should be discarded as well. Done > >> return; >> +/* >> + * @brief Check whether the specified token is a string or ID. >> + * @param pToken - token to be examined >> + * @return true - if the token value is enclosed into quotes (') >> + * @return false in other cases >> + * The empty value is considered to be a string. >> + */ >> +static bool token_is_string(const Token* pToken) >> +{ >> + if (!pToken || pToken->n == 0) >> + return true; >> + return (pToken->n >= 2) && >> + (pToken->z[0] == '\'') && >> + (pToken->z[pToken->n - 1] == '\''); >> +} > I fixed codestyle violations: > > diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c > index 158e6269b..3938ae429 100644 > --- a/src/box/sql/pragma.c > +++ b/src/box/sql/pragma.c > @@ -428,20 +428,20 @@ sql_pragma_index_list(struct Parse *parse, const char *tbl_name) > } > } > > -/* > +/** > * @brief Check whether the specified token is a string or ID. > - * @param pToken - token to be examined > + * @param token - token to be examined > * @return true - if the token value is enclosed into quotes (') > * @return false in other cases > * The empty value is considered to be a string. > */ > -static bool token_is_string(const Token* pToken) > +static bool > +token_is_string(const struct Token *token) > { > - if (!pToken || pToken->n == 0) > + if (token == NULL || token->n == 0) > return true; > - return (pToken->n >= 2) && > - (pToken->z[0] == '\'') && > - (pToken->z[pToken->n - 1] == '\''); > + return token->n >= 2 && token->z[0] == '\'' && > + token->z[token->n - 1] == '\''; > } thx. > > /* > >> + >> /* >> * Process a pragma statement. >> * >> @@ -601,6 +644,14 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ >> } >> >> case PragTyp_DEFAULT_ENGINE: { >> + if (!token_is_string(pValue)) > I’d better use sql_token() function. sql_token() examines every character in a string, while token_is_string() - only the leading and the trailing ones. But if you insist i'll use sql_token. >> + { > Nit: put bracer at the previous line: > > if (…) { fixed. > >> + diag_set(ClientError, ER_ILLEGAL_PARAMS, >> + "string value is expected”); >> + pParse->rc = SQL_TARANTOOL_ERROR; >> + pParse->nErr++; >> + goto pragma_out; >> + } >> if (sql_default_engine_set(zRight) != 0) { >> pParse->rc = SQL_TARANTOOL_ERROR; >> pParse->nErr++; >> diff --git a/test/sql-tap/gh-3733-pragma.test.lua b/test/sql-tap/gh-3733-pragma.test.lua >> new file mode 100755 >> index 000000000..5b0dcd26a >> --- /dev/null >> +--- >> +--- cleanup >> +— > In sql-tap/ tests we don’t need cleanup: > it is done automatically after execution. > Got it. Removed the final "drop table" statement. >> +test:do_execsql_test( >> + "pragma-8.1", >> + [[ >> + DROP TABLE IF EXISTS gh3733; >> + ]], { >> + >> +}) >> + >> +test:finish_test() The code changes are below. Removed the last traces of SQLite's busyHandler. Removed error handling, so the pragmas index_info & index_list return an empty tuple in a case of error. Closes #3733 --- Branch: https://github.com/tarantool/tarantool/tree/stanztt/gh-3733-obsolete-pragmas Issue: https://github.com/tarantool/tarantool/issues/3733  src/box/sql/pragma.c                 | 50 ++++++----------------------  src/box/sql/sqliteInt.h              | 17 ----------  test/sql-tap/gh-3733-pragma.test.lua | 41 ++++++++++-------------  3 files changed, 28 insertions(+), 80 deletions(-) diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index 7391f034b..62ff1f2b2 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -342,37 +342,15 @@ sql_pragma_index_info(struct Parse *parse,                MAYBE_UNUSED const PragmaName *pragma,                const char *tbl_name, const char *idx_name)  { -    if (idx_name == NULL) { -        diag_set(ClientError, ER_ILLEGAL_PARAMS, -             "index name was not specified"); -        parse->rc = SQL_TARANTOOL_ERROR; -        parse->nErr++; -        return; -    } -    if(tbl_name == NULL) { -        diag_set(ClientError, ER_ILLEGAL_PARAMS, -             "table name was not specified"); -        parse->rc = SQL_TARANTOOL_ERROR; -        parse->nErr++; +    if (idx_name == NULL || tbl_name == NULL)          return; -    } -      struct space *space = space_by_name(tbl_name); -    if (space == NULL) { -        diag_set(ClientError, ER_NO_SUCH_SPACE, -             tbl_name); -        parse->rc = SQL_TARANTOOL_ERROR; -        parse->nErr++; +    if (space == NULL)          return; -    } -      uint32_t iid = box_index_id_by_name(space->def->id, idx_name,                          strlen(idx_name)); -    if (iid == BOX_ID_NIL) { -        sqlite3ErrorMsg(parse, "no such index: %s", idx_name); +    if (iid == BOX_ID_NIL)          return; -    } -      struct index *idx = space_index(space, iid);      assert(idx != NULL);      parse->nMem = 6; @@ -411,13 +389,8 @@ sql_pragma_index_list(struct Parse *parse, const char *tbl_name)      if (tbl_name == NULL)          return;      struct space *space = space_by_name(tbl_name); -    if (space == NULL) { -        diag_set(ClientError, ER_NO_SUCH_SPACE, -             tbl_name); -        parse->rc = SQL_TARANTOOL_ERROR; -        parse->nErr++; +    if (space == NULL)          return; -    }      parse->nMem = 3;      struct Vdbe *v = sqlite3GetVdbe(parse);      for (uint32_t i = 0; i < space->index_count; ++i) { @@ -430,18 +403,18 @@ sql_pragma_index_list(struct Parse *parse, const char *tbl_name)  /*   * @brief Check whether the specified token is a string or ID. - * @param pToken - token to be examined + * @param token - token to be examined   * @return true - if the token value is enclosed into quotes (')   * @return false in other cases   * The empty value is considered to be a string.   */ -static bool token_is_string(const Token* pToken) +static bool +token_is_string(const struct Token* token)  { -    if (!pToken || pToken->n == 0) +    if (!token || token->n == 0)          return true; -    return  (pToken->n >= 2) && -        (pToken->z[0] == '\'') && -        (pToken->z[pToken->n - 1] == '\''); +    return token->n >= 2 && token->z[0] == '\'' && +           token->z[token->n - 1] == '\'';  }  /* @@ -643,8 +616,7 @@ sqlite3Pragma(Parse * pParse, Token * pId,    /* First part of [schema.]id field */          }      case PragTyp_DEFAULT_ENGINE: { -        if (!token_is_string(pValue)) -        { +        if (!token_is_string(pValue)) {              diag_set(ClientError, ER_ILLEGAL_PARAMS,                   "string value is expected");              pParse->rc = SQL_TARANTOOL_ERROR; diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 50668b775..1633c1a35 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -824,7 +824,6 @@ struct sqlite3_io_methods {  #define SQLITE_FCNTL_VFSNAME                11  #define SQLITE_FCNTL_POWERSAFE_OVERWRITE    12  #define SQLITE_FCNTL_PRAGMA                 13 -#define SQLITE_FCNTL_BUSYHANDLER            14  #define SQLITE_FCNTL_TEMPFILENAME           15  #define SQLITE_FCNTL_MMAP_SIZE              16  #define SQLITE_FCNTL_TRACE                  17 @@ -1306,22 +1305,6 @@ extern const int sqlite3one;  #undef WHERETRACE_ENABLED  #endif -/* - * An instance of the following structure is used to store the busy-handler - * callback for a given sqlite handle. - * - * The sqlite.busyHandler member of the sqlite struct contains the busy - * callback for the database handle. Each pager opened via the sqlite - * handle is passed a pointer to sqlite.busyHandler. The busy-handler - * callback is currently invoked only from within pager.c. - */ -typedef struct BusyHandler BusyHandler; -struct BusyHandler { -    int (*xFunc) (void *, int);    /* The busy callback */ -    void *pArg;        /* First arg to busy callback */ -    int nBusy;        /* Incremented with each busy call */ -}; -  /*   * A convenience macro that returns the number of elements in   * an array. diff --git a/test/sql-tap/gh-3733-pragma.test.lua b/test/sql-tap/gh-3733-pragma.test.lua index 5b0dcd26a..216f0f8dd 100755 --- a/test/sql-tap/gh-3733-pragma.test.lua +++ b/test/sql-tap/gh-3733-pragma.test.lua @@ -1,7 +1,7 @@  #!/usr/bin/env tarantool  test = require("sqltester") -test:plan(18) +test:plan(17)  ---  --- Prerequisites @@ -53,51 +53,55 @@ test:do_execsql_test(  })  --- ---- pragma index_list returns an error for unknown table +--- pragma index_list returns an empty tuple for unknown table  --- -test:do_catchsql_test( +test:do_execsql_test(      "pragma-4.1",      [[          pragma index_list(fufel);      ]], { -    1, "Space 'FUFEL' does not exist" +    -- +    --  })  --- ---- pragma index_info returns an error for unknown index +--- pragma index_info returns an empty tuple for unknown index  ---  test:do_execsql_test(      "pragma-5.1",      [[          pragma index_info(gh3733.IDX)      ]], { -    -- +    --      0, 0, 'ID', 0, 'BINARY', 'integer' -    -- +    --  }) -test:do_catchsql_test( +test:do_execsql_test(      "pragma-5.2",      [[          pragma index_info(no_table);      ]], { -    1, "Illegal parameters, table name was not specified" +    -- +    --  }) -test:do_catchsql_test( +test:do_execsql_test(      "pragma-5.3",      [[          pragma index_info(wrong_table.IDX);      ]], { -    1, "Space 'WRONG_TABLE' does not exist" +    -- +    --  }) -test:do_catchsql_test( +test:do_execsql_test(      "pragma-5.4",      [[          pragma index_info(gh3733.wrong_index);      ]], { -    1, "no such index: WRONG_INDEX" +    -- +    --  })  --- @@ -170,15 +174,4 @@ test:do_execsql_test(      --  }) ---- ---- cleanup ---- -test:do_execsql_test( -    "pragma-8.1", -    [[ -        DROP TABLE IF EXISTS gh3733; -    ]], { - -}) -  test:finish_test() -- 2.17.1