Skip to content
New issue

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

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

Already on GitHub? # to your account

MDB improvements #1421

Merged
merged 1 commit into from
Jan 16, 2019
Merged

MDB improvements #1421

merged 1 commit into from
Jan 16, 2019

Conversation

argakiig
Copy link
Contributor

@argakiig argakiig commented Dec 2, 2018

MDB_NOSYNC could provide improvements with disk usage by not calling fsync after commit
MDB_NORDAHEAD improves memory usage by not trying to read ahead in the DB, This has no effect on windows but other OSes should benefit. Windows does not read ahead so it is not affected

@SergiySW
Copy link
Contributor

SergiySW commented Dec 2, 2018

MDB_NOSYNC Don't flush system buffers to disk when committing a transaction. This optimization means a system crash can corrupt the database or lose the last transactions if buffers are not yet flushed to disk. The risk is governed by how often the system flushes dirty buffers to disk and how often mdb_env_sync() is called. However, if the filesystem preserves write order and the MDB_WRITEMAP flag is not used, transactions exhibit ACI (atomicity, consistency, isolation) properties and only lose D (durability). I.e. database integrity is maintained, but a system crash may undo the final transactions. Note that (MDB_NOSYNC | MDB_WRITEMAP) leaves the system with no hint for when to write transactions to disk, unless mdb_env_sync() is called. (MDB_MAPASYNC | MDB_WRITEMAP) may be preferable. This flag may be changed at any time using mdb_env_set_flags().
http://www.lmdb.tech/doc/group__mdb.html

@cryptocode
Copy link
Contributor

cryptocode commented Dec 2, 2018

Yeah I think this would need to be a config flag since it can more easily cause corruption. A pattern I've seen is "fast flags", e.g. MDB_NOSYNC is fast and MDB_NOSYNC | MDB_WRITEMAP | MDB_MAPASYNC is fastest, but the default should be neither.

@argakiig
Copy link
Contributor Author

argakiig commented Dec 2, 2018

if MDB_NOSYNC is enabled without MDB_WRITEMAP you still maintain the ACI, at the cost of durability. the risk is you lose some final transactions and have to bootstrap them later. How is this different from the node being offline for extended periods.

@cryptocode
Copy link
Contributor

cryptocode commented Dec 2, 2018

MDB_NOSYNC can corrupt the database on system crashes/outage. That's relatively low risk and may be a fine tradeoff for some nodes, but I do think it should be a config option.

@argakiig
Copy link
Contributor Author

argakiig commented Dec 2, 2018

ah, I see, let me see about getting a config flag option, up

@cryptocode
Copy link
Contributor

cryptocode commented Dec 2, 2018

@argakiig seems like you're right about not using MDB_WRITEMAP still maintaining ACI according to docs, a config flag would be nice though.

@clemahieu
Copy link
Contributor

I think the first order of business is to figure out if there's any performance gain from these flags, if not we should document it and not make a change.

@rkeene rkeene added enhancement experiment This item indicates that the change is an experiment and is not fully baked labels Dec 5, 2018
@zhyatt zhyatt added this to the Research for Future Release milestone Jan 10, 2019
@argakiig
Copy link
Contributor Author

Will create a separate PR for testing MDB_NOSYNC/MDB_WRITEMAP with config options This is now just for MDB_NORDAHEAD which, when supported, will stop the DB from being loaded

@rkeene rkeene merged commit 572cf5c into nanocurrency:master Jan 16, 2019
@argakiig argakiig deleted the MDB_improvements branch January 16, 2019 16:13
rkeene pushed a commit that referenced this pull request Jan 17, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement experiment This item indicates that the change is an experiment and is not fully baked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants