[Tarantool-patches] [PATCH 0/2] fix assertion fault in SELECT * FROM "_vfunc"

[Tarantool-patches] [PATCH 0/2] fix assertion fault in SELECT * FROM "_vfunc"

[Chris Sosnin]

First patch allows performing schema upgrades within one release.

Second patch updates _vfunc format in 2.3.1.1 (the last number is not
visible to users).

branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4666-select-from-vfunc
issue #1:https://github.com/tarantool/tarantool/issues/4804
issue #2:https://github.com/tarantool/tarantool/issues/4666

Chris Sosnin (2):
  box: allow schema upgrades within a release
  sql: fix assertion fault in SELECT * FROM "_vfunc"

 src/box/bootstrap.snap                        | Bin 5976 -> 5970 bytes
 src/box/lua/upgrade.lua                       |  17 ++++++++++++++---
 test/box-py/bootstrap.result                  |  11 +++++++++--
 test/sql/engine.cfg                           |   1 +
 test/sql/gh-4666-sql-select-from-vfunc.result |   7 +++++++
 .../gh-4666-sql-select-from-vfunc.test.lua    |   4 ++++
 6 files changed, 35 insertions(+), 5 deletions(-)
 create mode 100644 test/sql/gh-4666-sql-select-from-vfunc.result
 create mode 100644 test/sql/gh-4666-sql-select-from-vfunc.test.lua

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 1/2] box: allow schema upgrades within a release

[Chris Sosnin]

To avoid cases when a user has an incorrectly upgraded schema, we
introduce new versioning, which can be used to perform upgrades
within a single release.

Closes #4804
Needed for #4666
---
 src/box/lua/upgrade.lua | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 075cc236e..92c3b460e 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -27,12 +27,14 @@ local mkversion = {}
 mkversion.__index = mkversion
 setmetatable(mkversion, {__call = function(c, ...) return c.new(...) end})
 
-function mkversion.new(major, minor, patch)
+function mkversion.new(major, minor, patch, build)
     local self = setmetatable({}, mkversion)
     self.major = major
     self.minor = minor
     self.patch = patch
+    self.build = build or 0
     self.id = bit.bor(bit.lshift(bit.bor(bit.lshift(major, 8), minor), 8), patch)
+    self.id = bit.bor(bit.lshift(self.id, 8), self.build)
     return self
 end
 
@@ -982,8 +984,9 @@ local function get_version()
     local major = version[2]
     local minor = version[3]
     local patch = version[4] or 0
+    local build = version[5] or 0
 
-    return mkversion(major, minor, patch)
+    return mkversion(major, minor, patch, build)
 end
 
 local function upgrade(options)
@@ -1025,7 +1028,8 @@ local function upgrade(options)
         box.space._schema:replace({'version',
                                    handler.version.major,
                                    handler.version.minor,
-                                   handler.version.patch})
+                                   handler.version.patch,
+                                   handler.version.build})
         ::continue::
     end
 end
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 1/2] box: allow schema upgrades within a release

[Vladislav Shpilevoy]

Thanks for the patch!

On 19/03/2020 10:14, Chris Sosnin wrote:
> To avoid cases when a user has an incorrectly upgraded schema, we
> introduce new versioning, which can be used to perform upgrades
> within a single release.
> 
> Closes #4804
> Needed for #4666
> ---
>  src/box/lua/upgrade.lua | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index 075cc236e..92c3b460e 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -27,12 +27,14 @@ local mkversion = {}
>  mkversion.__index = mkversion
>  setmetatable(mkversion, {__call = function(c, ...) return c.new(...) end})
>  
> -function mkversion.new(major, minor, patch)
> +function mkversion.new(major, minor, patch, build)
>      local self = setmetatable({}, mkversion)
>      self.major = major
>      self.minor = minor
>      self.patch = patch
> +    self.build = build or 0
>      self.id = bit.bor(bit.lshift(bit.bor(bit.lshift(major, 8), minor), 8), patch)
> +    self.id = bit.bor(bit.lshift(self.id, 8), self.build)
>      return self
>  end

Since we don't update mkversion.__tostring(), user during upgrade
will see his logs as

    > set schema version to 2.3.1
    > set schema version to 2.3.1
    > set schema version to 2.3.1
    ... depending on how many schema versions we have within
    release.

Perhaps, it is worth adding the build version to __tostring()
method. So as not to confuse the user, and simplify debug so
as user could tell between which builds the upgrade failed.

It does not affect version visibility - box.info.version() is
reported differently.

Re: [Tarantool-patches] [PATCH 1/2] box: allow schema upgrades within a release

[Chris Sosnin]

Hi! Thank you for the review!

> On 22 Mar 2020, at 22:43, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Thanks for the patch!
> 
> On 19/03/2020 10:14, Chris Sosnin wrote:
>> To avoid cases when a user has an incorrectly upgraded schema, we
>> introduce new versioning, which can be used to perform upgrades
>> within a single release.
>> 
>> Closes #4804
>> Needed for #4666
>> ---
>> src/box/lua/upgrade.lua | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>> index 075cc236e..92c3b460e 100644
>> --- a/src/box/lua/upgrade.lua
>> +++ b/src/box/lua/upgrade.lua
>> @@ -27,12 +27,14 @@ local mkversion = {}
>> mkversion.__index = mkversion
>> setmetatable(mkversion, {__call = function(c, ...) return c.new(...) end})
>> 
>> -function mkversion.new(major, minor, patch)
>> +function mkversion.new(major, minor, patch, build)
>>     local self = setmetatable({}, mkversion)
>>     self.major = major
>>     self.minor = minor
>>     self.patch = patch
>> +    self.build = build or 0
>>     self.id = bit.bor(bit.lshift(bit.bor(bit.lshift(major, 8), minor), 8), patch)
>> +    self.id = bit.bor(bit.lshift(self.id, 8), self.build)
>>     return self
>> end
> 
> Since we don't update mkversion.__tostring(), user during upgrade
> will see his logs as
> 
>> set schema version to 2.3.1
>> set schema version to 2.3.1
>> set schema version to 2.3.1
>    ... depending on how many schema versions we have within
>    release.
> 
> Perhaps, it is worth adding the build version to __tostring()
> method. So as not to confuse the user, and simplify debug so
> as user could tell between which builds the upgrade failed.
> 
> It does not affect version visibility - box.info.version() is
> reported differently.

Thank you for clarification, I didn’t touch it on purpose and I was wrong.
Fixed.

[Tarantool-patches] [PATCH 2/2] sql: fix assertion fault in SELECT * FROM "_vfunc"

[Chris Sosnin]

It fails because it was forgotten to update _vfunc format along
with _func in 2.2.1. This leads to bad allocation of vdbe cursor
and setting wrong memory to 0.

Closes #4666
---
 src/box/bootstrap.snap                        | Bin 5976 -> 5970 bytes
 src/box/lua/upgrade.lua                       |   7 +++++++
 test/box-py/bootstrap.result                  |  11 +++++++++--
 test/sql/engine.cfg                           |   1 +
 test/sql/gh-4666-sql-select-from-vfunc.result |   7 +++++++
 .../gh-4666-sql-select-from-vfunc.test.lua    |   4 ++++
 6 files changed, 28 insertions(+), 2 deletions(-)
 create mode 100644 test/sql/gh-4666-sql-select-from-vfunc.result
 create mode 100644 test/sql/gh-4666-sql-select-from-vfunc.test.lua

diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 8bd4f7ce24216a8bcced6aa97c20c83eb7a02c77..ad952a8cba39aeb0e3c2d1ef23b5786de5f83034 100644
GIT binary patch
delta 5462
<snipped>

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 92c3b460e..2fea6f943 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -974,6 +974,12 @@ local function upgrade_to_2_3_1()
     create_session_settings_space()
 end
 
+local function upgrade_to_2_3_1_1()
+    local _func = box.space[box.schema.FUNC_ID]
+    local _vfunc = box.space[box.schema.VFUNC_ID]
+    _vfunc:format(_func:format())
+end
+
 --------------------------------------------------------------------------------
 
 local function get_version()
@@ -1011,6 +1017,7 @@ local function upgrade(options)
         {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
         {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
         {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
+        {version = mkversion(2, 3, 1, 1), func = upgrade_to_2_3_1_1, auto = true},
     }
 
     for _, handler in ipairs(handlers) do
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index 0876e77a6..6fb10d88c 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -4,7 +4,7 @@ box.internal.bootstrap()
 box.space._schema:select{}
 ---
 - - ['max_id', 511]
-  - ['version', 2, 3, 1]
+  - ['version', 2, 3, 1, 1]
 ...
 box.space._cluster:select{}
 ---
@@ -63,7 +63,14 @@ box.space._space:select{}
         'type': 'string'}, {'name': 'last_altered', 'type': 'string'}]]
   - [297, 1, '_vfunc', 'sysview', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
         'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'setuid',
-        'type': 'unsigned'}]]
+        'type': 'unsigned'}, {'name': 'language', 'type': 'string'}, {'name': 'body',
+        'type': 'string'}, {'name': 'routine_type', 'type': 'string'}, {'name': 'param_list',
+        'type': 'array'}, {'name': 'returns', 'type': 'string'}, {'name': 'aggregate',
+        'type': 'string'}, {'name': 'sql_data_access', 'type': 'string'}, {'name': 'is_deterministic',
+        'type': 'boolean'}, {'name': 'is_sandboxed', 'type': 'boolean'}, {'name': 'is_null_call',
+        'type': 'boolean'}, {'name': 'exports', 'type': 'array'}, {'name': 'opts',
+        'type': 'map'}, {'name': 'comment', 'type': 'string'}, {'name': 'created',
+        'type': 'string'}, {'name': 'last_altered', 'type': 'string'}]]
   - [304, 1, '_user', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'owner',
         'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'type',
         'type': 'string'}, {'name': 'auth', 'type': 'map'}]]
diff --git a/test/sql/engine.cfg b/test/sql/engine.cfg
index 1e9f08c6a..22ea90043 100644
--- a/test/sql/engine.cfg
+++ b/test/sql/engine.cfg
@@ -2,6 +2,7 @@
     "vinyl-opts.test.lua" : {
         "vinyl": {"engine": "vinyl"}
     },
+    "gh-4666-sql-select-from-vfunc.test.lua": { },
     "bind.test.lua": {
         "remote": {"remote": "true"},
         "local": {"remote": "false"}
diff --git a/test/sql/gh-4666-sql-select-from-vfunc.result b/test/sql/gh-4666-sql-select-from-vfunc.result
new file mode 100644
index 000000000..5e5ea8aaf
--- /dev/null
+++ b/test/sql/gh-4666-sql-select-from-vfunc.result
@@ -0,0 +1,7 @@
+-- test-run result file version 2
+--
+-- Make sure assertion does not fail.
+--
+_ = box.execute([[select * from "_vfunc";]])
+ | ---
+ | ...
diff --git a/test/sql/gh-4666-sql-select-from-vfunc.test.lua b/test/sql/gh-4666-sql-select-from-vfunc.test.lua
new file mode 100644
index 000000000..3ac0aadc5
--- /dev/null
+++ b/test/sql/gh-4666-sql-select-from-vfunc.test.lua
@@ -0,0 +1,4 @@
+--
+-- Make sure assertion does not fail.
+--
+_ = box.execute([[select * from "_vfunc";]])
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 2/2] sql: fix assertion fault in SELECT * FROM "_vfunc"

[Vladislav Shpilevoy]

> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index 92c3b460e..2fea6f943 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -974,6 +974,12 @@ local function upgrade_to_2_3_1()
>      create_session_settings_space()
>  end
>  
> +local function upgrade_to_2_3_1_1()
> +    local _func = box.space[box.schema.FUNC_ID]
> +    local _vfunc = box.space[box.schema.VFUNC_ID]
> +    _vfunc:format(_func:format())

It is worth adding a comment why such a trivial thing should be done.

Re: [Tarantool-patches] [PATCH 2/2] sql: fix assertion fault in SELECT * FROM "_vfunc"

[Chris Sosnin]

Hi! Thank you for the review!

> On 22 Mar 2020, at 22:42, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>> index 92c3b460e..2fea6f943 100644
>> --- a/src/box/lua/upgrade.lua
>> +++ b/src/box/lua/upgrade.lua
>> @@ -974,6 +974,12 @@ local function upgrade_to_2_3_1()
>>     create_session_settings_space()
>> end
>> 
>> +local function upgrade_to_2_3_1_1()
>> +    local _func = box.space[box.schema.FUNC_ID]
>> +    local _vfunc = box.space[box.schema.VFUNC_ID]
>> +    _vfunc:format(_func:format())
> 
> It is worth adding a comment why such a trivial thing should be done.

I added the following one:

+local function upgrade_to_2_3_1_1()
+    -- Formats must be equal in order to avoid crashes in SQL
+    -- caused by incorrectly allocated cursor.
+    local _func = box.space[box.schema.FUNC_ID]
+    local _vfunc = box.space[box.schema.VFUNC_ID]
+    _vfunc:format(_func:format())
+end
+

Re: [Tarantool-patches] [PATCH 0/2] fix assertion fault in SELECT * FROM "_vfunc"

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

LGTM.

On 19/03/2020 10:14, Chris Sosnin wrote:
> First patch allows performing schema upgrades within one release.
> 
> Second patch updates _vfunc format in 2.3.1.1 (the last number is not
> visible to users).
> 
> branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4666-select-from-vfunc
> issue #1:https://github.com/tarantool/tarantool/issues/4804
> issue #2:https://github.com/tarantool/tarantool/issues/4666
> 
> Chris Sosnin (2):
>   box: allow schema upgrades within a release
>   sql: fix assertion fault in SELECT * FROM "_vfunc"
> 
>  src/box/bootstrap.snap                        | Bin 5976 -> 5970 bytes
>  src/box/lua/upgrade.lua                       |  17 ++++++++++++++---
>  test/box-py/bootstrap.result                  |  11 +++++++++--
>  test/sql/engine.cfg                           |   1 +
>  test/sql/gh-4666-sql-select-from-vfunc.result |   7 +++++++
>  .../gh-4666-sql-select-from-vfunc.test.lua    |   4 ++++
>  6 files changed, 35 insertions(+), 5 deletions(-)
>  create mode 100644 test/sql/gh-4666-sql-select-from-vfunc.result
>  create mode 100644 test/sql/gh-4666-sql-select-from-vfunc.test.lua
> 

[Tarantool-patches] [PATCH 0/2] fix assertion fault in SELECT * FROM "_vfunc"

[Chris Sosnin]

First patch allows performing schema upgrades within one release

Second patch updates _vfunc format in 2.3.1.1.

branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4666-select-from-vfunc
issue#1: https://github.com/tarantool/tarantool/issues/4804
issue#2: https://github.com/tarantool/tarantool/issues/4666

This branch is already reviewed by Vlad, I rebased it and regenerated
snap file just in case.

Chris Sosnin (2):
  box: allow schema upgrades within a release
  sql: fix assertion fault in SELECT * FROM "_vfunc"

 src/box/bootstrap.snap                        | Bin 5976 -> 5968 bytes
 src/box/lua/upgrade.lua                       |  25 +++++++++++++++---
 test/box-py/bootstrap.result                  |  11 ++++++--
 test/sql/engine.cfg                           |   1 +
 test/sql/gh-4666-sql-select-from-vfunc.result |   7 +++++
 .../gh-4666-sql-select-from-vfunc.test.lua    |   4 +++
 6 files changed, 42 insertions(+), 6 deletions(-)
 create mode 100644 test/sql/gh-4666-sql-select-from-vfunc.result
 create mode 100644 test/sql/gh-4666-sql-select-from-vfunc.test.lua

-- 
2.21.1 (Apple Git-122.3)