From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (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 70DBB469710 for ; Fri, 20 Nov 2020 16:47:39 +0300 (MSK) Date: Fri, 20 Nov 2020 16:47:37 +0300 From: Mergen Imeev Message-ID: <20201120134737.GA492604@tarantool.org> References: <20201110112913.28083-1-sergos@tarantool.org> <20201116103129.GA324907@tarantool.org> <05C5398B-E522-4855-AA34-FB9B41606098@tarantool.org> <20201116210931.GB13996@tarantool.org> <0174562A-F615-4DA8-B265-2124388131AE@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <0174562A-F615-4DA8-B265-2124388131AE@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for the fixes. I have one unimportant comment: next time please include link to branch https://github.com/tarantool/tarantool/tree/sergos/gh-5231-box-execute-writes-in-ro instead of link to compare https://github.com/tarantool/tarantool/compare/sergos/gh-5231-box-execute-writes-in-ro LGTM. On Thu, Nov 19, 2020 at 10:41:23AM +0300, Sergey Ostanevich wrote: > Hi! > > Thanks for review! Updated, force-pushed, new patch attached. > > Sergos > > > On 17 Nov 2020, at 00:09, Nikita Pettik wrote: > > > > On 16 Nov 23:36, Sergey Ostanevich wrote: > >> diff --git a/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua > >> new file mode 100755 > >> index 000000000..6d95adc2e > >> --- /dev/null > >> +++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua > >> @@ -0,0 +1,105 @@ > >> +#!/usr/bin/env tarantool > >> + > >> +test = require("sqltester") > >> +test:plan(9) > >> + > >> +local expected_err = "Can't modify data because this instance is in read-only mode." > >> + > >> +test:execsql([[ > >> + CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A)); > >> + INSERT INTO TEST (A, B) VALUES (3, 3); > >> +]]) > >> + > >> +box.cfg{read_only = true} > >> + > >> +test:do_catchsql_test( > >> + "sql-errors-1.1", > >> + [[ > >> + INSERT INTO TEST (A, B) VALUES (1, 1); > >> + ]], { > >> + -- > > > > These comments are artifacts from porting SQLite test suite with > > auto-tools, they are not really needed in new tests :) > > > ============= > > From 4804ac485b38ddd280735970f6b9422bad1b30b6 Mon Sep 17 00:00:00 2001 > From: Sergey Ostanevich > Date: Tue, 10 Nov 2020 13:52:10 +0300 > Subject: [PATCH] sql: forbid data changes in read-only mode > > A number of places in sql.c uses direct access to box_process_rw() that > does not check read-only setting. Fixed by use of an intended interface > of box_process1(). > > Closes #5231 > --- > > Branch: https://github.com/tarantool/tarantool/compare/sergos/gh-5231-box-execute-writes-in-ro > Issue: https://github.com/tarantool/tarantool/issues/5231 > > @ChangeLog > - Data changes are blocked for SQL if node is read-only (gh-5231). > > src/box/sql.c | 8 +- > .../gh-5231-box-execute-writes-in-ro.test.lua | 87 +++++++++++++++++++ > 2 files changed, 91 insertions(+), 4 deletions(-) > create mode 100755 test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua > > diff --git a/src/box/sql.c b/src/box/sql.c > index a551bffc3..c55d0f4e5 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -453,7 +453,7 @@ insertOrReplace(struct space *space, const char *tuple, const char *tuple_end, > request.space_id = space->def->id; > request.type = type; > mp_tuple_assert(request.tuple, request.tuple_end); > - return box_process_rw(&request, space, NULL); > + return box_process1(&request, NULL); > } > > int tarantoolsqlInsert(struct space *space, const char *tuple, > @@ -531,7 +531,7 @@ sql_delete_by_key(struct space *space, uint32_t iid, char *key, > request.space_id = space->def->id; > request.index_id = iid; > assert(space_index(space, iid)->def->opts.is_unique); > - return box_process_rw(&request, space, &unused); > + return box_process1(&request, &unused); > } > > /* > @@ -597,7 +597,7 @@ int tarantoolsqlClearTable(struct space *space, uint32_t *tuple_count) > request.key = tuple_extract_key(tuple, pk->def->key_def, > MULTIKEY_NONE, &key_size); > request.key_end = request.key + key_size; > - rc = box_process_rw(&request, space, &unused); > + rc = box_process1(&request, &unused); > if (rc != 0) { > iterator_delete(iter); > return -1; > @@ -834,7 +834,7 @@ tarantoolsqlIncrementMaxid(uint64_t *space_max_id) > request.key_end = key + sizeof(key); > request.type = IPROTO_UPDATE; > request.space_id = space_schema->def->id; > - if (box_process_rw(&request, space_schema, &res) != 0 || res == NULL || > + if (box_process1(&request, &res) != 0 || res == NULL || > tuple_field_u64(res, 1, space_max_id) != 0) > return -1; > return 0; > diff --git a/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua > new file mode 100755 > index 000000000..05c23c2f8 > --- /dev/null > +++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua > @@ -0,0 +1,87 @@ > +#!/usr/bin/env tarantool > + > +test = require("sqltester") > +test:plan(9) > + > +local expected_err = "Can't modify data because this instance is in read-only mode." > + > +test:execsql([[ > + CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A)); > + INSERT INTO TEST (A, B) VALUES (3, 3); > +]]) > + > +box.cfg{read_only = true} > + > +test:do_catchsql_test( > + "gh-5231-1", > + [[ > + INSERT INTO TEST (A, B) VALUES (1, 1); > + ]], { > + 1, expected_err > + }) > + > +test:do_catchsql_test( > + "gh-5231-2", > + [[ > + DELETE FROM TEST; > + ]], { > + 1, expected_err > + }) > + > +test:do_catchsql_test( > + "gh-5231-3", > + [[ > + REPLACE INTO TEST VALUES (1, 2); > + ]], { > + 1, expected_err > + }) > + > +test:do_catchsql_test( > + "gh-5231-4", > + [[ > + UPDATE TEST SET B=4 WHERE A=3; > + ]], { > + 1, expected_err > + }) > + > +test:do_catchsql_test( > + "gh-5231-5", > + [[ > + TRUNCATE TABLE TEST; > + ]], { > + 1, expected_err > + }) > + > +test:do_catchsql_test( > + "gh-5231-6", > + [[ > + CREATE TABLE TEST2 (A INT, PRIMARY KEY (A)); > + ]], { > + 1, expected_err > + }) > + > +test:do_catchsql_test( > + "gh-5231-7", > + [[ > + ALTER TABLE TEST ADD CONSTRAINT UK_CON UNIQUE (B); > + ]], { > + 1, expected_err > + }) > + > +test:do_catchsql_test( > + "gh-5231-8", > + [[ > + ALTER TABLE TEST RENAME TO TEST2; > + ]], { > + 1, expected_err > + }) > + > +test:do_catchsql_test( > + "gh-5231-9", > + [[ > + DROP TABLE TEST; > + ]], { > + 1, expected_err > + }) > + > +test:finish_test() > -- > 2.24.3 (Apple Git-128) > >