[tarantool-patches] Re: [PATCH v2] sql: remove useless pragmas

[n.pettik]

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

> ---
> 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().

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

> 		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] == '\'';
 }
 
 /*

> +
> /*
>  * 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.

> +		{

Nit: put bracer at the previous line:

if (…) {

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

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