Re: [Tarantool-patches] [PATCH] sql: forbid data changes in read-only mode

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for the patch. See 5 comments below.

On Tue, Nov 10, 2020 at 02:29:13PM +0300, sergos@tarantool.org wrote:
> From: Sergey Ostanevich <sergos@tarantool.org>
> 
> 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
> 
1. As far as I know, these links should be located below '---'.

2. Shouldn't there be a ChangeLog here? After all it is a user-visible change.

> ---
>  src/box/sql.c                                 |  9 ++-
>  .../gh-5231-box-execute-writes-in-ro.test.lua | 74 +++++++++++++++++++
>  2 files changed, 79 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..11f4b8ee4 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -54,6 +54,7 @@
>  #include "fk_constraint.h"
>  #include "mpstream/mpstream.h"
>  #include "sql_stmt_cache.h"
> +#include "backtrace.h"
3. What is this for?

>  
>  static sql *db = NULL;
>  
> @@ -453,7 +454,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 +532,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 +598,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 +835,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..913bc0981
> --- /dev/null
> +++ b/test/sql-tap/gh-5231-box-execute-writes-in-ro.test.lua
> @@ -0,0 +1,74 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +
> +local test = tap.test('gh-5231-box-execute-writes-in-ro')
> +local expected_err = "Can't modify data because this instance is in read-only mode."
> +
> +box.cfg()
> +box.execute("CREATE TABLE TEST (A INT, B INT, PRIMARY KEY (A))")
> +local res, err = box.execute("INSERT INTO TEST (A, B) VALUES (3, 3)")
> +box.cfg{read_only = true}
> +
> +test:plan(9)
> +
> +res, err = box.execute("INSERT INTO TEST (A, B) VALUES (1, 1)")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "insert should fail in read-only mode"
> +)
> +res, err = box.execute("DELETE FROM TEST")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "delete should fail in read-only mode"
> +)
> +res, err = box.execute("REPLACE INTO TEST VALUES (1, 2)")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "replace should fail in read-only mode"
> +)
> +res, err = box.execute("UPDATE TEST SET B=4 WHERE A=3")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "update should fail in read-only mode"
> +)
> +res, err = box.execute("TRUNCATE TABLE TEST")
> +test:is(
> +    tostring(err), 
4. Trailing whitespace.

> +    expected_err,
> +    "truncate should fail in read-only mode"
> +)
> +box.execute("CREATE TABLE TEST2 (A INT, PRIMARY KEY (A))")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "create should fail in read-only mode"
> +)
> +box.execute("ALTER TABLE TEST ADD CONSTRAINT 'uk' UNIQUE (B)")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "add constraint should fail in read-only mode"
> +)
> +box.execute("ALTER TABLE TEST RENAME TO TEST2")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "rename should fail in read-only mode"
> +)
> +res, err = box.execute("DROP TABLE TEST")
> +test:is(
> +    tostring(err),
> +    expected_err,
> +    "drop table should fail in read-only mode"
> +)
> +
> +-- cleanup
> +box.cfg{read_only=false}
> +res, err = box.execute("DROP TABLE TEST")
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.24.3 (Apple Git-128)
> 
5. About this test - almost all sql-tap tests use specialized functions. Is
there a reason we shouldn't be using them here? I propose to rewrite this test
using these functions.