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

fix a bug, c api, if enable inplace_update_support, and use create sn… #9471

Closed
wants to merge 2 commits into from
Closed

fix a bug, c api, if enable inplace_update_support, and use create sn… #9471

wants to merge 2 commits into from

Conversation

shenxingwuying
Copy link

c api release snapshot will core dump when enable inplace_update_support and create snapshot

@riversand963 riversand963 self-requested a review February 5, 2022 01:32
riversand963
riversand963 suggested changes Feb 9, 2022
View reviewed changes
Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

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

Thanks @shenxingwuying for the fix.
Once ready to merge, please add an entry to "Behavior changes" in HISTORY.md.
The PR description will be the final commit message. A clear and concise description will be appreciated.

db/c.cc Outdated Show resolved Hide resolved
db/c.cc Outdated Show resolved Hide resolved
riversand963
riversand963 approved these changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

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

Thanks @shenxingwuying for the PR.
Mostly LGTM with a few minor comments.

db/db_inplace_update_test.cc Outdated Show resolved Hide resolved
db/db_impl/db_impl.cc Outdated Show resolved Hide resolved
db/c_test.c Outdated Show resolved Hide resolved
db/c_test.c Outdated Show resolved Hide resolved
@riversand963
Copy link
Contributor

btw, can you rebase to latest main so that the CIs can run?

@shenxingwuying shenxingwuying force-pushed the dev/inplace_and_release_snapshot branch from 79eee27 to fdc2ff5 Compare March 3, 2022 15:39
@shenxingwuying shenxingwuying force-pushed the dev/inplace_and_release_snapshot branch from fdc2ff5 to 00daaaf Compare March 17, 2022 15:33
@facebook-github-bot
Copy link
Contributor

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

duyuqi added 2 commits March 21, 2022 22:01
@shenxingwuying
fix a bug, c api, if enable inplace_update_support, and use create sn…
2ca9888 
…apshot, release it will core
@shenxingwuying
Fix a bug causing rocksdb core dump
1952ae7 
Summary:
This bug meets two conditions:
- the option inplace_update_support is enabled
- users use Snapshot

Because get snapshot will be nullptr,
and then release a nullptr snapshot
will core dump.
@shenxingwuying shenxingwuying force-pushed the dev/inplace_and_release_snapshot branch from 00daaaf to 1952ae7 Compare March 21, 2022 14:02
@facebook-github-bot
Copy link
Contributor

@shenxingwuying has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ajkr pushed a commit that referenced this pull request Mar 29, 2022
@ajkr
fix a bug, c api, if enable inplace_update_support, and use create sn… (
6a385e9 
#9471)

Summary:
c api release snapshot will core dump when enable inplace_update_support and create snapshot

Pull Request resolved: #9471

Reviewed By: akankshamahajan15

Differential Revision: D34965103

Pulled By: riversand963

fbshipit-source-id: c3aeeb9ea7126c2eda1466102794fecf57b6ab77
ajkr pushed a commit that referenced this pull request Mar 29, 2022
@ajkr
fix a bug, c api, if enable inplace_update_support, and use create sn… (
fdd0e3a 
#9471)

Summary:
c api release snapshot will core dump when enable inplace_update_support and create snapshot

Pull Request resolved: #9471

Reviewed By: akankshamahajan15

Differential Revision: D34965103

Pulled By: riversand963

fbshipit-source-id: c3aeeb9ea7126c2eda1466102794fecf57b6ab77
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@riversand963 riversand963 Awaiting requested review from riversand963

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shenxingwuying @riversand963 @facebook-github-bot