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.

Sign up for GitHub

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

Jump to bottom

txn: run statement on_rollback triggers before rolling back statement #9936

Merged
merged 2 commits into from
May 29, 2024
Merged

txn: run statement on_rollback triggers before rolling back statement #9936

merged 2 commits into from
May 29, 2024

Conversation

CuriousGeorgiy
Copy link
Member

@CuriousGeorgiy CuriousGeorgiy commented Apr 11, 2024 •
edited

This patch fixes the order of running a transaction statement's on_rollback triggers and rolling back the statement itself.

Closes #9893

@CuriousGeorgiy CuriousGeorgiy requested a review from a team as a code owner April 11, 2024 10:33
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-9893-rollback-ddl-on-_space-space branch 2 times, most recently from 0779ee6 to 0ed3fe6 Compare April 11, 2024 12:00
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-9893-rollback-ddl-on-_space-space branch 4 times, most recently from 6986041 to 11725c6 Compare April 11, 2024 14:15
drewdzzz
drewdzzz reviewed Apr 11, 2024
View reviewed changes
Copy link
Contributor

@drewdzzz drewdzzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an opportunity to alter _space when Tarantool is running?
It seems that this case is really tricky, so I'd prefer to forbid _space:alter.

@drewdzzz drewdzzz assigned CuriousGeorgiy and unassigned drewdzzz Apr 11, 2024
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-9893-rollback-ddl-on-_space-space branch 3 times, most recently from 6946a0e to 71316f4 Compare April 12, 2024 09:58
@coveralls
Copy link

coveralls commented Apr 12, 2024 •
edited

Coverage Status

coverage: 87.102% (+0.01%) from 87.088%
when pulling af153d4 on CuriousGeorgiy:gh-9893-rollback-ddl-on-_space-space
into 8de2296
on tarantool:master.

@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-9893-rollback-ddl-on-_space-space branch 4 times, most recently from e71b773 to 0870e08 Compare April 22, 2024 13:19
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-9893-rollback-ddl-on-_space-space branch from 0870e08 to 73b94b1 Compare April 22, 2024 13:19
drewdzzz
drewdzzz reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@drewdzzz drewdzzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that we need to add priv_reload_info to txn_stmt_rollback_info.
As I understand, we assume that space _priv is small, so we could save all tuples in on_replace, insert the new tuple, if any, and delete the old one, if any - then we could pass priv_reload_info as on_rollback->data to the trigger.

@drewdzzz drewdzzz assigned CuriousGeorgiy and unassigned drewdzzz May 8, 2024
@drewdzzz
Copy link
Contributor

drewdzzz commented May 8, 2024

You can allocate priv_reload_info with region_alloc(&txn->region) - it's cheap so it's not a problem.

@CuriousGeorgiy
Copy link
Member Author

CuriousGeorgiy commented May 13, 2024 •
edited

@drewdzzz The solution you suggest requires manually patching the port contents to restore the old space state, i.e., removing the new tuple, if any, and restoring the old tuple, if any.

Since ports are append-only structures, supporting this operation requires hacking around them and adding a lot of new one-time-use code (for insertion and removal of port entries into the middle of the port entry list) compared to the current solution.

I would like a second opinion to opt for your solution.

@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-9893-rollback-ddl-on-_space-space branch 3 times, most recently from 6a49aec to b3156d3 Compare May 15, 2024 10:16
@CuriousGeorgiy
Copy link
Member Author

CuriousGeorgiy commented May 15, 2024 •
edited

@drewdzzz @sergepetrenko I implemented the solution that @sergepetrenko suggested, please take a look. I guess #10004 is not worth taking a look then if we all agree on this solution, so I will just close it.

drewdzzz
drewdzzz approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@drewdzzz drewdzzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch, LGTM, please address my comment.

src/box/user.h Outdated Show resolved Hide resolved
@drewdzzz drewdzzz removed their assignment May 15, 2024
drewdzzz
drewdzzz reviewed May 15, 2024
View reviewed changes
src/box/user.cc Outdated Show resolved Hide resolved
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-9893-rollback-ddl-on-_space-space branch 4 times, most recently from 63f5a1a to 24e8fcc Compare May 17, 2024 09:31
sergepetrenko
sergepetrenko requested changes May 20, 2024
View reviewed changes
Copy link
Collaborator

@sergepetrenko sergepetrenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes!
I like this version much more. Please, handle the issues with the test and we'll be good to go.

test/box-luatest/rollback_ddl_on__priv_space_test.lua Outdated Show resolved Hide resolved
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-9893-rollback-ddl-on-_space-space branch from 24e8fcc to 22c2082 Compare May 27, 2024 09:18
@CuriousGeorgiy
box: pass statement being rolled back (if any) to priv_grant
e675d34 
In scope of tarantool#9893 we are going to run statement `on_rollback` triggers
before rolling back the corresponding statement. During rollback of DDL in
the `_priv` space, the database is accessed from `user_reload_privs` to
reload user privileges, so we need it to account for the current statement
being rolled back: i.e., the new tuple that was introduced (if any) must
not be used, while the old tuple (if any) must be used.

Needed for tarantool#9893

NO_CHANGELOG=<refactoring>
NO_DOC=<refactoring>
@CuriousGeorgiy
txn: run statement on_rollback triggers before rolling back statement
af153d4 
Logically, we call triggers after running statements. These triggers can
make significant changes (for instance, DDL triggers), so, for consistency,
we should call the statement's `on_rollback` triggers before rolling back
the statement. This also adheres to the logic that transaction
`on_rollback` triggers are called before rolling back individual
transaction statements.

One particular bug that this patch fixes is rolling back of DDL on the
`_space` space. DDL is essentially a replace operation on the `_space`
space, which also invokes the `on_replace_dd_space` trigger. In this
trigger, among other things, we swap the indexes of the original space,
`alter->old_space`, which is equal to the corresponding transaction
`stmt->space`, with the indexes of the newly created space,
`alter->new_space`:
https://github.com/tarantool/tarantool/blob/de80e0264f7deb58ea86ef85b37b92653a803430/src/box/alter.cc#L1036-L1047

If then a rollback happens, we first rollback the replace operation, using
`stmt->space`, and only after that do we swap back the indexes in
`alter_space_rollback`:
https://github.com/tarantool/tarantool/blob/de80e0264f7deb58ea86ef85b37b92653a803430/src/box/memtx_engine.cc#L659-L669
https://github.com/tarantool/tarantool/blob/de80e0264f7deb58ea86ef85b37b92653a803430/src/box/alter.cc#L916-L925

For DDL on the _space space, the replace operation and DDL occur on the
same space. This means that during rollback of the replace, we will try to
do a replace in the empty indexes that were created for `alter->new_space`.
Not only does this break the replace operation, but also the newly inserted
tuple, which remains in the index, gets deleted, and access to it causes
undefined behavior (heap-use-after-free).

As part of the work on this patch, tests of rollback of DDL on system
spaces which use `on_rollback` triggers were enumerated:
* `_sequence` — box/sequence.test.lua;
* `_sequence_data` — box/sequence.test.lua;
* `_space_sequence` — box/sequence.test.lua;
* `_trigger` — sql/ddl.test.lua, sql/errinj.test.lua;
* `_collation` — engine-luatest/gh_4544_collation_drop_test.lua,
                 box/ddl_collation.test.lua;
* `_space` — box/transaction.test.lua, sql/ddl.test.lua;
* `_index` — box/transaction.test.lua, sql/ddl.test.lua;
* `_cluster` — box/transaction.test.lua;
* `_func` — box/transaction.test.lua, box/function1.test.lua;
* `_priv` — box/errinj.test.lua,
            box-luatest/rollback_ddl_on__priv_space_test.lua;
* `_user` — box/transaction.test.lua,
            box-luatest/gh_4348_transactional_ddl_test.lua.

Closes tarantool#9893

NO_DOC=<bugfix>
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-9893-rollback-ddl-on-_space-space branch from 22c2082 to af153d4 Compare May 27, 2024 09:34
sergepetrenko
sergepetrenko approved these changes May 29, 2024
View reviewed changes
Copy link
Collaborator

@sergepetrenko sergepetrenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes!
LGTM.

@sergepetrenko sergepetrenko added the full-ci Enables all tests for a pull request label May 29, 2024
@sergepetrenko sergepetrenko merged commit d529082 into tarantool:master May 29, 2024
81 checks passed
@sergepetrenko
Copy link
Collaborator

Backports:
3.1#10065
2.11 #10066

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drewdzzz drewdzzz drewdzzz approved these changes

@sergepetrenko sergepetrenko sergepetrenko approved these changes

@p7nov p7nov p7nov approved these changes

Assignees

@sergepetrenko sergepetrenko

Labels
full-ci Enables all tests for a pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rollback of DDL on _space space is completely broken
5 participants
@CuriousGeorgiy @coveralls @drewdzzz @sergepetrenko @p7nov