* [Tarantool-patches] [PATCH] box: disallow to modify format of a view @ 2020-09-22 15:59 Roman Khabibov 2020-09-24 20:59 ` Vladislav Shpilevoy 2020-09-25 20:14 ` Vladislav Shpilevoy 0 siblings, 2 replies; 10+ messages in thread From: Roman Khabibov @ 2020-09-22 15:59 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy Ban ability to modify format or field count of a view. Since a view is a named select, and not a space, in fact, changing view format is not a valid operation. --- Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/ban-view-mod @ChangeLog - Ban ability to modify view's format and field count. src/box/alter.cc | 40 +++++++++++++ test/box/view-dont-modify-format.result | 69 +++++++++++++++++++++++ test/box/view-dont-modify-format.test.lua | 29 ++++++++++ 3 files changed, 138 insertions(+) create mode 100644 test/box/view-dont-modify-format.result create mode 100644 test/box/view-dont-modify-format.test.lua diff --git a/src/box/alter.cc b/src/box/alter.cc index ba96d9c62..4d0da347a 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -2086,6 +2086,32 @@ update_view_references(struct Select *select, int update_value, return 0; } +/** + * Check whether space format is changed during UPDATE/REPLACE. + * + * Used to disallow view's format modification. + * + * @param old_tuple Tuple of a space before UPDATE/REPLACE. + * @param new_tuple Tuple of a space after UPDATE/REPLACE. + */ +static bool +format_is_changed(struct tuple *old_tuple, struct tuple *new_tuple) +{ + const char *old_format = tuple_field(old_tuple, BOX_SPACE_FIELD_FORMAT); + const char *new_format = tuple_field(new_tuple, BOX_SPACE_FIELD_FORMAT); + assert(old_format != NULL && new_format != NULL); + const char *end = old_format; + mp_next(&end); + uint32_t length = end - old_format; + end = new_format; + mp_next(&end); + if (length != end - new_format) + return true; + if (memcmp(old_format, new_format, length) != 0) + return true; + return false; +} + /** * Trigger which is fired to commit creation of new SQL view. * Its purpose is to release memory of SELECT. @@ -2441,6 +2467,20 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) "view"); return -1; } + if (def->opts.is_view && old_space->def->field_count != + def->field_count) { + diag_set(ClientError, ER_ALTER_SPACE, + space_name(old_space), + "can not modify field count of a view"); + return -1; + } + if (def->opts.is_view && format_is_changed(old_tuple, + new_tuple)) { + diag_set(ClientError, ER_ALTER_SPACE, + space_name(old_space), + "can not modify format of a view"); + return -1; + } /* * Allow change of space properties, but do it * in WAL-error-safe mode. diff --git a/test/box/view-dont-modify-format.result b/test/box/view-dont-modify-format.result new file mode 100644 index 000000000..0d8c50601 --- /dev/null +++ b/test/box/view-dont-modify-format.result @@ -0,0 +1,69 @@ +-- test-run result file version 2 +-- +-- Make sure we can't modify a view format. +-- +box.execute("CREATE TABLE t1 (a INT PRIMARY KEY);") + | --- + | - row_count: 1 + | ... +box.execute("CREATE VIEW v AS SELECT * FROM t1;") + | --- + | - row_count: 1 + | ... + +-- +-- Try to add a field. +-- +view = box.space._space.index[2]:select('V')[1]:totable() + | --- + | ... +view_format = box.space.V:format() + | --- + | ... +f = {type = 'string', nullable_action = 'none', name = 'B', is_nullable = true} + | --- + | ... +table.insert(view_format, f) + | --- + | ... +view[5] = 2 + | --- + | ... +view[7] = view_format + | --- + | ... +box.space._space:replace(view) + | --- + | - error: 'Can''t modify space ''V'': can not modify field count of a view' + | ... + +-- +-- Try to modify format only. +-- +view = box.space.V + | --- + | ... +view:format{} + | --- + | - error: 'Can''t modify space ''V'': can not modify field count of a view' + | ... + +view_format = box.space.V:format() + | --- + | ... +view_format[1].name = 'B' + | --- + | ... +view:format(view_format) + | --- + | - error: 'Can''t modify space ''V'': can not modify format of a view' + | ... + +box.execute("DROP VIEW v;") + | --- + | - row_count: 1 + | ... +box.execute("DROP TABLE t1;") + | --- + | - row_count: 1 + | ... diff --git a/test/box/view-dont-modify-format.test.lua b/test/box/view-dont-modify-format.test.lua new file mode 100644 index 000000000..e553e7012 --- /dev/null +++ b/test/box/view-dont-modify-format.test.lua @@ -0,0 +1,29 @@ +-- +-- Make sure we can't modify a view format. +-- +box.execute("CREATE TABLE t1 (a INT PRIMARY KEY);") +box.execute("CREATE VIEW v AS SELECT * FROM t1;") + +-- +-- Try to add a field. +-- +view = box.space._space.index[2]:select('V')[1]:totable() +view_format = box.space.V:format() +f = {type = 'string', nullable_action = 'none', name = 'B', is_nullable = true} +table.insert(view_format, f) +view[5] = 2 +view[7] = view_format +box.space._space:replace(view) + +-- +-- Try to modify format only. +-- +view = box.space.V +view:format{} + +view_format = box.space.V:format() +view_format[1].name = 'B' +view:format(view_format) + +box.execute("DROP VIEW v;") +box.execute("DROP TABLE t1;") -- 2.24.3 (Apple Git-128) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH] box: disallow to modify format of a view 2020-09-22 15:59 [Tarantool-patches] [PATCH] box: disallow to modify format of a view Roman Khabibov @ 2020-09-24 20:59 ` Vladislav Shpilevoy 2020-09-25 10:18 ` Roman Khabibov 2020-09-25 20:14 ` Vladislav Shpilevoy 1 sibling, 1 reply; 10+ messages in thread From: Vladislav Shpilevoy @ 2020-09-24 20:59 UTC (permalink / raw) To: Roman Khabibov, tarantool-patches Hi! Thanks for the patch! > diff --git a/src/box/alter.cc b/src/box/alter.cc > index ba96d9c62..4d0da347a 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -2441,6 +2467,20 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) > "view"); > return -1; > } > + if (def->opts.is_view && old_space->def->field_count != > + def->field_count) { > + diag_set(ClientError, ER_ALTER_SPACE, > + space_name(old_space), > + "can not modify field count of a view"); > + return -1; > + } > + if (def->opts.is_view && format_is_changed(old_tuple, > + new_tuple)) { > + diag_set(ClientError, ER_ALTER_SPACE, > + space_name(old_space), > + "can not modify format of a view"); > + return -1; > + } Wouldn't it be easier to just prohibit update of such tuple completely? Raise an error if new_tuple != NULL and old_tuple != NULL and the space is a view. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH] box: disallow to modify format of a view 2020-09-24 20:59 ` Vladislav Shpilevoy @ 2020-09-25 10:18 ` Roman Khabibov 0 siblings, 0 replies; 10+ messages in thread From: Roman Khabibov @ 2020-09-25 10:18 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hi! Thanks for the review. > On Sep 24, 2020, at 23:59, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: > > Hi! Thanks for the patch! > >> diff --git a/src/box/alter.cc b/src/box/alter.cc >> index ba96d9c62..4d0da347a 100644 >> --- a/src/box/alter.cc >> +++ b/src/box/alter.cc >> @@ -2441,6 +2467,20 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) >> "view"); >> return -1; >> } >> + if (def->opts.is_view && old_space->def->field_count != >> + def->field_count) { >> + diag_set(ClientError, ER_ALTER_SPACE, >> + space_name(old_space), >> + "can not modify field count of a view"); >> + return -1; >> + } >> + if (def->opts.is_view && format_is_changed(old_tuple, >> + new_tuple)) { >> + diag_set(ClientError, ER_ALTER_SPACE, >> + space_name(old_space), >> + "can not modify format of a view"); >> + return -1; >> + } > > Wouldn't it be easier to just prohibit update of such > tuple completely? Raise an error if new_tuple != NULL and > old_tuple != NULL and the space is a view. For some reason, I thought that you can rename views, because some vendors allow it in ALTER TABLE RENAME TO. But I looked at our standard file: you really can't rename a view. So that, you are right. commit 515f2b8315fd266a1e9a13ab1b3357c680786cd5 Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Fri Sep 11 09:33:39 2020 +0300 box: disallow to alter view Ban ability to modify view on box level. Since a view is a named select, and not a space, in fact, altering view is not a valid operation. diff --git a/src/box/alter.cc b/src/box/alter.cc index ba96d9c62..22578c9c4 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -2398,6 +2398,12 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) } } else { /* UPDATE, REPLACE */ assert(old_space != NULL && new_tuple != NULL); + if (old_space->def->opts.is_view) { + diag_set(ClientError, ER_ALTER_SPACE, + space_name(old_space), + "view may not be altered"); + return -1; + } struct space_def *def = space_def_new_from_tuple(new_tuple, ER_ALTER_SPACE, region); diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c index 14f6c1a97..54a8d5b80 100644 --- a/src/box/sql/alter.c +++ b/src/box/sql/alter.c @@ -60,11 +60,6 @@ sql_alter_table_rename(struct Parse *parse) diag_set(ClientError, ER_NO_SUCH_SPACE, tbl_name); goto tnt_error; } - if (space->def->opts.is_view) { - diag_set(ClientError, ER_ALTER_SPACE, tbl_name, - "view may not be altered"); - goto tnt_error; - } sql_set_multi_write(parse, false); /* Drop and reload the internal table schema. */ struct Vdbe *v = sqlGetVdbe(parse); diff --git a/test/sql/view.result b/test/sql/view.result index 1e73ff621..b556b5482 100644 --- a/test/sql/view.result +++ b/test/sql/view.result @@ -44,7 +44,7 @@ v1[6]['view'] = false; ... box.space._space:replace(v1); --- -- error: 'Can''t modify space ''V1'': can not convert a space to a view and vice versa' +- error: 'Can''t modify space ''V1'': view may not be altered' ... t1 = box.space._space.index[2]:select('T1')[1]:totable(); --- @@ -68,7 +68,7 @@ v1[6]['view'] = true; ... box.space._space:replace(v1); --- -- error: Space declared as a view must have SQL statement +- error: 'Can''t modify space ''V1'': view may not be altered' ... -- Views can't be created via space_create(). box.schema.create_space('view', {view = true}) @@ -400,3 +400,106 @@ box.execute('DROP TABLE t1;') --- - row_count: 1 ... +-- +-- Make sure that we can't alter a view. +-- +box.execute("CREATE TABLE t1 (a INT PRIMARY KEY);") +--- +- row_count: 1 +... +box.execute("CREATE VIEW v AS SELECT * FROM t1;") +--- +- row_count: 1 +... +-- +-- Try to change owner. +-- +view = box.space._space.index[2]:select('V')[1]:totable() +--- +... +view[2] = 1 +--- +... +box.space._space:replace(view) +--- +- error: 'Can''t modify space ''V'': view may not be altered' +... +-- +-- Try to rename. +-- +view = box.space._space.index[2]:select('V')[1]:totable() +--- +... +view[3] = 'a' +--- +... +box.space._space:replace(view) +--- +- error: 'Can''t modify space ''V'': view may not be altered' +... +-- +-- Try to change engine. +-- +view = box.space._space.index[2]:select('V')[1]:totable() +--- +... +view[4] = 'a' +--- +... +box.space._space:replace(view) +--- +- error: 'Can''t modify space ''V'': view may not be altered' +... +-- +-- Try to add a field. +-- +view = box.space._space.index[2]:select('V')[1]:totable() +--- +... +view_format = box.space.V:format() +--- +... +f = {type = 'string', nullable_action = 'none', name = 'B', is_nullable = true} +--- +... +table.insert(view_format, f) +--- +... +view[5] = 2 +--- +... +view[7] = view_format +--- +... +box.space._space:replace(view) +--- +- error: 'Can''t modify space ''V'': view may not be altered' +... +-- +-- Try to modify format only. +-- +view = box.space.V +--- +... +view:format{} +--- +- error: 'Can''t modify space ''V'': view may not be altered' +... +view_format = box.space.V:format() +--- +... +view_format[1].name = 'B' +--- +... +view:format(view_format) +--- +- error: 'Can''t modify space ''V'': view may not be altered' +... +box.execute("DROP VIEW v;") +--- +- row_count: 1 +... +box.execute("DROP TABLE t1;") +--- +- row_count: 1 +... diff --git a/test/sql/view.test.lua b/test/sql/view.test.lua index 47ca726af..2500c9e7a 100644 --- a/test/sql/view.test.lua +++ b/test/sql/view.test.lua @@ -138,3 +138,54 @@ box.execute('SELECT * FROM t1;') box.execute('DROP VIEW v;') box.execute('DROP TABLE t;') box.execute('DROP TABLE t1;') + +-- +-- Make sure that we can't alter a view. +-- +box.execute("CREATE TABLE t1 (a INT PRIMARY KEY);") +box.execute("CREATE VIEW v AS SELECT * FROM t1;") + +-- +-- Try to change owner. +-- +view = box.space._space.index[2]:select('V')[1]:totable() +view[2] = 1 +box.space._space:replace(view) + +-- +-- Try to rename. +-- +view = box.space._space.index[2]:select('V')[1]:totable() +view[3] = 'a' +box.space._space:replace(view) + +-- +-- Try to change engine. +-- +view = box.space._space.index[2]:select('V')[1]:totable() +view[4] = 'a' +box.space._space:replace(view) + +-- +-- Try to add a field. +-- +view = box.space._space.index[2]:select('V')[1]:totable() +view_format = box.space.V:format() +f = {type = 'string', nullable_action = 'none', name = 'B', is_nullable = true} +table.insert(view_format, f) +view[5] = 2 +view[7] = view_format +box.space._space:replace(view) + +-- +-- Try to modify format only. +-- +view = box.space.V +view:format{} + +view_format = box.space.V:format() +view_format[1].name = 'B' +view:format(view_format) + +box.execute("DROP VIEW v;") +box.execute("DROP TABLE t1;") ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH] box: disallow to modify format of a view 2020-09-22 15:59 [Tarantool-patches] [PATCH] box: disallow to modify format of a view Roman Khabibov 2020-09-24 20:59 ` Vladislav Shpilevoy @ 2020-09-25 20:14 ` Vladislav Shpilevoy 2020-09-26 13:01 ` Roman Khabibov 1 sibling, 1 reply; 10+ messages in thread From: Vladislav Shpilevoy @ 2020-09-25 20:14 UTC (permalink / raw) To: Roman Khabibov, tarantool-patches Hi! Thanks for the patch! LGTM. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH] box: disallow to modify format of a view 2020-09-25 20:14 ` Vladislav Shpilevoy @ 2020-09-26 13:01 ` Roman Khabibov 2020-09-28 15:52 ` Nikita Pettik 0 siblings, 1 reply; 10+ messages in thread From: Roman Khabibov @ 2020-09-26 13:01 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches, Vladislav Shpilevoy Hi! Thanks. Nikita, could you, please, look through? > On Sep 25, 2020, at 23:14, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: > > Hi! Thanks for the patch! > > LGTM. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH] box: disallow to modify format of a view 2020-09-26 13:01 ` Roman Khabibov @ 2020-09-28 15:52 ` Nikita Pettik 2020-09-28 18:41 ` Vladislav Shpilevoy 0 siblings, 1 reply; 10+ messages in thread From: Nikita Pettik @ 2020-09-28 15:52 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches, Vladislav Shpilevoy On 26 Sep 16:01, Roman Khabibov wrote: > Hi! Thanks. > > Nikita, could you, please, look through? Changed error msg to 'view can not be altered' and pushed to master. Changelog for 2.6 is updated; branch is dropped. > > On Sep 25, 2020, at 23:14, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: > > > > Hi! Thanks for the patch! > > > > LGTM. > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH] box: disallow to modify format of a view 2020-09-28 15:52 ` Nikita Pettik @ 2020-09-28 18:41 ` Vladislav Shpilevoy 2020-09-28 23:50 ` Nikita Pettik 0 siblings, 1 reply; 10+ messages in thread From: Vladislav Shpilevoy @ 2020-09-28 18:41 UTC (permalink / raw) To: Nikita Pettik, Roman Khabibov; +Cc: tarantool-patches It seems this is a bugfix and should be pushed to the older version too. On 28.09.2020 17:52, Nikita Pettik wrote: > On 26 Sep 16:01, Roman Khabibov wrote: >> Hi! Thanks. >> >> Nikita, could you, please, look through? > > Changed error msg to 'view can not be altered' and pushed to master. > Changelog for 2.6 is updated; branch is dropped. > >>> On Sep 25, 2020, at 23:14, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: >>> >>> Hi! Thanks for the patch! >>> >>> LGTM. >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH] box: disallow to modify format of a view 2020-09-28 18:41 ` Vladislav Shpilevoy @ 2020-09-28 23:50 ` Nikita Pettik 2020-10-01 22:35 ` Vladislav Shpilevoy 0 siblings, 1 reply; 10+ messages in thread From: Nikita Pettik @ 2020-09-28 23:50 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches On 28 Sep 20:41, Vladislav Shpilevoy wrote: > It seems this is a bugfix and should be pushed to the older version too. I consider this change to be very minor and don't think it is worth backporting. If you really feel that it must be backported - feel free to do it. > On 28.09.2020 17:52, Nikita Pettik wrote: > > On 26 Sep 16:01, Roman Khabibov wrote: > >> Hi! Thanks. > >> > >> Nikita, could you, please, look through? > > > > Changed error msg to 'view can not be altered' and pushed to master. > > Changelog for 2.6 is updated; branch is dropped. > > > >>> On Sep 25, 2020, at 23:14, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: > >>> > >>> Hi! Thanks for the patch! > >>> > >>> LGTM. > >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH] box: disallow to modify format of a view 2020-09-28 23:50 ` Nikita Pettik @ 2020-10-01 22:35 ` Vladislav Shpilevoy 2020-10-02 11:13 ` Nikita Pettik 0 siblings, 1 reply; 10+ messages in thread From: Vladislav Shpilevoy @ 2020-10-01 22:35 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches On 29.09.2020 01:50, Nikita Pettik wrote: > On 28 Sep 20:41, Vladislav Shpilevoy wrote: >> It seems this is a bugfix and should be pushed to the older version too. > > I consider this change to be very minor and don't think it is worth > backporting. If you really feel that it must be backported - feel free to do it. Я не догоняю че-то. У нас есть четкая политика - все багфиксы пропушиваются на все поддерживаемые версии, где эти баги есть. Тут нет правила, что ну баг вроде мелкий, можно забить. Нет никакой двузначности. Тут даже конфликтов нет, это тупо черри-пик. Я пропушил в 2.5 и 2.4, обновил changelog. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH] box: disallow to modify format of a view 2020-10-01 22:35 ` Vladislav Shpilevoy @ 2020-10-02 11:13 ` Nikita Pettik 0 siblings, 0 replies; 10+ messages in thread From: Nikita Pettik @ 2020-10-02 11:13 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches On 02 Oct 00:35, Vladislav Shpilevoy wrote: > On 29.09.2020 01:50, Nikita Pettik wrote: > > On 28 Sep 20:41, Vladislav Shpilevoy wrote: > >> It seems this is a bugfix and should be pushed to the older version too. > > > > I consider this change to be very minor and don't think it is worth > > backporting. If you really feel that it must be backported - feel free to do it. > > Я не догоняю че-то. У нас есть четкая политика - все багфиксы > пропушиваются на все поддерживаемые версии, где эти баги есть. Тут > нет правила, что ну баг вроде мелкий, можно забить. Нет никакой > двузначности. Тут даже конфликтов нет, это тупо черри-пик. > > Я пропушил в 2.5 и 2.4, обновил changelog. Влад, отстрелить себе ногу (руку, голову, палец соседа) в СКЛ можно 1001 способом, но это не значит, что на каждый случай стоит того, чтобы писать очередную затычку (по моему мнению) и заносить ее во все ветки. Но спасибо, что сделал это. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-02 11:13 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-22 15:59 [Tarantool-patches] [PATCH] box: disallow to modify format of a view Roman Khabibov 2020-09-24 20:59 ` Vladislav Shpilevoy 2020-09-25 10:18 ` Roman Khabibov 2020-09-25 20:14 ` Vladislav Shpilevoy 2020-09-26 13:01 ` Roman Khabibov 2020-09-28 15:52 ` Nikita Pettik 2020-09-28 18:41 ` Vladislav Shpilevoy 2020-09-28 23:50 ` Nikita Pettik 2020-10-01 22:35 ` Vladislav Shpilevoy 2020-10-02 11:13 ` Nikita Pettik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox