[tarantool-patches] [PATCH v3] sql: remove useless pragmas and unused code
Stanislav Zudin
szudin at tarantool.org
Mon Feb 11 16:17:18 MSK 2019
On 08.02.2019 22:12, n.pettik wrote:
>
>> On 8 Feb 2019, at 13:19, Stanislav Zudin <szudin at 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
More information about the Tarantool-patches
mailing list