From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 37DB3469719 for ; Fri, 25 Sep 2020 13:18:51 +0300 (MSK) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\)) From: Roman Khabibov In-Reply-To: <0785ca58-7933-0b97-9dba-15882a8b7c75@tarantool.org> Date: Fri, 25 Sep 2020 13:18:48 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20200922155903.48421-1-roman.habibov@tarantool.org> <0785ca58-7933-0b97-9dba-15882a8b7c75@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] box: disallow to modify format of a view List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. > On Sep 24, 2020, at 23:59, Vladislav Shpilevoy = wrote: >=20 > Hi! Thanks for the patch! >=20 >> 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 !=3D >> + 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; >> + } >=20 > Wouldn't it be easier to just prohibit update of such > tuple completely? Raise an error if new_tuple !=3D NULL and > old_tuple !=3D 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 Date: Fri Sep 11 09:33:39 2020 +0300 box: disallow to alter view =20 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 !=3D NULL && new_tuple !=3D 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 =3D 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 =3D 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'] =3D 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 =3D box.space._space.index[2]:select('T1')[1]:totable(); --- @@ -68,7 +68,7 @@ v1[6]['view'] =3D 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 =3D 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 =3D box.space._space.index[2]:select('V')[1]:totable() +--- +... +view[2] =3D 1 +--- +... +box.space._space:replace(view) +--- +- error: 'Can''t modify space ''V'': view may not be altered' +... +-- +-- Try to rename. +-- +view =3D box.space._space.index[2]:select('V')[1]:totable() +--- +... +view[3] =3D 'a' +--- +... +box.space._space:replace(view) +--- +- error: 'Can''t modify space ''V'': view may not be altered' +... +-- +-- Try to change engine. +-- +view =3D box.space._space.index[2]:select('V')[1]:totable() +--- +... +view[4] =3D 'a' +--- +... +box.space._space:replace(view) +--- +- error: 'Can''t modify space ''V'': view may not be altered' +... +-- +-- Try to add a field. +-- +view =3D box.space._space.index[2]:select('V')[1]:totable() +--- +... +view_format =3D box.space.V:format() +--- +... +f =3D {type =3D 'string', nullable_action =3D 'none', name =3D 'B', = is_nullable =3D true} +--- +... +table.insert(view_format, f) +--- +... +view[5] =3D 2 +--- +... +view[7] =3D view_format +--- +... +box.space._space:replace(view) +--- +- error: 'Can''t modify space ''V'': view may not be altered' +... +-- +-- Try to modify format only. +-- +view =3D box.space.V +--- +... +view:format{} +--- +- error: 'Can''t modify space ''V'': view may not be altered' +... +view_format =3D box.space.V:format() +--- +... +view_format[1].name =3D '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 =3D box.space._space.index[2]:select('V')[1]:totable() +view[2] =3D 1 +box.space._space:replace(view) + +-- +-- Try to rename. +-- +view =3D box.space._space.index[2]:select('V')[1]:totable() +view[3] =3D 'a' +box.space._space:replace(view) + +-- +-- Try to change engine. +-- +view =3D box.space._space.index[2]:select('V')[1]:totable() +view[4] =3D 'a' +box.space._space:replace(view) + +-- +-- Try to add a field. +-- +view =3D box.space._space.index[2]:select('V')[1]:totable() +view_format =3D box.space.V:format() +f =3D {type =3D 'string', nullable_action =3D 'none', name =3D 'B', = is_nullable =3D true} +table.insert(view_format, f) +view[5] =3D 2 +view[7] =3D view_format +box.space._space:replace(view) + +-- +-- Try to modify format only. +-- +view =3D box.space.V +view:format{} + +view_format =3D box.space.V:format() +view_format[1].name =3D 'B' +view:format(view_format) + +box.execute("DROP VIEW v;") +box.execute("DROP TABLE t1;")