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 AD196262B6 for ; Mon, 11 Feb 2019 13:24:17 -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 BKvjHFyDaopw for ; Mon, 11 Feb 2019 13:24:17 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 3CDCE262AC for ; Mon, 11 Feb 2019 13:24:17 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v3] sql: remove useless pragmas and unused code From: "n.pettik" In-Reply-To: <4ebcc5a8-d1e0-d7db-98b1-d830802598bf@tarantool.org> Date: Mon, 11 Feb 2019 21:24:14 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20190208101947.8945-1-szudin@tarantool.org> <4ebcc5a8-d1e0-d7db-98b1-d830802598bf@tarantool.org> 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: tarantool-patches@freelists.org Cc: szudin@tarantool.org > On 11 Feb 2019, at 16:17, Stanislav Zudin = wrote: >=20 > On 08.02.2019 22:12, n.pettik wrote: >>=20 >>> On 8 Feb 2019, at 13:19, Stanislav Zudin = wrote: >>>=20 >>> 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. >>>=20 >>> Closes #3733 >> Please, fix commit subject: now it is not only about removal of = useless pragmas. > Ok. Please, don=E2=80=99t 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 =E2=80=94continue. 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=E2=80=99s 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 =3D=3D NULL || tbl_name =3D=3D NULL) >>> + if (idx_name =3D=3D NULL) { >>> + diag_set(ClientError, ER_ILLEGAL_PARAMS, >>> + "index name was not specified"); >>> + parse->rc =3D SQL_TARANTOOL_ERROR; >>> + parse->nErr++; >>> + return; >>> + } >> I=E2=80=99m sorry, but it seems that I interpreted comment from >> P.Gulutzan in a wrong way. We shouldn=E2=80=99t 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 =3D=3D NULL) { >>> + diag_set(ClientError, ER_ILLEGAL_PARAMS, >>> + "table name was not specified"); >>> + parse->rc =3D SQL_TARANTOOL_ERROR; >>> + parse->nErr++; >>> return; >>> + } >>> + >>> struct space *space =3D space_by_name(tbl_name); >>> - if (space =3D=3D NULL) >>> + if (space =3D=3D NULL) { >>> + diag_set(ClientError, ER_NO_SUCH_SPACE, >>> + tbl_name); >>> + parse->rc =3D SQL_TARANTOOL_ERROR; >>> + parse->nErr++; >>> return; >>> + } >>> + >>> uint32_t iid =3D box_index_id_by_name(space->def->id, idx_name, >>> strlen(idx_name)); >>> - if (iid =3D=3D BOX_ID_NIL) >>> + if (iid =3D=3D BOX_ID_NIL) { >>> + sqlite3ErrorMsg(parse, "no such index: %s", idx_name); >> Note that in this case you don=E2=80=99t 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 =3D space_index(space, iid); >>> assert(idx !=3D NULL); >>> parse->nMem =3D 6; >>> @@ -389,18 +411,39 @@ sql_pragma_index_list(struct Parse *parse, = const char *tbl_name) >>> if (tbl_name =3D=3D NULL) >>> return; >>> struct space *space =3D space_by_name(tbl_name); >>> - if (space =3D=3D NULL) >>> + if (space =3D=3D NULL) { >>> + diag_set(ClientError, ER_NO_SUCH_SPACE, >>> + tbl_name); >>> + parse->rc =3D SQL_TARANTOOL_ERROR; >>> + parse->nErr++; >> Seems that this change should be discarded as well. > Done >>=20 >>> 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 =3D=3D 0) >>> + return true; >>> + return (pToken->n >=3D 2) && >>> + (pToken->z[0] =3D=3D '\'') && >>> + (pToken->z[pToken->n - 1] =3D=3D '\''); >>> +} >> I fixed codestyle violations: >>=20 >> 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 =3D=3D 0) >> + if (token =3D=3D NULL || token->n =3D=3D 0) >> return true; >> - return (pToken->n >=3D 2) && >> - (pToken->z[0] =3D=3D '\'') && >> - (pToken->z[pToken->n - 1] =3D=3D '\''); >> + return token->n >=3D 2 && token->z[0] =3D=3D '\'' && >> + token->z[token->n - 1] =3D=3D '\''; >> } > thx. >> /* >>=20 >>> + >>> /* >>> * Process a pragma statement. >>> * >>> @@ -601,6 +644,14 @@ sqlite3Pragma(Parse * pParse, Token * pId, = /* First part of [schema.]id field */ >>> } >>>=20 >>> case PragTyp_DEFAULT_ENGINE: { >>> + if (!token_is_string(pValue)) >> I=E2=80=99d 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: >>=20 >> if (=E2=80=A6) { > fixed. >>=20 >>> + diag_set(ClientError, ER_ILLEGAL_PARAMS, >>> + "string value is expected=E2=80=9D); >>> + pParse->rc =3D SQL_TARANTOOL_ERROR; >>> + pParse->nErr++; >>> + goto pragma_out; >>> + } >>> if (sql_default_engine_set(zRight) !=3D 0) { >>> pParse->rc =3D 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 >>> +=E2=80=94 >> In sql-tap/ tests we don=E2=80=99t need cleanup: >> it is done automatically after execution. >>=20 > 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. >=20 > 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. >=20 > Closes #3733 > --- > Branch: = https://github.com/tarantool/tarantool/tree/stanztt/gh-3733-obsolete-pragm= as > Issue: https://github.com/tarantool/tarantool/issues/3733 >=20 > 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(-) >=20 > 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 =3D=3D NULL) { > - diag_set(ClientError, ER_ILLEGAL_PARAMS, > - "index name was not specified"); > - parse->rc =3D SQL_TARANTOOL_ERROR; > - parse->nErr++; > - return; > - } > - if(tbl_name =3D=3D NULL) { > - diag_set(ClientError, ER_ILLEGAL_PARAMS, > - "table name was not specified"); > - parse->rc =3D SQL_TARANTOOL_ERROR; > - parse->nErr++; > + if (idx_name =3D=3D NULL || tbl_name =3D=3D NULL) > return; > - } > - > struct space *space =3D space_by_name(tbl_name); > - if (space =3D=3D NULL) { > - diag_set(ClientError, ER_NO_SUCH_SPACE, > - tbl_name); > - parse->rc =3D SQL_TARANTOOL_ERROR; > - parse->nErr++; > + if (space =3D=3D NULL) > return; > - } > - > uint32_t iid =3D box_index_id_by_name(space->def->id, idx_name, > strlen(idx_name)); > - if (iid =3D=3D BOX_ID_NIL) { > - sqlite3ErrorMsg(parse, "no such index: %s", idx_name); > + if (iid =3D=3D BOX_ID_NIL) > return; > - } > - > struct index *idx =3D space_index(space, iid); > assert(idx !=3D NULL); > parse->nMem =3D 6; > @@ -411,13 +389,8 @@ sql_pragma_index_list(struct Parse *parse, const = char *tbl_name) > if (tbl_name =3D=3D NULL) > return; > struct space *space =3D space_by_name(tbl_name); > - if (space =3D=3D NULL) { > - diag_set(ClientError, ER_NO_SUCH_SPACE, > - tbl_name); > - parse->rc =3D SQL_TARANTOOL_ERROR; > - parse->nErr++; > + if (space =3D=3D NULL) > return; > - } > parse->nMem =3D 3; > struct Vdbe *v =3D sqlite3GetVdbe(parse); > for (uint32_t i =3D 0; i < space->index_count; ++i) { > @@ -430,18 +403,18 @@ sql_pragma_index_list(struct Parse *parse, const = char *tbl_name) >=20 > /* > * @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 =3D=3D 0) > + if (!token || token->n =3D=3D 0) > return true; > - return (pToken->n >=3D 2) && > - (pToken->z[0] =3D=3D '\'') && > - (pToken->z[pToken->n - 1] =3D=3D '\''); > + return token->n >=3D 2 && token->z[0] =3D=3D '\'' && > + token->z[token->n - 1] =3D=3D '\''; > } >=20 > /* > @@ -643,8 +616,7 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* = First part of [schema.]id field */ > } >=20 > 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 =3D 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 >=20 > -/* > - * 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 =3D require("sqltester") >=20 > -test:plan(18) > +test:plan(17) >=20 > --- > --- Prerequisites > @@ -53,51 +53,55 @@ test:do_execsql_test( > }) >=20 > --- > ---- 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" > + -- > + -- > }) >=20 > --- > ---- 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' > - -- > + -- > }) >=20 > -test:do_catchsql_test( > +test:do_execsql_test( > "pragma-5.2", > [[ > pragma index_info(no_table); > ]], { > - 1, "Illegal parameters, table name was not specified" > + -- > + -- > }) >=20 > -test:do_catchsql_test( > +test:do_execsql_test( > "pragma-5.3", > [[ > pragma index_info(wrong_table.IDX); > ]], { > - 1, "Space 'WRONG_TABLE' does not exist" > + -- > + -- > }) >=20 > -test:do_catchsql_test( > +test:do_execsql_test( > "pragma-5.4", > [[ > pragma index_info(gh3733.wrong_index); > ]], { > - 1, "no such index: WRONG_INDEX" > + -- > + -- > }) >=20 > --- > @@ -170,15 +174,4 @@ test:do_execsql_test( > -- > }) >=20 > ---- > ---- cleanup > ---- > -test:do_execsql_test( > - "pragma-8.1", > - [[ > - DROP TABLE IF EXISTS gh3733; > - ]], { > - > -}) > - > test:finish_test() > --=20 > 2.17.1 >=20 >=20