Re: [Tarantool-patches] [PATCH 1/1] box: introduce space:alter()

["Serge Petrenko" <sergepetrenko@tarantool.org>]

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


Hi! Thanks for the patch!
  
>Суббота, 22 августа 2020, 1:46 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
>There was no way to change certain space parameters without its
>recreation or manual update of internal system space _space. Even
>if some of them were legal to update: field_count, owner, flag of
>being temporary, is_sync flag.
>
>The patch introduces function space:alter(), which accepts a
>subset of parameters from box.schema.space.create which are
>mutable, and 'name' parameter. There is a method space:rename(),
>but still the parameter is added to space:alter() too, to be
>consistent with index:alter(), which also accepts a new name.
>
>Closes #5155
>
>@TarantoolBot document
>Title: New function space:alter(options)
>
>Space objects in Lua (stored in `box.space` table) now have a new
>method: `space:alter(options)`.
>
>The method accepts a table with parameters `field_count`, `user`,
>`format`, `temporary`, `is_sync`, and `name`. All parameters have
>the same meaning as in `box.schema.space.create(name, options)`.
>
>Note, `name` parameter in `box.schema.space.create` is separated
>from `options` table. It is not so in `space:alter(options)` -
>here all parameters are specified in the `options` table.
>
>The function does not return anything in case of success, and
>throws an error when fails.
>
>From 'Synchronous replication' page, from 'Limitations and known
>problems' it is necessary to delete the note about "no way to
>enable synchronous replication for existing spaces". Instead it
>is necessary to say, that it can be enabled using
>`space:alter({is_sync = true})`. And can be disabled by setting
>`is_sync = false`.
>https://www.tarantool.io/en/doc/2.5/book/replication/repl_sync/#limitations-and-known-problems
>
>The function will appear in >= 2.5.2.
>---
>Branch:  http://github.com/tarantool/tarantool/tree/gerold103/gh-5155-space-alter
>Issue:  https://github.com/tarantool/tarantool/issues/5155
>
>@ChangeLog
>* New function `space:alter(options)` to change some space settings without recreation nor touching `_space` space.
>
> src/box/lua/schema.lua | 63 +++++++
> test/box/alter.result | 378 ++++++++++++++++++++++++++++++++++++++++
> test/box/alter.test.lua | 142 +++++++++++++++
> 3 files changed, 583 insertions(+)
>
>diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
>index 3c3fa3ef8..7fe1ea7a2 100644
>--- a/src/box/lua/schema.lua
>+++ b/src/box/lua/schema.lua
>@@ -539,6 +539,64 @@ box.schema.space.rename = function(space_id, space_name)
>     _space:update(space_id, {{"=", 3, space_name}})
> end
> 
>+local alter_space_template = {
>+ field_count = 'number',
>+ user = 'string, number',
>+ format = 'table',
>+ temporary = 'boolean',
>+ is_sync = 'boolean',
>+ name = 'string',
>+}
>+
>+box.schema.space.alter = function(space_id, options)
>+ local space = box.space[space_id]
>+ if not space then
>+ box.error(box.error.NO_SUCH_SPACE, '#'..tostring(space_id))
>+ end
>+ check_param_table(options, alter_space_template)
>+
>+ local _space = box.space._space
>+ local tuple = _space:get({space.id})
>+ assert(tuple ~= nil)
>+
>+ local owner
>+ if options.user then
>+ owner = user_or_role_resolve(options.user)
>+ if not owner then
>+ box.error(box.error.NO_SUCH_USER, options.user)
>+ end
>+ else
>+ owner = tuple.owner
>+ end
>+
>+ local name = options.name or tuple.name
>+ local field_count = options.field_count or tuple.field_count
>+ local flags = tuple.flags
>+
>+ if options.temporary ~= nil then
>+ flags.temporary = options.temporary
>+ end
>+
>+ if options.is_sync ~= nil then
>+ flags.is_sync = options.is_sync
>+ end
>+
>+ local format
>+ if options.format ~= nil then
>+ format = update_format(options.format)
>+ else
>+ format = tuple.format
>+ end
>+
>+ tuple = tuple:totable()
>+ tuple[2] = owner
>+ tuple[3] = name
>+ tuple[5] = field_count
>+ tuple[6] = flags
>+ tuple[7] = format
>+ _space:replace(tuple)
>+end
>+
> box.schema.index = {}
> 
> local function update_index_parts_1_6_0(parts)
>@@ -1718,6 +1776,11 @@ space_mt.rename = function(space, name)
>     check_space_exists(space)
>     return box.schema.space.rename(space.id, name)
> end
>+space_mt.alter = function(space, options)
>+ check_space_arg(space, 'alter')
>+ check_space_exists(space)
>+ return box.schema.space.alter(space.id, options)
>+end
> space_mt.create_index = function(space, name, options)
>     check_space_arg(space, 'create_index')
>     check_space_exists(space)
>diff --git a/test/box/alter.result b/test/box/alter.result
>index f150faac1..237c2d8aa 100644
>--- a/test/box/alter.result
>+++ b/test/box/alter.result
>@@ -1181,3 +1181,381 @@ s:drop()
> box.internal.collation.drop('test')
> ---
> ...
>+--
>+-- gh-5155: space:alter(), added in order to make it possible to turn on/off
>+-- is_sync, but it is handy for other options too.
>+--
>+s = box.schema.create_space('test')
>+---
>+...
>+pk = s:create_index('pk')
>+---
>+...
>+-- Bad usage.
>+s:alter({unknown = true})
>+---
>+- error: Illegal parameters, unexpected option 'unknown'
>+...
>+ok, err = pcall(s.alter, {})
>+---
>+...
>+assert(err:match("Use space:alter") ~= nil)
>+---
>+- true
>+...
>+-- Alter field_count.
>+s:replace{1}
>+---
>+- [1]
>+...
>+-- Can't update on non-empty space.
>+s:alter({field_count = 2})
>+---
>+- error: Tuple field count 1 does not match space field count 2
>+...
>+s:delete{1}
>+---
>+- [1]
>+...
>+-- Can update on empty space.
>+s:alter({field_count = 2})
>+---
>+...
>+s:replace{1}
>+---
>+- error: Tuple field count 1 does not match space field count 2
>+...
>+s:replace{1, 1}
>+---
>+- [1, 1]
>+...
>+-- When not specified or nil - ignored.
>+s:alter({field_count = nil})
>+---
>+...
>+s:replace{2}
>+---
>+- error: Tuple field count 1 does not match space field count 2
>+...
>+s:replace{2, 2}
>+---
>+- [2, 2]
>+...
>+-- Set to 0 drops the restriction.
>+s:alter({field_count = 0})
>+---
>+...
>+s:truncate()
>+---
>+...
>+s:replace{1}
>+---
>+- [1]
>+...
>+s:delete{1}
>+---
>+- [1]
>+...
>+-- Invalid values.
>+s:alter({field_count = box.NULL})
>+---
>+- error: Illegal parameters, options parameter 'field_count' should be of type number
>+...
>+s:alter({field_count = 'string'})
>+---
>+- error: Illegal parameters, options parameter 'field_count' should be of type number
>+...
>+-- Alter owner.
>+owner1 = box.space._space:get{s.id}.owner
>+---
>+...
>+box.schema.user.create('test')
>+---
>+...
>+-- When not specified or nil - ignored.
>+s:alter({user = nil})
>+---
>+...
>+owner2 = box.space._space:get{s.id}.owner
>+---
>+...
>+assert(owner2 == owner1)
>+---
>+- true
>+...
>+s:alter({user = 'test'})
>+---
>+...
>+owner2 = box.space._space:get{s.id}.owner
>+---
>+...
>+assert(owner2 ~= owner1)
>+---
>+- true
>+...
>+s:alter({user = owner1})
>+---
>+...
>+owner2 = box.space._space:get{s.id}.owner
>+---
>+...
>+assert(owner2 == owner1)
>+---
>+- true
>+...
>+box.schema.user.drop('test')
>+---
>+...
>+-- Invalid values.
>+s:alter({user = box.NULL})
>+---
>+- error: 'Illegal parameters, options parameter ''user'' should be one of types: string,
>+ number'
>+...
>+s:alter({user = true})
>+---
>+- error: 'Illegal parameters, options parameter ''user'' should be one of types: string,
>+ number'
>+...
>+s:alter({user = 'not_existing'})
>+---
>+- error: User 'not_existing' is not found
>+...
>+-- Alter format.
>+format = {{name = 'field1', type = 'unsigned'}}
>+---
>+...
>+s:alter({format = format})
>+---
>+...
>+s:format()
>+---
>+- [{'name': 'field1', 'type': 'unsigned'}]
>+...
>+-- When not specified or nil - ignored.
>+s:alter({format = nil})
>+---
>+...
>+s:format()
>+---
>+- [{'name': 'field1', 'type': 'unsigned'}]
>+...
>+t = s:replace{1}
>+---
>+...
>+assert(t.field1 == 1)
>+---
>+- true
>+...
>+s:alter({format = {}})
>+---
>+...
>+assert(t.field1 == nil)
>+---
>+- true
>+...
>+s:delete{1}
>+---
>+- [1]
>+...
>+-- Invalid values.
>+s:alter({format = box.NULL})
>+---
>+- error: Illegal parameters, options parameter 'format' should be of type table
>+...
>+s:alter({format = true})
>+---
>+- error: Illegal parameters, options parameter 'format' should be of type table
>+...
>+s:alter({format = {{{1, 2, 3, 4}}}})
>+---
>+- error: 'Illegal parameters, format[1]: name (string) is expected'
>+...
>+--
>+-- Alter temporary.
>+--
>+s:alter({temporary = true})
>+---
>+...
>+assert(s.temporary)
>+---
>+- true
>+...
>+-- When not specified or nil - ignored.
>+s:alter({temporary = nil})
>+---
>+...
>+assert(s.temporary)
>+---
>+- true
>+...
>+s:alter({temporary = false})
>+---
>+...
>+assert(not s.temporary)
>+---
>+- true
>+...
>+-- Ensure absence is not treated like 'true'.
>+s:alter({temporary = nil})
>+---
>+...
>+assert(not s.temporary)
>+---
>+- true
>+...
>+-- Invalid values.
>+s:alter({temporary = box.NULL})
>+---
>+- error: Illegal parameters, options parameter 'temporary' should be of type boolean
>+...
>+s:alter({temporary = 100})
>+---
>+- error: Illegal parameters, options parameter 'temporary' should be of type boolean
>+...
>+--
>+-- Alter is_sync.
>+--
>+old_synchro_quorum = box.cfg.replication_synchro_quorum
>+---
>+...
>+old_synchro_timeout = box.cfg.replication_synchro_timeout
>+---
>+...
>+box.cfg{ \
>+ replication_synchro_quorum = 2, \
>+ replication_synchro_timeout = 0.001, \
>+}
>+---
>+...
>+s:alter({is_sync = true})
>+---
>+...
>+assert(s.is_sync)
>+---
>+- true
>+...
>+s:replace{1}
>+---
>+- error: Quorum collection for a synchronous transaction is timed out
>+...
>+-- When not specified or nil - ignored.
>+s:alter({is_sync = nil})
>+---
>+...
>+assert(s.is_sync)
>+---
>+- true
>+...
>+s:alter({is_sync = false})
>+---
>+...
>+assert(not s.is_sync)
>+---
>+- true
>+...
>+-- Ensure absence is not treated like 'true'.
>+s:alter({is_sync = nil})
>+---
>+...
>+assert(not s.is_sync)
>+---
>+- true
>+...
>+-- Invalid values.
>+s:alter({is_sync = box.NULL})
>+---
>+- error: Illegal parameters, options parameter 'is_sync' should be of type boolean
>+...
>+s:alter({is_sync = 100})
>+---
>+- error: Illegal parameters, options parameter 'is_sync' should be of type boolean
>+...
>+s:replace{1}
>+---
>+- [1]
>+...
>+s:delete{1}
>+---
>+- [1]
>+...
>+box.cfg{ \
>+ replication_synchro_quorum = old_synchro_quorum, \
>+ replication_synchro_timeout = old_synchro_timeout, \
>+}
>+---
>+...
>+-- Alter name.
>+s:alter({name = 'test2'})
>+---
>+...
>+assert(box.space.test2 ~= nil)
>+---
>+- true
>+...
>+assert(box.space.test == nil)
>+---
>+- true
>+...
>+assert(s.name == 'test2')
>+---
>+- true
>+...
>+-- When not specified or nil - ignored.
>+s:alter({name = nil})
>+---
>+...
>+assert(box.space.test2 ~= nil)
>+---
>+- true
>+...
>+assert(box.space.test == nil)
>+---
>+- true
>+...
>+assert(s.name == 'test2')
>+---
>+- true
>+...
>+s:alter({name = '_space'})
>+---
>+- error: Duplicate key exists in unique index 'name' in space '_space'
>+...
>+s:alter({name = 'test'})
>+---
>+...
>+assert(box.space.test ~= nil)
>+---
>+- true
>+...
>+assert(box.space.test2 == nil)
>+---
>+- true
>+...
>+assert(s.name == 'test')
>+---
>+- true
>+...
>+-- Invalid values.
>+s:alter({name = box.NULL})
>+---
>+- error: Illegal parameters, options parameter 'name' should be of type string
>+...
>+s:alter({name = 100})
>+---
>+- error: Illegal parameters, options parameter 'name' should be of type string
>+...
>+s:drop()
>+---
>+...
>+s:alter({})
>+---
>+- error: Space 'test' does not exist
>+...
>+ok, err = pcall(box.schema.space.alter, s.id, {})
>+---
>+...
>+assert(err:match('does not exist') ~= nil)
>+---
>+- true
>+...
>diff --git a/test/box/alter.test.lua b/test/box/alter.test.lua
>index 3cb0c4f84..abd08e2fa 100644
>--- a/test/box/alter.test.lua
>+++ b/test/box/alter.test.lua
>@@ -478,3 +478,145 @@ validate_indexes()
> 
> s:drop()
> box.internal.collation.drop('test')
>+
>+--
>+-- gh-5155: space:alter(), added in order to make it possible to turn on/off
>+-- is_sync, but it is handy for other options too.
>+--
>+s = box.schema.create_space('test')
>+pk = s:create_index('pk')
>+
>+-- Bad usage.
>+s:alter({unknown = true})
>+ok, err = pcall(s.alter, {})
>+assert(err:match("Use space:alter") ~= nil)
>+
>+-- Alter field_count.
>+s:replace{1}
>+-- Can't update on non-empty space.
>+s:alter({field_count = 2})
>+s:delete{1}
>+-- Can update on empty space.
>+s:alter({field_count = 2})
>+s:replace{1}
>+s:replace{1, 1}
>+-- When not specified or nil - ignored.
>+s:alter({field_count = nil})
>+s:replace{2}
>+s:replace{2, 2}
>+-- Set to 0 drops the restriction.
>+s:alter({field_count = 0})
>+s:truncate()
>+s:replace{1}
>+s:delete{1}
>+-- Invalid values.
>+s:alter({field_count = box.NULL})
>+s:alter({field_count = 'string'})
>+
>+-- Alter owner.
>+owner1 = box.space._space:get{s.id}.owner
>+box.schema.user.create('test')
>+-- When not specified or nil - ignored.
>+s:alter({user = nil})
>+owner2 = box.space._space:get{s.id}.owner
>+assert(owner2 == owner1)
>+s:alter({user = 'test'})
>+owner2 = box.space._space:get{s.id}.owner
>+assert(owner2 ~= owner1)
>+s:alter({user = owner1})
>+owner2 = box.space._space:get{s.id}.owner
>+assert(owner2 == owner1)
>+box.schema.user.drop('test')
>+-- Invalid values.
>+s:alter({user = box.NULL})
>+s:alter({user = true})
>+s:alter({user = 'not_existing'})
>+
>+-- Alter format.
>+format = {{name = 'field1', type = 'unsigned'}}
>+s:alter({format = format})
>+s:format()
>+-- When not specified or nil - ignored.
>+s:alter({format = nil})
>+s:format()
>+t = s:replace{1}
>+assert(t.field1 == 1)
>+s:alter({format = {}})
>+assert(t.field1 == nil)
>+s:delete{1}
>+-- Invalid values.
>+s:alter({format = box.NULL})
>+s:alter({format = true})
>+s:alter({format = {{{1, 2, 3, 4}}}})
>+
>+--
>+-- Alter temporary.
>+--
>+s:alter({temporary = true})
>+assert(s.temporary)
>+-- When not specified or nil - ignored.
>+s:alter({temporary = nil})
>+assert(s.temporary)
>+s:alter({temporary = false})
>+assert(not s.temporary)
>+-- Ensure absence is not treated like 'true'.
>+s:alter({temporary = nil})
>+assert(not s.temporary)
>+-- Invalid values.
>+s:alter({temporary = box.NULL})
>+s:alter({temporary = 100})
>+
>+--
>+-- Alter is_sync.
>+--
>+old_synchro_quorum = box.cfg.replication_synchro_quorum
>+old_synchro_timeout = box.cfg.replication_synchro_timeout
>+box.cfg{ \
>+ replication_synchro_quorum = 2, \
>+ replication_synchro_timeout = 0.001, \
>+}
>+s:alter({is_sync = true})
>+assert(s.is_sync)
>+s:replace{1}
>+-- When not specified or nil - ignored.
>+s:alter({is_sync = nil})
>+assert(s.is_sync)
>+s:alter({is_sync = false})
>+assert(not s.is_sync)
>+-- Ensure absence is not treated like 'true'.
>+s:alter({is_sync = nil})
>+assert(not s.is_sync)
>+-- Invalid values.
>+s:alter({is_sync = box.NULL})
>+s:alter({is_sync = 100})
>+s:replace{1}
>+s:delete{1}
>+box.cfg{ \
>+ replication_synchro_quorum = old_synchro_quorum, \
>+ replication_synchro_timeout = old_synchro_timeout, \
>+}
>+
>+-- Alter name.
>+s:alter({name = 'test2'})
>+assert(box.space.test2 ~= nil)
>+assert(box.space.test == nil)
>+assert(s.name == 'test2')
>+-- When not specified or nil - ignored.
>+s:alter({name = nil})
>+assert(box.space.test2 ~= nil)
>+assert(box.space.test == nil)
>+assert(s.name == 'test2')
>+s:alter({name = '_space'})
>+s:alter({name = 'test'})
>+assert(box.space.test ~= nil)
>+assert(box.space.test2 == nil)
>+assert(s.name == 'test')
>+-- Invalid values.
>+s:alter({name = box.NULL})
>+s:alter({name = 100})
>+
>+s:drop()
>+
>+s:alter({})
>+ok, err = pcall(box.schema.space.alter, s.id, {})
>+assert(err:match('does not exist') ~= nil)
>--
>2.21.1 (Apple Git-122.3)
 
 
LGTM.
--
Serge Petrenko

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