From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 27 Feb 2019 12:02:37 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH] Do not enable commit if read_only = true Message-ID: <20190227090237.lu32o77lkyllgvui@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Georgy Kirichenko Cc: tarantool-patches@freelists.org List-ID: 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 > #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;