[tarantool-patches] [PATCH v3] sql: remove useless pragmas and unused code

[Stanislav Zudin <szudin@tarantool.org>]

On 08.02.2019 22:12, n.pettik wrote:
>
>> On 8 Feb 2019, at 13:19, Stanislav Zudin <szudin@tarantool.org> 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-4.1>
+    -- </pragma-4.1>
  })

  ---
---- 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)
      ]], {
-    -- <limit-1.2.1>
+    -- <pragma-5.1>
      0, 0, 'ID', 0, 'BINARY', 'integer'
-    -- </limit-1.2.1>
+    -- </pragma-5.1>
  })

-test:do_catchsql_test(
+test:do_execsql_test(
      "pragma-5.2",
      [[
          pragma index_info(no_table);
      ]], {
-    1, "Illegal parameters, table name was not specified"
+    -- <pragma-5.2>
+    -- </pragma-5.2>
  })

-test:do_catchsql_test(
+test:do_execsql_test(
      "pragma-5.3",
      [[
          pragma index_info(wrong_table.IDX);
      ]], {
-    1, "Space 'WRONG_TABLE' does not exist"
+    -- <pragma-5.3>
+    -- </pragma-5.3>
  })

-test:do_catchsql_test(
+test:do_execsql_test(
      "pragma-5.4",
      [[
          pragma index_info(gh3733.wrong_index);
      ]], {
-    1, "no such index: WRONG_INDEX"
+    -- <pragma-5.4>
+    -- </pragma-5.4>
  })

  ---
@@ -170,15 +174,4 @@ test:do_execsql_test(
      -- </pragma-7.7>
  })

----
---- cleanup
----
-test:do_execsql_test(
-    "pragma-8.1",
-    [[
-        DROP TABLE IF EXISTS gh3733;
-    ]], {
-
-})
-
  test:finish_test()
-- 
2.17.1