* [tarantool-patches] [PATCH] Do not enable commit if read_only = true
@ 2019-02-27 7:36 Georgy Kirichenko
2019-02-27 9:02 ` Vladimir Davydov
0 siblings, 1 reply; 5+ messages in thread
From: Georgy Kirichenko @ 2019-02-27 7:36 UTC (permalink / raw)
To: tarantool-patches; +Cc: Georgy Kirichenko
Disable commit if server is in read only mode.
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"
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;
if (txn->in_sub_stmt) {
diag_set(ClientError, ER_COMMIT_IN_SUB_STMT);
return -1;
diff --git a/test/box/misc.result b/test/box/misc.result
index 699358d53..80dfc4cf7 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -1207,3 +1207,22 @@ box.cfg{too_long_threshold = too_long_threshold}
s:drop()
---
...
+-- Commit after read_only = true (gh-4016).
+s = box.schema.space.create('test')
+---
+...
+_ = s:create_index('pk')
+---
+...
+box.begin() s:replace({1}) box.cfg{read_only = true} box.commit()
+---
+...
+box.rollback()
+---
+...
+box.cfg{read_only = false}
+---
+...
+s:drop()
+---
+...
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index ee81c7be1..24ad0d1d1 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -342,3 +342,11 @@ rows == expected_rows
lsn == expected_lsn
box.cfg{too_long_threshold = too_long_threshold}
s:drop()
+
+-- Commit after read_only = true (gh-4016).
+s = box.schema.space.create('test')
+_ = s:create_index('pk')
+box.begin() s:replace({1}) box.cfg{read_only = true} box.commit()
+box.rollback()
+box.cfg{read_only = false}
+s:drop()
--
2.21.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [tarantool-patches] [PATCH] Do not enable commit if read_only = true
2019-02-27 7:36 [tarantool-patches] [PATCH] Do not enable commit if read_only = true Georgy Kirichenko
@ 2019-02-27 9:02 ` Vladimir Davydov
2019-02-28 10:25 ` [tarantool-patches] " Konstantin Osipov
0 siblings, 1 reply; 5+ messages in thread
From: Vladimir Davydov @ 2019-02-27 9:02 UTC (permalink / raw)
To: Georgy Kirichenko; +Cc: tarantool-patches
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;
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH] Do not enable commit if read_only = true
2019-02-27 9:02 ` Vladimir Davydov
@ 2019-02-28 10:25 ` Konstantin Osipov
2019-03-03 20:49 ` Георгий Кириченко
0 siblings, 1 reply; 5+ messages in thread
From: Konstantin Osipov @ 2019-02-28 10:25 UTC (permalink / raw)
To: tarantool-patches; +Cc: Georgy Kirichenko
* Vladimir Davydov <vdavydov.dev@gmail.com> [19/02/27 12:26]:
> 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.
Having thought about it we should go over all in-flight
transactions in vinyl and abort them.
We already use this approach on DDL.
We could make it more general.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH] Do not enable commit if read_only = true
2019-02-28 10:25 ` [tarantool-patches] " Konstantin Osipov
@ 2019-03-03 20:49 ` Георгий Кириченко
2019-03-04 9:41 ` Konstantin Osipov
0 siblings, 1 reply; 5+ messages in thread
From: Георгий Кириченко @ 2019-03-03 20:49 UTC (permalink / raw)
To: tarantool-patches; +Cc: Konstantin Osipov
[-- Attachment #1: Type: text/plain, Size: 923 bytes --]
On Thursday, February 28, 2019 1:25:30 PM MSK Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/02/27 12:26]:
> > 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.
>
> Having thought about it we should go over all in-flight
> transactions in vinyl and abort them.
>
> We already use this approach on DDL.
> We could make it more general.
I did a little investigation and found that current implementation does not
allow us do reach expected behavior using vinyl aborts because of
* transaction placed in the write set only after uniqueness check - this leads
to a race if read only was set during this check.
* it is valid only for vy_update and vy_upsert invocations.
* ddl could stuck at schema_latch and continues after read_only was set.
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH] Do not enable commit if read_only = true
2019-03-03 20:49 ` Георгий Кириченко
@ 2019-03-04 9:41 ` Konstantin Osipov
0 siblings, 0 replies; 5+ messages in thread
From: Konstantin Osipov @ 2019-03-04 9:41 UTC (permalink / raw)
To: Георгий
Кириченко
Cc: tarantool-patches
* Георгий Кириченко <georgy@tarantool.org> [19/03/03 23:50]:
> On Thursday, February 28, 2019 1:25:30 PM MSK Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/02/27 12:26]:
> > > 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.
> >
> > Having thought about it we should go over all in-flight
> > transactions in vinyl and abort them.
> >
> > We already use this approach on DDL.
> > We could make it more general.
> I did a little investigation and found that current implementation does not
> allow us do reach expected behavior using vinyl aborts because of
> * transaction placed in the write set only after uniqueness
> check - this leads
> to a race if read only was set during this check.
tx_manager_abort_writers() looks for a single lsm only anyway. In
case of read-only you need to abort all write transactions
against all spaces.
You have two broad options with this problem: fix the existing
infrastructure or begin building a new one.
Adding an extra check to txn_commit() is neither. You will have
two checks for read_only, but your ddl will continue to be broken.
For example, the problem you mention about vinyl write
transactions being added to tx_manager->writers after yield also
affects vinyl ddl. By adding an extra read_only check to
txn_commit() you won't fix it.
So I think we should first consider fixing tx_manager->writers list, so
that transactions end up in this list before yield, and
implementing vy_tx_about_writers() for all write transactions. I
asked Vova to look into this since this is vinyl domain and a bit
tricky one.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-04 9:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-27 7:36 [tarantool-patches] [PATCH] Do not enable commit if read_only = true Georgy Kirichenko
2019-02-27 9:02 ` Vladimir Davydov
2019-02-28 10:25 ` [tarantool-patches] " Konstantin Osipov
2019-03-03 20:49 ` Георгий Кириченко
2019-03-04 9:41 ` Konstantin Osipov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox