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

WT-2985 checkpoint core dump #3100

Merged
merged 7 commits into from
Oct 20, 2016
Merged

Conversation

keithbostic
Copy link
Contributor

No description provided.

If checkpoint skips writing a leaf page that's never been written before,
reconciliation must ignore it when reviewing an internal page's leaf pages.
it's relatively expensive to dirty a page.
@keithbostic keithbostic self-assigned this Oct 19, 2016
This reverts commit d986497.
that don't have disk addresses in checkpoints is that some of the blocks
may have been written, and the checkpoint can leak blocks. We could
probably fix it in the block manager layer, but that prevents
checkpoints from mapping one-to-one with the blocks in the file, and
that's scary.
@michaelcahill
Copy link
Contributor

@keithbostic, sorry for the radio silence -- I've had back to back meetings for several days.

The latest version of this is what I was expecting: checkpoint should visit these pages, I think. That test is intended to skip simple pages that were clean at the beginning of the checkpoint (or on disk) and become dirty while the checkpoint is running. It should not be a common enough case for pages to be this state after a split has failed that there's any measurable performance penalty for visiting them.

Copy link
Contributor

@michaelcahill michaelcahill left a comment

Choose a reason for hiding this comment

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

This LGTM, the code shuffling is a style preference on my part: feel free to ignore it.

* clean and we can't skip future checkpoints until this page is
* written.
*/
__wt_tree_modify_set(session);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind pull this up into the caller so the inlined function has no side effects?

Copy link
Contributor Author

@keithbostic keithbostic Oct 20, 2016

Choose a reason for hiding this comment

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

Yes, you're right, that's better, done & pushed.

@keithbostic
Copy link
Contributor Author

@michaelcahill, this last set of changes is passing all of my tests, so if you're happy with it, it's ready to merge.

The reason I resisted the second approach was I don't like the checkpoint sync code looking inside the page modify structure, that's relatively ugly (not that the additional code in reconciliation was all that pretty, but at least it was in reconciliation).

Anyway, the only other idea I had was to add a new "multiblock with failed addresses" flag to the page modify structure, and that's not much better.

underlying skip-page function shouldn't have side effects.
@michaelcahill
Copy link
Contributor

Thanks @keithbostic, I'll merge.

@michaelcahill michaelcahill merged commit ceeb57b into develop Oct 20, 2016
@michaelcahill michaelcahill deleted the wt-2985-checkpoint-core-dump branch October 20, 2016 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants