[tarantool-patches] [PATCH] Do not enable commit if read_only = true
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Feb 27 12:02:37 MSK 2019
On Wed, Feb 27, 2019 at 10:36:00AM +0300, Georgy Kirichenko wrote:
> Disable commit if server is in read only mode.
The commit message is very poor. Please elaborate why this is important.
>
> Closes: #4016
> ---
> Issue: https://github.com/tarantool/tarantool/issues/4016
> Branch: https://github.com/tarantool/tarantool/tree/g.kirichenko/gh-4016-readonly-commit
> src/box/box.cc | 2 +-
> src/box/box.h | 3 +++
> src/box/txn.c | 6 ++++++
> test/box/misc.result | 19 +++++++++++++++++++
> test/box/misc.test.lua | 8 ++++++++
> 5 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 73d94f79b..ec196bcc0 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -138,7 +138,7 @@ static struct fiber_pool tx_fiber_pool;
> */
> static struct cbus_endpoint tx_prio_endpoint;
>
> -static int
> +int
> box_check_writable(void)
> {
> /* box is only writable if box.cfg.read_only == false and */
> diff --git a/src/box/box.h b/src/box/box.h
> index 9f5b3acbd..d9e403d7c 100644
> --- a/src/box/box.h
> +++ b/src/box/box.h
> @@ -101,6 +101,9 @@ box_set_ro(bool ro);
> bool
> box_is_ro(void);
>
> +int
> +box_check_writable(void);
> +
> /**
> * Wait until the instance switches to a desired mode.
> * \param ro wait read-only if set or read-write if unset
> diff --git a/src/box/txn.c b/src/box/txn.c
> index d55d5b93c..769a57a5a 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -34,6 +34,7 @@
> #include "journal.h"
> #include <fiber.h>
> #include "xrow.h"
> +#include "box.h"
Ouch. Can we avoid introducing this dependency?
>
> double too_long_threshold;
>
> @@ -448,6 +449,11 @@ box_txn_commit()
> */
> if (! txn)
> return 0;
> + /*
> + * Check that tarantool didn't switch to ro.
> + */
> + if (box_check_writable() != 0)
> + return -1;
What about temporary and local spaces? We don't want this check to fail
transactions for those. Please fix and add a corresponding test case.
Also, may be it's worth moving the ro check completely to txn_commit?
IMO it looks weird that we check it both when processing a request and
when committing a transaction.
An alternative approach would be setting a trigger on yield and checking
that we are still rw on resume, aborting transactions if we are not.
This would remove the check on txn_commit and probably allow us to
eliminate box.h dependency. Please check it out.
Also, please try to implement a test that checks this for vinyl +
replication. After all, this problem is only relevant to vinyl.
> if (txn->in_sub_stmt) {
> diag_set(ClientError, ER_COMMIT_IN_SUB_STMT);
> return -1;
More information about the Tarantool-patches
mailing list