[tarantool-patches] [PATCH 0/3] A set of small SQL fixes

[tarantool-patches] [PATCH 0/3] A set of small SQL fixes

[Vladislav Shpilevoy]

A set of small patches fixing some SQL assertions or incorrect behaviour,
which appeared during working on 3505 - box.sql.execute() removal.

Branch: http://github.com/tarantool/tarantool/tree/imeevma/small-patches

Mergen Imeev (3):
  sql: EXPLAIN through net.box leads to SEGFAULT
  sql: SELECT from system spaces returns unpacked msgpack.
  sql: too many autogenerated ids leads to SEGFAULT

 src/box/execute.c        | 44 ++++++++++++++++++++++++++--------------
 src/box/lua/net_box.c    | 15 +++++++-------
 src/box/sql/vdbe.c       |  8 ++------
 src/box/sql/vdbeaux.c    |  6 ------
 test/sql/iproto.result   | 40 ++++++++++++++++++++++++++++++++++++
 test/sql/iproto.test.lua | 24 +++++++++++++++++++++-
 6 files changed, 102 insertions(+), 35 deletions(-)

-- 
2.17.2 (Apple Git-113)

[tarantool-patches] [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT

[Vladislav Shpilevoy]

From: Mergen Imeev <imeevma@gmail.com>

EXPLAIN envokes SEGMENTATION FAULT when being executed through
net.box. It happens due to column type of the result of this
function being NULL.
---
 src/box/execute.c        | 22 ++++++++++++++--------
 src/box/lua/net_box.c    | 15 ++++++++-------
 test/sql/iproto.result   | 15 +++++++++++++++
 test/sql/iproto.test.lua | 13 ++++++++++++-
 4 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index e45044488..5b747cf7f 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -529,9 +529,6 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
 		return -1;
 
 	for (int i = 0; i < column_count; ++i) {
-		size_t size = mp_sizeof_map(2) +
-			      mp_sizeof_uint(IPROTO_FIELD_NAME) +
-			      mp_sizeof_uint(IPROTO_FIELD_TYPE);
 		const char *name = sqlite3_column_name(stmt, i);
 		const char *type = sqlite3_column_datatype(stmt, i);
 		/*
@@ -540,18 +537,27 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
 		 * column_name simply returns them.
 		 */
 		assert(name != NULL);
-		size += mp_sizeof_str(strlen(name));
-		size += mp_sizeof_str(strlen(type));
+		size_t size = mp_sizeof_uint(IPROTO_FIELD_NAME) +
+			      mp_sizeof_str(strlen(name));
+		if (type != NULL) {
+			size += mp_sizeof_map(2) +
+				mp_sizeof_uint(IPROTO_FIELD_TYPE) +
+				mp_sizeof_str(strlen(type));
+		} else {
+			size += mp_sizeof_map(1);
+		}
 		char *pos = (char *) obuf_alloc(out, size);
 		if (pos == NULL) {
 			diag_set(OutOfMemory, size, "obuf_alloc", "pos");
 			return -1;
 		}
-		pos = mp_encode_map(pos, 2);
+		pos = mp_encode_map(pos, 1 + (type != NULL));
 		pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
 		pos = mp_encode_str(pos, name, strlen(name));
-		pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
-		pos = mp_encode_str(pos, type, strlen(type));
+		if (type != NULL) {
+			pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
+			pos = mp_encode_str(pos, type, strlen(type));
+		}
 	}
 	return 0;
 }
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index c7063d9c8..342c6a768 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -644,8 +644,7 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
 	lua_createtable(L, count, 0);
 	for (uint32_t i = 0; i < count; ++i) {
 		uint32_t map_size = mp_decode_map(data);
-		assert(map_size == 2);
-		(void) map_size;
+		assert(map_size == 1 || map_size == 2);
 		uint32_t key = mp_decode_uint(data);
 		assert(key == IPROTO_FIELD_NAME);
 		(void) key;
@@ -654,11 +653,13 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
 		const char *str = mp_decode_str(data, &len);
 		lua_pushlstring(L, str, len);
 		lua_setfield(L, -2, "name");
-		key = mp_decode_uint(data);
-		assert(key == IPROTO_FIELD_TYPE);
-		const char *type = mp_decode_str(data, &len);
-		lua_pushlstring(L, type, len);
-		lua_setfield(L, -2, "type");
+		if (map_size == 2) {
+			key = mp_decode_uint(data);
+			assert(key == IPROTO_FIELD_TYPE);
+			const char *type = mp_decode_str(data, &len);
+			lua_pushlstring(L, type, len);
+			lua_setfield(L, -2, "type");
+		}
 		lua_rawseti(L, -2, i + 1);
 	}
 }
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index d077ee861..31e5d2e75 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -773,6 +773,21 @@ cn:execute('drop table test')
 cn:close()
 ---
 ...
+--
+-- EXPLAIN could lead to segfault because it does not send column
+-- data type, but looks like DQL. Netbox had been trying to decode
+-- type for any DQL.
+--
+cn = remote.connect(box.cfg.listen)
+---
+...
+-- Segmentation fault when EXPLAIN executed using net.box.
+_ = cn:execute("EXPLAIN SELECT 1;")
+---
+...
+cn:close()
+---
+...
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 1470edad7..a0f0f15ee 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -247,10 +247,21 @@ cn:execute('drop table test')
 
 cn:close()
 
+--
+-- EXPLAIN could lead to segfault because it does not send column
+-- data type, but looks like DQL. Netbox had been trying to decode
+-- type for any DQL.
+--
+cn = remote.connect(box.cfg.listen)
+
+-- Segmentation fault when EXPLAIN executed using net.box.
+_ = cn:execute("EXPLAIN SELECT 1;")
+
+cn:close()
+
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'create', 'space')
 space = nil
 
 -- Cleanup xlog
 box.snapshot()
-
-- 
2.17.2 (Apple Git-113)

[tarantool-patches] Re: [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT

[n.pettik]

> EXPLAIN envokes SEGMENTATION FAULT when being executed through

Typo: invokes.

> net.box. It happens due to column type of the result of this
> function being NULL.

I’ve checked and not only EXPLAIN results, but also EXPLAIN QUERY PLAN,
several pragmas (for instance, pragma table_info()) etc. So, initially problem
was not only in EXPLAIN statement. Please exposed commit message and
add tests on other cases.

Also, why did you decide to avoid sending type at all, instead of sending
UNKNOWN for example?

[tarantool-patches] Re: [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 9885 bytes --]

Hi! Thank you for review! Diff between versions and new patch
below.

On 11/19/18 8:27 PM, n.pettik wrote:

>> EXPLAIN envokes SEGMENTATION FAULT when being executed through
> Typo: invokes.
>
>> net.box. It happens due to column type of the result of this
>> function being NULL.
> I’ve checked and not only EXPLAIN results, but also EXPLAIN QUERY PLAN,
> several pragmas (for instance, pragma table_info()) etc. So, initially problem
> was not only in EXPLAIN statement. Please exposed commit message and
> add tests on other cases.
>
> Also, why did you decide to avoid sending type at all, instead of sending
> UNKNOWN for example?

*Diff between versions:*

diff --git a/src/box/execute.c b/src/box/execute.c
index 5b747cf..cd809f8 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -529,6 +529,9 @@ sql_get_description(struct sqlite3_stmt *stmt, 
struct obuf *out,
          return -1;

      for (int i = 0; i < column_count; ++i) {
+        size_t size = mp_sizeof_map(2) +
+                  mp_sizeof_uint(IPROTO_FIELD_NAME) +
+                  mp_sizeof_uint(IPROTO_FIELD_TYPE);
          const char *name = sqlite3_column_name(stmt, i);
          const char *type = sqlite3_column_datatype(stmt, i);
          /*
@@ -537,27 +540,20 @@ sql_get_description(struct sqlite3_stmt *stmt, 
struct obuf *out,
           * column_name simply returns them.
           */
          assert(name != NULL);
-        size_t size = mp_sizeof_uint(IPROTO_FIELD_NAME) +
-                  mp_sizeof_str(strlen(name));
-        if (type != NULL) {
-            size += mp_sizeof_map(2) +
-                mp_sizeof_uint(IPROTO_FIELD_TYPE) +
-                mp_sizeof_str(strlen(type));
-        } else {
-            size += mp_sizeof_map(1);
-        }
+        if (type == NULL)
+            type = "UNKNOWN";
+        size += mp_sizeof_str(strlen(name));
+        size += mp_sizeof_str(strlen(type));
          char *pos = (char *) obuf_alloc(out, size);
          if (pos == NULL) {
              diag_set(OutOfMemory, size, "obuf_alloc", "pos");
              return -1;
          }
-        pos = mp_encode_map(pos, 1 + (type != NULL));
+        pos = mp_encode_map(pos, 2);
          pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
          pos = mp_encode_str(pos, name, strlen(name));
-        if (type != NULL) {
-            pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
-            pos = mp_encode_str(pos, type, strlen(type));
-        }
+        pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
+        pos = mp_encode_str(pos, type, strlen(type));
      }
      return 0;
  }
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 342c6a7..c7063d9 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -644,7 +644,8 @@ netbox_decode_metadata(struct lua_State *L, const 
char **data)
      lua_createtable(L, count, 0);
      for (uint32_t i = 0; i < count; ++i) {
          uint32_t map_size = mp_decode_map(data);
-        assert(map_size == 1 || map_size == 2);
+        assert(map_size == 2);
+        (void) map_size;
          uint32_t key = mp_decode_uint(data);
          assert(key == IPROTO_FIELD_NAME);
          (void) key;
@@ -653,13 +654,11 @@ netbox_decode_metadata(struct lua_State *L, const 
char **data)
          const char *str = mp_decode_str(data, &len);
          lua_pushlstring(L, str, len);
          lua_setfield(L, -2, "name");
-        if (map_size == 2) {
-            key = mp_decode_uint(data);
-            assert(key == IPROTO_FIELD_TYPE);
-            const char *type = mp_decode_str(data, &len);
-            lua_pushlstring(L, type, len);
-            lua_setfield(L, -2, "type");
-        }
+        key = mp_decode_uint(data);
+        assert(key == IPROTO_FIELD_TYPE);
+        const char *type = mp_decode_str(data, &len);
+        lua_pushlstring(L, type, len);
+        lua_setfield(L, -2, "type");
          lua_rawseti(L, -2, i + 1);
      }
  }
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 31e5d2e..f229427 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -774,9 +774,9 @@ cn:close()
  ---
  ...
  --
--- EXPLAIN could lead to segfault because it does not send column
--- data type, but looks like DQL. Netbox had been trying to decode
--- type for any DQL.
+-- Some commands, e.g. EXPLAIN, could lead to segfault because it
+-- does not send column data type, but looks like DQL. Netbox had
+-- been trying to decode type for any DQL.
  --
  cn = remote.connect(box.cfg.listen)
  ---
@@ -785,6 +785,32 @@ cn = remote.connect(box.cfg.listen)
  _ = cn:execute("EXPLAIN SELECT 1;")
  ---
  ...
+-- Segmentation fault when PRAGMA TABLE_INFO() executed using
+-- net.box.
+box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
+---
+...
+cn:execute("PRAGMA TABLE_INFO(test);")
+---
+- metadata:
+  - name: cid
+    type: UNKNOWN
+  - name: name
+    type: UNKNOWN
+  - name: type
+    type: UNKNOWN
+  - name: notnull
+    type: UNKNOWN
+  - name: dflt_value
+    type: UNKNOWN
+  - name: pk
+    type: UNKNOWN
+  rows:
+  - [0, 'ID', 'integer', 1, null, 1]
+...
+box.sql.execute('DROP TABLE test')
+---
+...
  cn:close()
  ---
  ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index a0f0f15..9bcc7ef 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -248,15 +248,21 @@ cn:execute('drop table test')
  cn:close()

  --
--- EXPLAIN could lead to segfault because it does not send column
--- data type, but looks like DQL. Netbox had been trying to decode
--- type for any DQL.
+-- Some commands, e.g. EXPLAIN, could lead to segfault because it
+-- does not send column data type, but looks like DQL. Netbox had
+-- been trying to decode type for any DQL.
  --
  cn = remote.connect(box.cfg.listen)

  -- Segmentation fault when EXPLAIN executed using net.box.
  _ = cn:execute("EXPLAIN SELECT 1;")

+-- Segmentation fault when PRAGMA TABLE_INFO() executed using
+-- net.box.
+box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
+cn:execute("PRAGMA TABLE_INFO(test);")
+box.sql.execute('DROP TABLE test')
+
  cn:close()

  box.schema.user.revoke('guest', 'read,write,execute', 'universe')


*New version:*

commit 29dde0365922bfcec0d159b36c6454854adf34f7
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Nov 16 13:34:46 2018 +0300

     sql: Print column type even if it is NULL

     Some commands, e.g. EXPLAIN, invokes SEGMENTATION FAULT when being
     executed through net.box. It happens due to column type of the
     result of this function being NULL. This patch adds checks for
     received column type.

diff --git a/src/box/execute.c b/src/box/execute.c
index e450444..cd809f8 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -540,6 +540,8 @@ sql_get_description(struct sqlite3_stmt *stmt, 
struct obuf *out,
           * column_name simply returns them.
           */
          assert(name != NULL);
+        if (type == NULL)
+            type = "UNKNOWN";
          size += mp_sizeof_str(strlen(name));
          size += mp_sizeof_str(strlen(type));
          char *pos = (char *) obuf_alloc(out, size);
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index d077ee8..f229427 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -773,6 +773,47 @@ cn:execute('drop table test')
  cn:close()
  ---
  ...
+--
+-- Some commands, e.g. EXPLAIN, could lead to segfault because it
+-- does not send column data type, but looks like DQL. Netbox had
+-- been trying to decode type for any DQL.
+--
+cn = remote.connect(box.cfg.listen)
+---
+...
+-- Segmentation fault when EXPLAIN executed using net.box.
+_ = cn:execute("EXPLAIN SELECT 1;")
+---
+...
+-- Segmentation fault when PRAGMA TABLE_INFO() executed using
+-- net.box.
+box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
+---
+...
+cn:execute("PRAGMA TABLE_INFO(test);")
+---
+- metadata:
+  - name: cid
+    type: UNKNOWN
+  - name: name
+    type: UNKNOWN
+  - name: type
+    type: UNKNOWN
+  - name: notnull
+    type: UNKNOWN
+  - name: dflt_value
+    type: UNKNOWN
+  - name: pk
+    type: UNKNOWN
+  rows:
+  - [0, 'ID', 'integer', 1, null, 1]
+...
+box.sql.execute('DROP TABLE test')
+---
+...
+cn:close()
+---
+...
  box.schema.user.revoke('guest', 'read,write,execute', 'universe')
  ---
  ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 1470eda..9bcc7ef 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -247,10 +247,27 @@ cn:execute('drop table test')

  cn:close()

+--
+-- Some commands, e.g. EXPLAIN, could lead to segfault because it
+-- does not send column data type, but looks like DQL. Netbox had
+-- been trying to decode type for any DQL.
+--
+cn = remote.connect(box.cfg.listen)
+
+-- Segmentation fault when EXPLAIN executed using net.box.
+_ = cn:execute("EXPLAIN SELECT 1;")
+
+-- Segmentation fault when PRAGMA TABLE_INFO() executed using
+-- net.box.
+box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
+cn:execute("PRAGMA TABLE_INFO(test);")
+box.sql.execute('DROP TABLE test')
+
+cn:close()
+
  box.schema.user.revoke('guest', 'read,write,execute', 'universe')
  box.schema.user.revoke('guest', 'create', 'space')
  space = nil

  -- Cleanup xlog
  box.snapshot()
-


[-- Attachment #2: Type: text/html, Size: 13100 bytes --]

[tarantool-patches] Re: [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT

[Vladislav Shpilevoy]

It was just a question, "why did you decide to do not
send type". It was not necessary to start sending UNKNOWN. I
think, that unknown type should be sent never.

On 22/11/2018 21:08, Imeev Mergen wrote:
> Hi! Thank you for review! Diff between versions and new patch
> below.
> 
> On 11/19/18 8:27 PM, n.pettik wrote:
> 
>>> EXPLAIN envokes SEGMENTATION FAULT when being executed through
>> Typo: invokes.
>>
>>> net.box. It happens due to column type of the result of this
>>> function being NULL.
>> I’ve checked and not only EXPLAIN results, but also EXPLAIN QUERY PLAN,
>> several pragmas (for instance, pragma table_info()) etc. So, initially problem
>> was not only in EXPLAIN statement. Please exposed commit message and
>> add tests on other cases.
>>
>> Also, why did you decide to avoid sending type at all, instead of sending
>> UNKNOWN for example?
> 
> *Diff between versions:*
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 5b747cf..cd809f8 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -529,6 +529,9 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
>           return -1;
> 
>       for (int i = 0; i < column_count; ++i) {
> +        size_t size = mp_sizeof_map(2) +
> +                  mp_sizeof_uint(IPROTO_FIELD_NAME) +
> +                  mp_sizeof_uint(IPROTO_FIELD_TYPE);
>           const char *name = sqlite3_column_name(stmt, i);
>           const char *type = sqlite3_column_datatype(stmt, i);
>           /*
> @@ -537,27 +540,20 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
>            * column_name simply returns them.
>            */
>           assert(name != NULL);
> -        size_t size = mp_sizeof_uint(IPROTO_FIELD_NAME) +
> -                  mp_sizeof_str(strlen(name));
> -        if (type != NULL) {
> -            size += mp_sizeof_map(2) +
> -                mp_sizeof_uint(IPROTO_FIELD_TYPE) +
> -                mp_sizeof_str(strlen(type));
> -        } else {
> -            size += mp_sizeof_map(1);
> -        }
> +        if (type == NULL)
> +            type = "UNKNOWN";
> +        size += mp_sizeof_str(strlen(name));
> +        size += mp_sizeof_str(strlen(type));
>           char *pos = (char *) obuf_alloc(out, size);
>           if (pos == NULL) {
>               diag_set(OutOfMemory, size, "obuf_alloc", "pos");
>               return -1;
>           }
> -        pos = mp_encode_map(pos, 1 + (type != NULL));
> +        pos = mp_encode_map(pos, 2);
>           pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
>           pos = mp_encode_str(pos, name, strlen(name));
> -        if (type != NULL) {
> -            pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
> -            pos = mp_encode_str(pos, type, strlen(type));
> -        }
> +        pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
> +        pos = mp_encode_str(pos, type, strlen(type));
>       }
>       return 0;
>   }
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 342c6a7..c7063d9 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -644,7 +644,8 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
>       lua_createtable(L, count, 0);
>       for (uint32_t i = 0; i < count; ++i) {
>           uint32_t map_size = mp_decode_map(data);
> -        assert(map_size == 1 || map_size == 2);
> +        assert(map_size == 2);
> +        (void) map_size;
>           uint32_t key = mp_decode_uint(data);
>           assert(key == IPROTO_FIELD_NAME);
>           (void) key;
> @@ -653,13 +654,11 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
>           const char *str = mp_decode_str(data, &len);
>           lua_pushlstring(L, str, len);
>           lua_setfield(L, -2, "name");
> -        if (map_size == 2) {
> -            key = mp_decode_uint(data);
> -            assert(key == IPROTO_FIELD_TYPE);
> -            const char *type = mp_decode_str(data, &len);
> -            lua_pushlstring(L, type, len);
> -            lua_setfield(L, -2, "type");
> -        }
> +        key = mp_decode_uint(data);
> +        assert(key == IPROTO_FIELD_TYPE);
> +        const char *type = mp_decode_str(data, &len);
> +        lua_pushlstring(L, type, len);
> +        lua_setfield(L, -2, "type");
>           lua_rawseti(L, -2, i + 1);
>       }
>   }
> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
> index 31e5d2e..f229427 100644
> --- a/test/sql/iproto.result
> +++ b/test/sql/iproto.result
> @@ -774,9 +774,9 @@ cn:close()
>   ---
>   ...
>   --
> --- EXPLAIN could lead to segfault because it does not send column
> --- data type, but looks like DQL. Netbox had been trying to decode
> --- type for any DQL.
> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
> +-- does not send column data type, but looks like DQL. Netbox had
> +-- been trying to decode type for any DQL.
>   --
>   cn = remote.connect(box.cfg.listen)
>   ---
> @@ -785,6 +785,32 @@ cn = remote.connect(box.cfg.listen)
>   _ = cn:execute("EXPLAIN SELECT 1;")
>   ---
>   ...
> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
> +-- net.box.
> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
> +---
> +...
> +cn:execute("PRAGMA TABLE_INFO(test);")
> +---
> +- metadata:
> +  - name: cid
> +    type: UNKNOWN
> +  - name: name
> +    type: UNKNOWN
> +  - name: type
> +    type: UNKNOWN
> +  - name: notnull
> +    type: UNKNOWN
> +  - name: dflt_value
> +    type: UNKNOWN
> +  - name: pk
> +    type: UNKNOWN
> +  rows:
> +  - [0, 'ID', 'integer', 1, null, 1]
> +...
> +box.sql.execute('DROP TABLE test')
> +---
> +...
>   cn:close()
>   ---
>   ...
> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
> index a0f0f15..9bcc7ef 100644
> --- a/test/sql/iproto.test.lua
> +++ b/test/sql/iproto.test.lua
> @@ -248,15 +248,21 @@ cn:execute('drop table test')
>   cn:close()
> 
>   --
> --- EXPLAIN could lead to segfault because it does not send column
> --- data type, but looks like DQL. Netbox had been trying to decode
> --- type for any DQL.
> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
> +-- does not send column data type, but looks like DQL. Netbox had
> +-- been trying to decode type for any DQL.
>   --
>   cn = remote.connect(box.cfg.listen)
> 
>   -- Segmentation fault when EXPLAIN executed using net.box.
>   _ = cn:execute("EXPLAIN SELECT 1;")
> 
> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
> +-- net.box.
> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
> +cn:execute("PRAGMA TABLE_INFO(test);")
> +box.sql.execute('DROP TABLE test')
> +
>   cn:close()
> 
>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> 
> 
> *New version:*
> 
> commit 29dde0365922bfcec0d159b36c6454854adf34f7
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Fri Nov 16 13:34:46 2018 +0300
> 
>      sql: Print column type even if it is NULL
> 
>      Some commands, e.g. EXPLAIN, invokes SEGMENTATION FAULT when being
>      executed through net.box. It happens due to column type of the
>      result of this function being NULL. This patch adds checks for
>      received column type.
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index e450444..cd809f8 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -540,6 +540,8 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
>            * column_name simply returns them.
>            */
>           assert(name != NULL);
> +        if (type == NULL)
> +            type = "UNKNOWN";
>           size += mp_sizeof_str(strlen(name));
>           size += mp_sizeof_str(strlen(type));
>           char *pos = (char *) obuf_alloc(out, size);
> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
> index d077ee8..f229427 100644
> --- a/test/sql/iproto.result
> +++ b/test/sql/iproto.result
> @@ -773,6 +773,47 @@ cn:execute('drop table test')
>   cn:close()
>   ---
>   ...
> +--
> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
> +-- does not send column data type, but looks like DQL. Netbox had
> +-- been trying to decode type for any DQL.
> +--
> +cn = remote.connect(box.cfg.listen)
> +---
> +...
> +-- Segmentation fault when EXPLAIN executed using net.box.
> +_ = cn:execute("EXPLAIN SELECT 1;")
> +---
> +...
> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
> +-- net.box.
> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
> +---
> +...
> +cn:execute("PRAGMA TABLE_INFO(test);")
> +---
> +- metadata:
> +  - name: cid
> +    type: UNKNOWN
> +  - name: name
> +    type: UNKNOWN
> +  - name: type
> +    type: UNKNOWN
> +  - name: notnull
> +    type: UNKNOWN
> +  - name: dflt_value
> +    type: UNKNOWN
> +  - name: pk
> +    type: UNKNOWN
> +  rows:
> +  - [0, 'ID', 'integer', 1, null, 1]
> +...
> +box.sql.execute('DROP TABLE test')
> +---
> +...
> +cn:close()
> +---
> +...
>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>   ---
>   ...
> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
> index 1470eda..9bcc7ef 100644
> --- a/test/sql/iproto.test.lua
> +++ b/test/sql/iproto.test.lua
> @@ -247,10 +247,27 @@ cn:execute('drop table test')
> 
>   cn:close()
> 
> +--
> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
> +-- does not send column data type, but looks like DQL. Netbox had
> +-- been trying to decode type for any DQL.
> +--
> +cn = remote.connect(box.cfg.listen)
> +
> +-- Segmentation fault when EXPLAIN executed using net.box.
> +_ = cn:execute("EXPLAIN SELECT 1;")
> +
> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
> +-- net.box.
> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
> +cn:execute("PRAGMA TABLE_INFO(test);")
> +box.sql.execute('DROP TABLE test')
> +
> +cn:close()
> +
>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>   box.schema.user.revoke('guest', 'create', 'space')
>   space = nil
> 
>   -- Cleanup xlog
>   box.snapshot()
> -
> 

[tarantool-patches] Re[2]: [tarantool-patches] Re: [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT

[Мерген Имеев]

[-- Attachment #1: Type: text/plain, Size: 11493 bytes --]

After discussion we decided that this patch will serve us
part of patch-set that should close newly filed bug:
https://github.com/tarantool/tarantool/issues/3832

The main Idea behind this ticket is that any statement
should return column type for when it returns column name.
And this ticket allow us to find an error without falling
into SEGMENTATION FAULT or returning less metadata.

Четверг, 22 ноября 2018, 21:31 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
>
>It was just a question, "why did you decide to do not
>send type". It was not necessary to start sending UNKNOWN. I
>think, that unknown type should be sent never.
>
>On 22/11/2018 21:08, Imeev Mergen wrote:
>> Hi! Thank you for review! Diff between versions and new patch
>> below.
>> 
>> On 11/19/18 8:27 PM, n.pettik wrote:
>> 
>>>> EXPLAIN envokes SEGMENTATION FAULT when being executed through
>>> Typo: invokes.
>>>
>>>> net.box. It happens due to column type of the result of this
>>>> function being NULL.
>>> I’ve checked and not only EXPLAIN results, but also EXPLAIN QUERY PLAN,
>>> several pragmas (for instance, pragma table_info()) etc. So, initially problem
>>> was not only in EXPLAIN statement. Please exposed commit message and
>>> add tests on other cases.
>>>
>>> Also, why did you decide to avoid sending type at all, instead of sending
>>> UNKNOWN for example?
>> 
>> *Diff between versions:*
>> 
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index 5b747cf..cd809f8 100644
>> --- a/src/box/execute.c
>> +++ b/src/box/execute.c
>> @@ -529,6 +529,9 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
>>           return -1;
>> 
>>       for (int i = 0; i < column_count; ++i) {
>> +        size_t size = mp_sizeof_map(2) +
>> +                  mp_sizeof_uint(IPROTO_FIELD_NAME) +
>> +                  mp_sizeof_uint(IPROTO_FIELD_TYPE);
>>           const char *name = sqlite3_column_name(stmt, i);
>>           const char *type = sqlite3_column_datatype(stmt, i);
>>           /*
>> @@ -537,27 +540,20 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
>>            * column_name simply returns them.
>>            */
>>           assert(name != NULL);
>> -        size_t size = mp_sizeof_uint(IPROTO_FIELD_NAME) +
>> -                  mp_sizeof_str(strlen(name));
>> -        if (type != NULL) {
>> -            size += mp_sizeof_map(2) +
>> -                mp_sizeof_uint(IPROTO_FIELD_TYPE) +
>> -                mp_sizeof_str(strlen(type));
>> -        } else {
>> -            size += mp_sizeof_map(1);
>> -        }
>> +        if (type == NULL)
>> +            type = "UNKNOWN";
>> +        size += mp_sizeof_str(strlen(name));
>> +        size += mp_sizeof_str(strlen(type));
>>           char *pos = (char *) obuf_alloc(out, size);
>>           if (pos == NULL) {
>>               diag_set(OutOfMemory, size, "obuf_alloc", "pos");
>>               return -1;
>>           }
>> -        pos = mp_encode_map(pos, 1 + (type != NULL));
>> +        pos = mp_encode_map(pos, 2);
>>           pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
>>           pos = mp_encode_str(pos, name, strlen(name));
>> -        if (type != NULL) {
>> -            pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
>> -            pos = mp_encode_str(pos, type, strlen(type));
>> -        }
>> +        pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
>> +        pos = mp_encode_str(pos, type, strlen(type));
>>       }
>>       return 0;
>>   }
>> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
>> index 342c6a7..c7063d9 100644
>> --- a/src/box/lua/net_box.c
>> +++ b/src/box/lua/net_box.c
>> @@ -644,7 +644,8 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
>>       lua_createtable(L, count, 0);
>>       for (uint32_t i = 0; i < count; ++i) {
>>           uint32_t map_size = mp_decode_map(data);
>> -        assert(map_size == 1 || map_size == 2);
>> +        assert(map_size == 2);
>> +        (void) map_size;
>>           uint32_t key = mp_decode_uint(data);
>>           assert(key == IPROTO_FIELD_NAME);
>>           (void) key;
>> @@ -653,13 +654,11 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
>>           const char *str = mp_decode_str(data, &len);
>>           lua_pushlstring(L, str, len);
>>           lua_setfield(L, -2, "name");
>> -        if (map_size == 2) {
>> -            key = mp_decode_uint(data);
>> -            assert(key == IPROTO_FIELD_TYPE);
>> -            const char *type = mp_decode_str(data, &len);
>> -            lua_pushlstring(L, type, len);
>> -            lua_setfield(L, -2, "type");
>> -        }
>> +        key = mp_decode_uint(data);
>> +        assert(key == IPROTO_FIELD_TYPE);
>> +        const char *type = mp_decode_str(data, &len);
>> +        lua_pushlstring(L, type, len);
>> +        lua_setfield(L, -2, "type");
>>           lua_rawseti(L, -2, i + 1);
>>       }
>>   }
>> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
>> index 31e5d2e..f229427 100644
>> --- a/test/sql/iproto.result
>> +++ b/test/sql/iproto.result
>> @@ -774,9 +774,9 @@ cn:close()
>>   ---
>>   ...
>>   --
>> --- EXPLAIN could lead to segfault because it does not send column
>> --- data type, but looks like DQL. Netbox had been trying to decode
>> --- type for any DQL.
>> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
>> +-- does not send column data type, but looks like DQL. Netbox had
>> +-- been trying to decode type for any DQL.
>>   --
>>   cn = remote.connect(box.cfg.listen)
>>   ---
>> @@ -785,6 +785,32 @@ cn = remote.connect(box.cfg.listen)
>>   _ = cn:execute("EXPLAIN SELECT 1;")
>>   ---
>>   ...
>> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
>> +-- net.box.
>> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
>> +---
>> +...
>> +cn:execute("PRAGMA TABLE_INFO(test);")
>> +---
>> +- metadata:
>> +  - name: cid
>> +    type: UNKNOWN
>> +  - name: name
>> +    type: UNKNOWN
>> +  - name: type
>> +    type: UNKNOWN
>> +  - name: notnull
>> +    type: UNKNOWN
>> +  - name: dflt_value
>> +    type: UNKNOWN
>> +  - name: pk
>> +    type: UNKNOWN
>> +  rows:
>> +  - [0, 'ID', 'integer', 1, null, 1]
>> +...
>> +box.sql.execute('DROP TABLE test')
>> +---
>> +...
>>   cn:close()
>>   ---
>>   ...
>> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
>> index a0f0f15..9bcc7ef 100644
>> --- a/test/sql/iproto.test.lua
>> +++ b/test/sql/iproto.test.lua
>> @@ -248,15 +248,21 @@ cn:execute('drop table test')
>>   cn:close()
>> 
>>   --
>> --- EXPLAIN could lead to segfault because it does not send column
>> --- data type, but looks like DQL. Netbox had been trying to decode
>> --- type for any DQL.
>> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
>> +-- does not send column data type, but looks like DQL. Netbox had
>> +-- been trying to decode type for any DQL.
>>   --
>>   cn = remote.connect(box.cfg.listen)
>> 
>>   -- Segmentation fault when EXPLAIN executed using net.box.
>>   _ = cn:execute("EXPLAIN SELECT 1;")
>> 
>> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
>> +-- net.box.
>> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
>> +cn:execute("PRAGMA TABLE_INFO(test);")
>> +box.sql.execute('DROP TABLE test')
>> +
>>   cn:close()
>> 
>>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>> 
>> 
>> *New version:*
>> 
>> commit 29dde0365922bfcec0d159b36c6454854adf34f7
>> Author: Mergen Imeev < imeevma@gmail.com >
>> Date:   Fri Nov 16 13:34:46 2018 +0300
>> 
>>      sql: Print column type even if it is NULL
>> 
>>      Some commands, e.g. EXPLAIN, invokes SEGMENTATION FAULT when being
>>      executed through net.box. It happens due to column type of the
>>      result of this function being NULL. This patch adds checks for
>>      received column type.
>> 
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index e450444..cd809f8 100644
>> --- a/src/box/execute.c
>> +++ b/src/box/execute.c
>> @@ -540,6 +540,8 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
>>            * column_name simply returns them.
>>            */
>>           assert(name != NULL);
>> +        if (type == NULL)
>> +            type = "UNKNOWN";
>>           size += mp_sizeof_str(strlen(name));
>>           size += mp_sizeof_str(strlen(type));
>>           char *pos = (char *) obuf_alloc(out, size);
>> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
>> index d077ee8..f229427 100644
>> --- a/test/sql/iproto.result
>> +++ b/test/sql/iproto.result
>> @@ -773,6 +773,47 @@ cn:execute('drop table test')
>>   cn:close()
>>   ---
>>   ...
>> +--
>> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
>> +-- does not send column data type, but looks like DQL. Netbox had
>> +-- been trying to decode type for any DQL.
>> +--
>> +cn = remote.connect(box.cfg.listen)
>> +---
>> +...
>> +-- Segmentation fault when EXPLAIN executed using net.box.
>> +_ = cn:execute("EXPLAIN SELECT 1;")
>> +---
>> +...
>> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
>> +-- net.box.
>> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
>> +---
>> +...
>> +cn:execute("PRAGMA TABLE_INFO(test);")
>> +---
>> +- metadata:
>> +  - name: cid
>> +    type: UNKNOWN
>> +  - name: name
>> +    type: UNKNOWN
>> +  - name: type
>> +    type: UNKNOWN
>> +  - name: notnull
>> +    type: UNKNOWN
>> +  - name: dflt_value
>> +    type: UNKNOWN
>> +  - name: pk
>> +    type: UNKNOWN
>> +  rows:
>> +  - [0, 'ID', 'integer', 1, null, 1]
>> +...
>> +box.sql.execute('DROP TABLE test')
>> +---
>> +...
>> +cn:close()
>> +---
>> +...
>>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>>   ---
>>   ...
>> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
>> index 1470eda..9bcc7ef 100644
>> --- a/test/sql/iproto.test.lua
>> +++ b/test/sql/iproto.test.lua
>> @@ -247,10 +247,27 @@ cn:execute('drop table test')
>> 
>>   cn:close()
>> 
>> +--
>> +-- Some commands, e.g. EXPLAIN, could lead to segfault because it
>> +-- does not send column data type, but looks like DQL. Netbox had
>> +-- been trying to decode type for any DQL.
>> +--
>> +cn = remote.connect(box.cfg.listen)
>> +
>> +-- Segmentation fault when EXPLAIN executed using net.box.
>> +_ = cn:execute("EXPLAIN SELECT 1;")
>> +
>> +-- Segmentation fault when PRAGMA TABLE_INFO() executed using
>> +-- net.box.
>> +box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
>> +cn:execute("PRAGMA TABLE_INFO(test);")
>> +box.sql.execute('DROP TABLE test')
>> +
>> +cn:close()
>> +
>>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>>   box.schema.user.revoke('guest', 'create', 'space')
>>   space = nil
>> 
>>   -- Cleanup xlog
>>   box.snapshot()
>> -
>> 


-- <br/>Мерген Имеев

[-- Attachment #2: Type: text/html, Size: 16463 bytes --]

[tarantool-patches] Re: Re[2]: Re: [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT

[Kirill Yukhin]

Hello,
On 22 Nov 23:55, Мерген Имеев wrote:
> After discussion we decided that this patch will serve us
> part of patch-set that should close newly filed bug:
> https://github.com/tarantool/tarantool/issues/3832
But we still need other two patches of the patch set.
Please, do not abandon them.

--
Regards, Kirill Yukhin

[tarantool-patches] Re: [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT

[n.pettik]

That’s why I don’t like any verbal agreements…

My point is if result-set features column names,
then those columns must have type. I fail to come up with
example where resulting set features column names,
but columns don’t have types.
On the other hand, mb it is worth to send type ANY instead of
UNKNOWN, it makes sense to a greater or lesser extent.

> On 22 Nov 2018, at 23:55, Мерген Имеев <imeevma@tarantool.org> wrote:
> 
> After discussion we decided that this patch will serve us
> part of patch-set that should close newly filed bug:
> https://github.com/tarantool/tarantool/issues/3832
> 
> The main Idea behind this ticket is that any statement
> should return column type for when it returns column name.
> And this ticket allow us to find an error without falling
> into SEGMENTATION FAULT or returning less metadata.
> 
> Четверг, 22 ноября 2018, 21:31 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
> It was just a question, "why did you decide to do not
> send type". It was not necessary to start sending UNKNOWN. I
> think, that unknown type should be sent never.

[tarantool-patches] Re: [PATCH 1/3] sql: EXPLAIN through net.box leads to SEGFAULT

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [18/11/28 15:51]:
> That’s why I don’t like any verbal agreements…
> 
> My point is if result-set features column names,
> then those columns must have type. I fail to come up with
> example where resulting set features column names,
> but columns don’t have types.
> On the other hand, mb it is worth to send type ANY instead of
> UNKNOWN, it makes sense to a greater or lesser extent.

It would make sense if there was ANY type in SQL. Until there is
no support for ANY in SQL it's better to convert all results to
use only supported types - integer and string.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] [PATCH 2/3] sql: SELECT from system spaces returns unpacked msgpack.

[Vladislav Shpilevoy]

From: Mergen Imeev <imeevma@gmail.com>

SELECT executed through net.box returns unpacked msgpask. Fixed in
this patch.
---
 src/box/execute.c        | 22 +++++++++++++++-------
 test/sql/iproto.result   |  9 +++++++++
 test/sql/iproto.test.lua |  4 ++++
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/src/box/execute.c b/src/box/execute.c
index 5b747cf7f..2a0302703 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -351,13 +351,21 @@ sql_column_to_messagepack(struct sqlite3_stmt *stmt, int i,
 	}
 	case SQLITE_BLOB: {
 		uint32_t len = sqlite3_column_bytes(stmt, i);
-		size = mp_sizeof_bin(len);
-		char *pos = (char *) region_alloc(region, size);
-		if (pos == NULL)
-			goto oom;
-		const char *s;
-		s = (const char *)sqlite3_column_blob(stmt, i);
-		mp_encode_bin(pos, s, len);
+		const char *s =
+			(const char *)sqlite3_column_blob(stmt, i);
+		if (sql_column_subtype(stmt, i) == SQL_SUBTYPE_MSGPACK) {
+			size = len;
+			char *pos = (char *)region_alloc(region, size);
+			if (pos == NULL)
+				goto oom;
+			memcpy(pos, s, len);
+		} else {
+			size = mp_sizeof_bin(len);
+			char *pos = (char *)region_alloc(region, size);
+			if (pos == NULL)
+				goto oom;
+			mp_encode_bin(pos, s, len);
+		}
 		break;
 	}
 	case SQLITE_NULL: {
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 31e5d2e75..b1313e10e 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -785,6 +785,15 @@ cn = remote.connect(box.cfg.listen)
 _ = cn:execute("EXPLAIN SELECT 1;")
 ---
 ...
+-- SELECT from system spaces returns unpacked msgpack.
+res = cn:execute('select "format" from "_space" limit 1;')
+---
+...
+res.rows
+---
+- - [[{'name': 'space_id', 'type': 'unsigned'}, {'name': 'lsn', 'type': 'unsigned'},
+      {'name': 'tuple', 'type': 'array'}]]
+...
 cn:close()
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index a0f0f15ee..5c90cba52 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -257,6 +257,10 @@ cn = remote.connect(box.cfg.listen)
 -- Segmentation fault when EXPLAIN executed using net.box.
 _ = cn:execute("EXPLAIN SELECT 1;")
 
+-- SELECT from system spaces returns unpacked msgpack.
+res = cn:execute('select "format" from "_space" limit 1;')
+res.rows
+
 cn:close()
 
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
-- 
2.17.2 (Apple Git-113)

[tarantool-patches] Re: [PATCH 2/3] sql: SELECT from system spaces returns unpacked msgpack.

[n.pettik]

Please, don’t state problem in commit message. Instead, it would be
better to indicate fix of the problem. I would call it like:

sql: decode mgspack after SELECT from system spaces

Otherwise it looks like you are introducing new bug :)

Also, your patch affects not only system spaces.
For example:

box.schema.space.create('t’)
box.space.t:format({{name = 'id', type = 'integer'}, {name = 'x', type = 'any'}})
box.space.t:create_index('i1', {parts = {1, 'int'}})
box.space.t:insert({1, {1,2,3}})
box.space.t:insert({2, {a = 3}})

Before your patch:

tarantool> cn:execute('select * from \"t\"')
---
- metadata:
  - name: id
    type: UNKNOWN
  - name: x
    type: UNKNOWN
  rows:
  - [1, !!binary kwECAw==]
  - [2, !!binary gaFhAw==]
...

After:

tarantool> cn:execute('select * from \"t\"')
---
- metadata:
  - name: id
    type: UNKNOWN
  - name: x
    type: UNKNOWN
  rows:
  - [1, [1, 2, 3]]
  - [2, {'a': 3}]
…

I guess more precise to say sort of:

sql: decode ARRAY and MAP types after SELECT

Patch itself is OK, but I would rework test in order to avoid
using system spaces. It is up to you.

[tarantool-patches] Re: [PATCH 2/3] sql: SELECT from system spaces returns unpacked msgpack.

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 6460 bytes --]

Hi! Thank you for review! Diff between versions and new patch
below.

On 11/19/18 8:27 PM, n.pettik wrote:
> Please, don’t state problem in commit message. Instead, it would be
> better to indicate fix of the problem. I would call it like:
>
> sql: decode mgspack after SELECT from system spaces
>
> Otherwise it looks like you are introducing new bug :)
>
> Also, your patch affects not only system spaces.
> For example:
>
> box.schema.space.create('t’)
> box.space.t:format({{name = 'id', type = 'integer'}, {name = 'x', type = 'any'}})
> box.space.t:create_index('i1', {parts = {1, 'int'}})
> box.space.t:insert({1, {1,2,3}})
> box.space.t:insert({2, {a = 3}})
>
> Before your patch:
>
> tarantool> cn:execute('select * from \"t\"')
> ---
> - metadata:
>    - name: id
>      type: UNKNOWN
>    - name: x
>      type: UNKNOWN
>    rows:
>    - [1, !!binary kwECAw==]
>    - [2, !!binary gaFhAw==]
> ...
>
> After:
>
> tarantool> cn:execute('select * from \"t\"')
> ---
> - metadata:
>    - name: id
>      type: UNKNOWN
>    - name: x
>      type: UNKNOWN
>    rows:
>    - [1, [1, 2, 3]]
>    - [2, {'a': 3}]
> …
>
> I guess more precise to say sort of:
>
> sql: decode ARRAY and MAP types after SELECT
>
> Patch itself is OK, but I would rework test in order to avoid
> using system spaces. It is up to you.
>
*Diff between versions:*

diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index b866140..6c50781 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -811,14 +811,37 @@ cn:execute("PRAGMA TABLE_INFO(test);")
  box.sql.execute('DROP TABLE test')
  ---
  ...
--- SELECT from system spaces returns unpacked msgpack.
-res = cn:execute('select "format" from "_space" limit 1;')
+-- SELECT returns unpacked msgpack.
+format = {{name = 'id', type = 'integer'}, {name = 'x', type = 'any'}}
  ---
  ...
-res.rows
+s = box.schema.space.create('test', {format=format})
+---
+...
+i1 = s:create_index('i1', {parts = {1, 'int'}})
+---
+...
+s:insert({1, {1,2,3}})
+---
+- [1, [1, 2, 3]]
+...
+s:insert({2, {a = 3}})
+---
+- [2, {'a': 3}]
+...
+cn:execute('select * from "test"')
+---
+- metadata:
+  - name: id
+    type: UNKNOWN
+  - name: x
+    type: UNKNOWN
+  rows:
+  - [1, [1, 2, 3]]
+  - [2, {'a': 3}]
+...
+s:drop()
  ---
-- - [[{'name': 'space_id', 'type': 'unsigned'}, {'name': 'lsn', 'type': 
'unsigned'},
-      {'name': 'tuple', 'type': 'array'}]]
  ...
  cn:close()
  ---
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 6299814..e12decd 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -263,9 +263,14 @@ box.sql.execute('CREATE TABLE test (id INT PRIMARY 
KEY)')
  cn:execute("PRAGMA TABLE_INFO(test);")
  box.sql.execute('DROP TABLE test')

--- SELECT from system spaces returns unpacked msgpack.
-res = cn:execute('select "format" from "_space" limit 1;')
-res.rows
+-- SELECT returns unpacked msgpack.
+format = {{name = 'id', type = 'integer'}, {name = 'x', type = 'any'}}
+s = box.schema.space.create('test', {format=format})
+i1 = s:create_index('i1', {parts = {1, 'int'}})
+s:insert({1, {1,2,3}})
+s:insert({2, {a = 3}})
+cn:execute('select * from "test"')
+s:drop()

  cn:close()

*New patch:*

commit 8482e0c0bc1fa44faa6abf7b7b8a27b77e91db92
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Nov 16 14:23:39 2018 +0300

     sql: decode ARRAY and MAP types after SELECT

     Before this patch MSGPACK received using SELECT statement through
     net.box was unpacked. Fixed in this patch.

diff --git a/src/box/execute.c b/src/box/execute.c
index cd809f8..fb3e08b 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -351,13 +351,21 @@ sql_column_to_messagepack(struct sqlite3_stmt 
*stmt, int i,
      }
      case SQLITE_BLOB: {
          uint32_t len = sqlite3_column_bytes(stmt, i);
-        size = mp_sizeof_bin(len);
-        char *pos = (char *) region_alloc(region, size);
-        if (pos == NULL)
-            goto oom;
-        const char *s;
-        s = (const char *)sqlite3_column_blob(stmt, i);
-        mp_encode_bin(pos, s, len);
+        const char *s =
+            (const char *)sqlite3_column_blob(stmt, i);
+        if (sql_column_subtype(stmt, i) == SQL_SUBTYPE_MSGPACK) {
+            size = len;
+            char *pos = (char *)region_alloc(region, size);
+            if (pos == NULL)
+                goto oom;
+            memcpy(pos, s, len);
+        } else {
+            size = mp_sizeof_bin(len);
+            char *pos = (char *)region_alloc(region, size);
+            if (pos == NULL)
+                goto oom;
+            mp_encode_bin(pos, s, len);
+        }
          break;
      }
      case SQLITE_NULL: {
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index f229427..6c50781 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -811,6 +811,38 @@ cn:execute("PRAGMA TABLE_INFO(test);")
  box.sql.execute('DROP TABLE test')
  ---
  ...
+-- SELECT returns unpacked msgpack.
+format = {{name = 'id', type = 'integer'}, {name = 'x', type = 'any'}}
+---
+...
+s = box.schema.space.create('test', {format=format})
+---
+...
+i1 = s:create_index('i1', {parts = {1, 'int'}})
+---
+...
+s:insert({1, {1,2,3}})
+---
+- [1, [1, 2, 3]]
+...
+s:insert({2, {a = 3}})
+---
+- [2, {'a': 3}]
+...
+cn:execute('select * from "test"')
+---
+- metadata:
+  - name: id
+    type: UNKNOWN
+  - name: x
+    type: UNKNOWN
+  rows:
+  - [1, [1, 2, 3]]
+  - [2, {'a': 3}]
+...
+s:drop()
+---
+...
  cn:close()
  ---
  ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 9bcc7ef..e12decd 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -263,6 +263,15 @@ box.sql.execute('CREATE TABLE test (id INT PRIMARY 
KEY)')
  cn:execute("PRAGMA TABLE_INFO(test);")
  box.sql.execute('DROP TABLE test')

+-- SELECT returns unpacked msgpack.
+format = {{name = 'id', type = 'integer'}, {name = 'x', type = 'any'}}
+s = box.schema.space.create('test', {format=format})
+i1 = s:create_index('i1', {parts = {1, 'int'}})
+s:insert({1, {1,2,3}})
+s:insert({2, {a = 3}})
+cn:execute('select * from "test"')
+s:drop()
+
  cn:close()

  box.schema.user.revoke('guest', 'read,write,execute', 'universe')


[-- Attachment #2: Type: text/html, Size: 8661 bytes --]

[tarantool-patches] Re: [PATCH 2/3] sql: SELECT from system spaces returns unpacked msgpack.

[n.pettik]

LGTM.

[tarantool-patches] [PATCH 3/3] sql: too many autogenerated ids leads to SEGFAULT

[Vladislav Shpilevoy]

From: Mergen Imeev <imeevma@gmail.com>

This probleam appeared because region was cleaned twice: once in
sqlite3VdbeHalt() and once in sqlite3VdbeDelete() which was
executed during sqlite3_finalize(). Autogenerated ids that were
saved there, were fetched after sqlite3VdbeHalt() and before
sqlite3_finalize(). In this patch region cleaning in
sqlite3VdbeHalt() were removed.

Follow up #2618
Follow up #3199
---
 src/box/sql/vdbe.c       |  8 ++------
 src/box/sql/vdbeaux.c    |  6 ------
 test/sql/iproto.result   | 16 ++++++++++++++++
 test/sql/iproto.test.lua |  7 +++++++
 4 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index b6afe9184..cc340e942 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2911,12 +2911,8 @@ case OP_MakeRecord: {
 	 * memory shouldn't be reused until it is written into WAL.
 	 *
 	 * However, if memory for ephemeral space is allocated
-	 * on region, it will be freed only in vdbeHalt() routine.
-	 * It is the only way to free this region memory,
-	 * since ephemeral spaces don't have nothing in common
-	 * with txn routine and region memory won't be released
-	 * after txn_commit() or txn_rollback() as it happens
-	 * with ordinary spaces.
+	 * on region, it will be freed only in sqlite3_finalize()
+	 * routine.
 	 */
 	if (bIsEphemeral) {
 		rc = sqlite3VdbeMemClearAndResize(pOut, nByte);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 615a0f064..f2faad862 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2498,12 +2498,6 @@ sqlite3VdbeHalt(Vdbe * p)
 		p->rc = SQLITE_NOMEM_BKPT;
 	}
 
-	/* Release all region memory which was allocated
-	 * to hold tuples to be inserted into ephemeral spaces.
-	 */
-	if (!box_txn())
-		fiber_gc();
-
 	assert(db->nVdbeActive > 0 || box_txn() ||
 	       p->anonymous_savepoint == NULL);
 	return (p->rc == SQLITE_BUSY ? SQLITE_BUSY : SQLITE_OK);
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index b1313e10e..b2a1e42cc 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -794,6 +794,22 @@ res.rows
 - - [[{'name': 'space_id', 'type': 'unsigned'}, {'name': 'lsn', 'type': 'unsigned'},
       {'name': 'tuple', 'type': 'array'}]]
 ...
+-- Too many autogenerated ids leads to SEGFAULT.
+cn = remote.connect(box.cfg.listen)
+---
+...
+box.sql.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT)')
+---
+...
+for i = 0, 1000 do cn:execute("INSERT INTO t1 VALUES (null)") end
+---
+...
+_ = cn:execute("INSERT INTO t1 SELECT NULL from t1")
+---
+...
+box.sql.execute('DROP TABLE t1')
+---
+...
 cn:close()
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 5c90cba52..da52d631e 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -261,6 +261,13 @@ _ = cn:execute("EXPLAIN SELECT 1;")
 res = cn:execute('select "format" from "_space" limit 1;')
 res.rows
 
+-- Too many autogenerated ids leads to SEGFAULT.
+cn = remote.connect(box.cfg.listen)
+box.sql.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT)')
+for i = 0, 1000 do cn:execute("INSERT INTO t1 VALUES (null)") end
+_ = cn:execute("INSERT INTO t1 SELECT NULL from t1")
+box.sql.execute('DROP TABLE t1')
+
 cn:close()
 
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
-- 
2.17.2 (Apple Git-113)

[tarantool-patches] Re: [PATCH 3/3] sql: too many autogenerated ids leads to SEGFAULT

[n.pettik]

> This probleam appeared because region was cleaned twice: once in

Typo: problem

> sqlite3VdbeHalt() and once in sqlite3VdbeDelete() which was
> executed during sqlite3_finalize(). Autogenerated ids that were
> saved there, were fetched after sqlite3VdbeHalt() and before
> sqlite3_finalize(). In this patch region cleaning in
> sqlite3VdbeHalt() were removed.

Typo: was (or better - has been removed).

Again, IMHO I would rephrase commit subject:

sql: remove region_truncate() from sqlite3VdbeHalt()

And explain in commit message why it was removed.

Patch itself is OK.

[tarantool-patches] Re: [PATCH 3/3] sql: too many autogenerated ids leads to SEGFAULT

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 4172 bytes --]

Hi! Thank you for review! New patch below.

On 11/19/18 8:27 PM, n.pettik wrote:
>> This probleam appeared because region was cleaned twice: once in
> Typo: problem
>
>> sqlite3VdbeHalt() and once in sqlite3VdbeDelete() which was
>> executed during sqlite3_finalize(). Autogenerated ids that were
>> saved there, were fetched after sqlite3VdbeHalt() and before
>> sqlite3_finalize(). In this patch region cleaning in
>> sqlite3VdbeHalt() were removed.
> Typo: was (or better - has been removed).
>
> Again, IMHO I would rephrase commit subject:
>
> sql: remove region_truncate() from sqlite3VdbeHalt()
>
> And explain in commit message why it was removed.
>
> Patch itself is OK.

Fixed commit-message.

*New patch:**
*

commit dd5b3aaa63b9fe2312a6fb30f3ba87bf8329b222
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Nov 17 13:05:55 2018 +0300

     sql: remove fiber_gc() from sqlite3VdbeHalt()

     Too many autogenerated ids leads to SEGFAULT. This problem
     appeared because region was cleaned twice: once in
     sqlite3VdbeHalt() and once in sqlite3VdbeDelete() which was
     executed during sqlite3_finalize(). Autogenerated ids that were
     saved there, were fetched after sqlite3VdbeHalt() and before
     sqlite3_finalize(). In this patch region cleaning in
     sqlite3VdbeHalt() has been removed.

     Follow up #2618
     Follow up #3199

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index b6afe91..cc340e9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2911,12 +2911,8 @@ case OP_MakeRecord: {
       * memory shouldn't be reused until it is written into WAL.
       *
       * However, if memory for ephemeral space is allocated
-     * on region, it will be freed only in vdbeHalt() routine.
-     * It is the only way to free this region memory,
-     * since ephemeral spaces don't have nothing in common
-     * with txn routine and region memory won't be released
-     * after txn_commit() or txn_rollback() as it happens
-     * with ordinary spaces.
+     * on region, it will be freed only in sqlite3_finalize()
+     * routine.
       */
      if (bIsEphemeral) {
          rc = sqlite3VdbeMemClearAndResize(pOut, nByte);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 615a0f0..f2faad8 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2498,12 +2498,6 @@ sqlite3VdbeHalt(Vdbe * p)
          p->rc = SQLITE_NOMEM_BKPT;
      }

-    /* Release all region memory which was allocated
-     * to hold tuples to be inserted into ephemeral spaces.
-     */
-    if (!box_txn())
-        fiber_gc();
-
      assert(db->nVdbeActive > 0 || box_txn() ||
             p->anonymous_savepoint == NULL);
      return (p->rc == SQLITE_BUSY ? SQLITE_BUSY : SQLITE_OK);
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 6c50781..000b359 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -843,6 +843,22 @@ cn:execute('select * from "test"')
  s:drop()
  ---
  ...
+-- Too many autogenerated ids leads to SEGFAULT.
+cn = remote.connect(box.cfg.listen)
+---
+...
+box.sql.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT)')
+---
+...
+for i = 0, 1000 do cn:execute("INSERT INTO t1 VALUES (null)") end
+---
+...
+_ = cn:execute("INSERT INTO t1 SELECT NULL from t1")
+---
+...
+box.sql.execute('DROP TABLE t1')
+---
+...
  cn:close()
  ---
  ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index e12decd..2501393 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -272,6 +272,13 @@ s:insert({2, {a = 3}})
  cn:execute('select * from "test"')
  s:drop()

+-- Too many autogenerated ids leads to SEGFAULT.
+cn = remote.connect(box.cfg.listen)
+box.sql.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT)')
+for i = 0, 1000 do cn:execute("INSERT INTO t1 VALUES (null)") end
+_ = cn:execute("INSERT INTO t1 SELECT NULL from t1")
+box.sql.execute('DROP TABLE t1')
+
  cn:close()

  box.schema.user.revoke('guest', 'read,write,execute', 'universe')


[-- Attachment #2: Type: text/html, Size: 5699 bytes --]

[tarantool-patches] Re: [PATCH 3/3] sql: too many autogenerated ids leads to SEGFAULT

[n.pettik]

LGTM.

[tarantool-patches] Re: [PATCH 0/3] A set of small SQL fixes

[Kirill Yukhin]

Hello,
On 19 Nov 16:47, Vladislav Shpilevoy wrote:
> A set of small patches fixing some SQL assertions or incorrect behaviour,
> which appeared during working on 3505 - box.sql.execute() removal.
> 
> Branch: http://github.com/tarantool/tarantool/tree/imeevma/small-patches
> 
> Mergen Imeev (3):
>   sql: EXPLAIN through net.box leads to SEGFAULT
>   sql: SELECT from system spaces returns unpacked msgpack.
>   sql: too many autogenerated ids leads to SEGFAULT
I've cherry-picked [2/3] and [2/3] patches to 2.1 branch.
1/3 won't be checked in. So, consider this thread closed.

--
Regards, Kirill Yukhin