Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

memtx: make stable DDL operations in mvcc mode #6138

Open
Korablev77 opened this issue Jun 9, 2021 · 1 comment
Open

memtx: make stable DDL operations in mvcc mode #6138

Korablev77 opened this issue Jun 9, 2021 · 1 comment
Labels
bug Something isn't working memtx mvcc

Comments

@Korablev77
Copy link
Contributor

Now it is technically possible to execute DDL requests in MVCC mode. Nevertheless TX manager hasn't been adapted to DDL yet. As a result we can observe many bugs involving DDL operations: #5998 #6137 #6007

One of the problems is that caches for metadata structures (like space_cache, user_cache, func_cache) are not versioned. Possible solution is to keep lists of corresponding structures with the TX id for each unique entry in the cache. It is likely to be enough for the most structures (user, func, coll).

More examples:

-- Can't create spaces with the same name in separate transactions, meanwhile
-- drop doesn't find the space by name.
--
box.cfg{memtx_use_mvcc_engine = true}

txn_proxy = require('txn_proxy')

tx1 = txn_proxy.new()
tx2 = txn_proxy.new()

tx1:begin()
tx2:begin()

tx1("box.schema.create_space('t')")
tx2("box.schema.create_space('t')")
-- 'Space ''t'' already exists'

tx1:begin()
tx2:begin()


tx1("box.schema.create_space('t1')")
tx2("box.space.t1:drop()")
-- 'Space ''t1'' does not exist'
@Korablev77 Korablev77 added bug Something isn't working memtx mvcc labels Jun 9, 2021
@Korablev77 Korablev77 self-assigned this Jun 10, 2021
Korablev77 added a commit that referenced this issue Jun 14, 2021
Korablev77 added a commit that referenced this issue Jun 22, 2021
There are a few issues concerning TX manager and DDL operations.

The thing is metadata stored for any object is not currently designed to
be versioned. It means that several transactions can see changes
provided by each other TX BEFORE they are committed. Which is considered
to be a disaster and violates all TXM principles.

Regarding users in particular, there's quite simple test case leading to
the crash. Imagine two transaction creating a user with the same ID at the
same moment:

tx1:begin()
tx2:begin()

tx1("box.schema.user.create('internal1')")
tx2("box.schema.user.create('internal2')")

First operation creates and adds user with ID1 to user cache; second
operation finds user by the same ID1 and attempts to update it (to be
more precise - its name). We get the same ID since ID for user is
generated based on the last value appearing in the space; before
transaction is committed the last value for both transactions is the
same.

Unfortunately on_rollback trigger supposes that old_tuple is not null.
Meanwhile it is: until first TX commits its changes, old_tuple does not
exist.

Part of #6138
Closes #5998
Korablev77 added a commit that referenced this issue Jun 23, 2021
There are a few issues concerning TX manager and DDL operations.

The thing is metadata stored for any object is not currently designed to
be versioned. It means that several transactions can see changes
provided by each other BEFORE they are committed. Which is considered
to be a disaster and violates all transaction processing principles
(speaking of serializable isolation level).

Regarding users in particular, there's quite simple test case leading to
the crash. Imagine two transaction creating a user with the same ID at the
same moment:

tx1:begin()
tx2:begin()

tx1("box.schema.user.create('internal1')")
tx2("box.schema.user.create('internal2')")

First operation creates and adds user with ID_1 to user cache; second
operation finds user by the same ID_1 and attempts to update it (to be
more precise - its name). We get the same ID since ID for user is
generated based on the last value appearing in the space; before
transaction is committed the last value for both transactions is the
same.

In on_replace_dd_user() trigger we assume that the operation is
update/replace if we find user in the cache by its id (in contrast to
other on_replace DDL triggers where we look at presence of old_tuple).
Unfortunately on_rollback trigger supposes that old_tuple is not null.
Meanwhile it is: until first TX commits its changes, old_tuple does not
exist.

In scope of this patch we are going to fix several problems at once.

Firstly, let's add to the struct user id of the transaction that
attempts at modifying it. It is assigned after change in
on_replace_dd_user() and reset to zero in on_commit/on_rollback triggers.
If any other transaction (with different id) wants to acquire the same
user - we raise an error saying that user is already busy. To deal with
the situation when TX deletes user, we also add is_deleted flag which
indicates that user has been deleted but transaction is still not
committed.

Secondly, let's rework on_rollback and introduce on_commit triggers of
on_replace_dd_user(). Now they accept struct user or user_def and reset
tx id of user.

Obviously, that patch doesn't fix all the problems. In fact, users have
quite sophisticated implementation. They are stored in two places: as an
static array and in cache. Furthermore, we maintain dependency graph for
roles and RB-Tree of privileges for each user. That makes tracking of
privilege updates complicated. So that we don't touch this part in scope
of current patch.

Part of #6138
Closes #5998
@Korablev77
Copy link
Contributor Author

Here's example from #5515:

fiber = require('fiber')
channel1 = fiber.channel()
channel2 = fiber.channel()
function test1()                                 \
    box.begin()                                  \
    local s = box.schema.space.create('test')    \
    s:create_index('pk')                         \
    channel1:put(1)                              \
    local ok = channel2:get()                    \
    box.session.push(ok)                         \
    box.rollback()                               \
    channel1:put(1)                              \
end
f = fiber.new(test1)
channel1:get()
tx1('box.space.test:select()')
tx1('box.space.test:replace({1, 2, 3})')

tx1 sees space created in test1() meanwhile it shouldn't. In case we prohibit fiber yields between end of alter and TX preparation, this example will become impossible.

Korablev77 added a commit that referenced this issue Jul 15, 2021
Korablev77 added a commit that referenced this issue Jul 16, 2021
To avoid sharing (ergo phantom reads) for different transaction in MVCC
mode, let's do following things. Firstly, after modification of any
shared objects (like space/user/function caches) disallow transaction to
yield to prevent other transactions see that changes. Yield is permitted
til the transaction is committed. Secondly, on committing transaction
that provides DDL changes let's abort all other transaction since they
may refer to obsolete schema. The last restriction may seem too strict,
but it is OK as primitive workaround until transactional DDL is
introduced.

Closes #5998
Closes #6140
Workaround for #6138
Korablev77 added a commit that referenced this issue Jul 19, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things. Firstly, after
modification of any shared objects (like space/user/function caches)
disallow transaction to yield in order to prevent other transactions see
that changes. Yield is permitted til the transaction is committed. In
case yield occurs, transaction is aborted. Secondly, on committing
transaction that provides DDL changes let's abort all other transaction
since they may refer to obsolete schema objects. The last restriction
may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort
transactions that have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
Korablev77 added a commit that referenced this issue Jul 27, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things. Firstly, after
modification of any shared objects (like space/user/function caches)
disallow transaction to yield in order to prevent other transactions see
that changes. Yield is permitted til the transaction is committed. In
case yield occurs, transaction is aborted. Secondly, on committing
transaction that provides DDL changes let's abort all other transaction
since they may refer to obsolete schema objects. The last restriction
may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort
transactions that have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
Korablev77 added a commit that referenced this issue Jul 29, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things. Firstly, after
modification of any shared objects (like space/user/function caches)
disallow transaction to yield in order to prevent other transactions see
that changes. Yield is permitted til the transaction is committed. In
case yield occurs, transaction is aborted. Secondly, on committing
transaction that provides DDL changes let's abort all other transaction
since they may refer to obsolete schema objects. The last restriction
may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort
transactions that have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
alyapunov pushed a commit that referenced this issue Aug 2, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things. Firstly, after
modification of any shared objects (like space/user/function caches)
disallow transaction to yield in order to prevent other transactions see
that changes. Yield is permitted til the transaction is committed. In
case yield occurs, transaction is aborted. Secondly, on committing
transaction that provides DDL changes let's abort all other transaction
since they may refer to obsolete schema objects. The last restriction
may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort
transactions that have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
Korablev77 added a commit that referenced this issue Aug 2, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things. Firstly, after
modification of any shared objects (like space/user/function caches)
disallow transaction to yield in order to prevent other transactions see
that changes. Yield is permitted til the transaction is committed. In
case yield occurs, transaction is aborted. Secondly, on committing
transaction that provides DDL changes let's abort all other transaction
since they may refer to obsolete schema objects. The last restriction
may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort
transactions that have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
Korablev77 added a commit that referenced this issue Aug 4, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
alyapunov pushed a commit that referenced this issue Aug 10, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
alyapunov pushed a commit that referenced this issue Aug 12, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
alyapunov pushed a commit that referenced this issue Aug 13, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
alyapunov pushed a commit that referenced this issue Aug 13, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
EvgenyMekhanik pushed a commit that referenced this issue Aug 13, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
alyapunov pushed a commit that referenced this issue Aug 13, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
Korablev77 added a commit that referenced this issue Aug 14, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
Korablev77 added a commit that referenced this issue Aug 16, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138
Korablev77 added a commit that referenced this issue Aug 16, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138

(cherry picked from commit 8f4be32)
Korablev77 added a commit that referenced this issue Aug 16, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138

(cherry picked from commit 8f4be32)
Korablev77 added a commit that referenced this issue Aug 18, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138

(cherry picked from commit 8f4be32)
Korablev77 added a commit that referenced this issue Aug 18, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138

(cherry picked from commit 8f4be32)
Korablev77 added a commit that referenced this issue Aug 18, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138

(cherry picked from commit 8f4be32)
Korablev77 added a commit that referenced this issue Aug 18, 2021
To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138

(cherry picked from commit 8f4be32)
@kyukhin kyukhin added this to the wishlist milestone Aug 19, 2021
@alyapunov alyapunov added the 13sp label Oct 25, 2021
@kyukhin kyukhin removed this from the wishlist milestone Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working memtx mvcc
Projects
None yet
Development

No branches or pull requests

4 participants