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

["n.pettik" <korablev@tarantool.org>]

> On 11 Feb 2019, at 16:17, Stanislav Zudin <szudin@tarantool.org> wrote:
> 
> 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.

Please, don’t put fixes to a HEAD~2 commit in a separate commit -
it is impossible to review. What you need to do is (interactive) rebase:
git rebase -i HEAD~2

Then pick commits to be edited and fix whatever you need.
After that, add changed files and continue rebasing with
git rebase —continue. However, in your case fixes are already
done, but they should be split into two commits. To process
this procedure, you should reset HEAD:

git reset HEAD~

This command will drop head commit and move all changed
made in that commit into unstaged area.
Then, by using interactive add command (git add -i) you
form two commits: one will contains fixes for the first commit
(where you remove all pragmas except for busy_handler),
another one - for the second commit.

Then, you will get this:

commit #4 Fixes for commit #2
commit #3 Fixes for commit #1
commit #2 sql: remove busy handler routine
commit #1 sql: remove useless pragmas

After that, you use git rebase:

git rebase -I HEAD~4

In rebase menu you can move commits. Lets make this:

commit #4 Fixes for commit #2
commit #3 sql: remove busy handler routine
commit #2 Fixes for commit #1
commit #1 sql: remove useless pragmas

Finally, you should squash commit #2 into commit #1
and commit #4 into commit #3. To do that in rebase
menu set fixup flag near commits #2 and #4.
That’s it. After you finish procedure, please push updated
patches and send them in whatever form you want.

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