8000 Bug-fix-3.8/make iterator usage safer after intermediate commits by mpoeter · Pull Request #14729 · arangodb/arangodb · GitHub
[go: up one dir, main page]
Skip to content

Bug-fix-3.8/make iterator usage safer after intermediate commits #14729

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
Merged
merged 21 commits into from
Sep 14, 2021
Merged

Bug-fix-3.8/make iterator usage safer after intermediate commits #14729

merged 21 commits into from
Sep 14, 2021

Conversation

mpoeter
Copy link
Contributor
@mpoeter mpoeter commented Sep 3, 2021 •
edited
Loading

Scope & Purpose

Backport of #14543 and #14346 (which builds upon the former).

Enterprise companion PR: https://github.com/arangodb/enterprise/pull/752

  • Fix internal iterator states after intermediate commits in write transactions. Iterators could point to invalid data after an intermediate commit, producing undefined behavior.

  • Fix read-own-write behavior in different scenarios:

    • in some cases writes performed by an AQL query could be observed within the same query. This was not intended and is fixed now.
    • AQL queries in streaming transactions could observe their own writes in even more cases, which could potentially result in an endless loop when the query iterates over the same collection that it is inserting documents into.
    • UPSERT did not find documents inserted by a previous iteration if the subquery relied on a non-unique secondary index.
    • disabled intermediate commits for queries with UPSERTs, because intermediate commits can invalidate the internal read-own-write iterator required by UPSERT. Previously, UPSERTs that triggered intermediate commits could have produced unexpected results (e.g., previous inserts that have been committed might not be visible) or even crashes.

To achieve the correct read-own-write behavior in streaming transactions, we sometimes have to copy the internal WriteBatch from the underlying RocksDB transaction. In particular, the copy is created whenever an AQL query with
modification operations (INSERT/REMOVE/UPDATE/UPSERT/REPLACE) is executed in the streaming transaction. If there have not been any other modifications so far (queries/document operations), then the WriteBatch is empty and creating the copy is essentially a no-op. However, if the transaction already contains a lot of modifications, creating the WriteBatch copy might incur some overhead that can now lead to decreased performance.

  • 💩 Bugfix (requires CHANGELOG entry)
  • 🔨 Refactoring/simplification
  • 📖 CHANGELOG entry made

Testing & Verification

  • This PR adds tests that were used to verify all changes:
    • Added new integration tests in shell_client

@mpoeter mpoeter added this to the 3.8 milestone Sep 3, 2021
@jsteemann jsteemann mentioned this pull request Sep 10, 2021
Merged
8 tasks
@mpoeter mpoeter marked this pull request as ready for review September 12, 2021 14:41
jsteemann
jsteemann reviewed Sep 13, 2021
View reviewed changes
jsteemann
jsteemann reviewed Sep 13, 2021
View reviewed changes
jsteemann
jsteemann suggested changes Sep 13, 2021
View reviewed changes
Copy link
Contributor
@jsteemann jsteemann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All good except some changes in EngineInfoContainerDBServerBased.cpp that should all be reverted in this PR.

jsteemann
jsteemann approved these changes Sep 13, 2021
View reviewed changes
Copy link
Contributor
@jsteemann jsteemann left a comment

Choose a reason for hiding this comment

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

LGTM!

@KVS85
Copy link
Contributor
KVS85 commented Sep 14, 2021

Go tests seem to be unreliable atm.

@KVS85 KVS85 merged commit e028d82 into 3.8 Sep 14, 2021
@KVS85 KVS85 deleted the bug-fix-3.8/safe-iterators-after-intermediate-commits2 branch September 14, 2021 07:04
neunhoef
neunhoef reviewed Sep 14, 2021
View reviewed changes
Copy link
Member
@neunhoef neunhoef left a comment

Choose a reason for hiding this comment

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

LGTM

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

@neunhoef neunhoef neunhoef left review comments

@jsteemann jsteemann jsteemann approved these changes

Assignees

@mpoeter mpoeter

Labels
None yet
Projects
None yet
Milestone
3.8
Development

Successfully merging this pull request may close these issues.
5 participants
@mpoeter @KVS85 @jsteemann @neunhoef @goedderz
0