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

Improve database consistency #15677

Open
appgurueu opened this issue Jan 13, 2025 · 6 comments
Open

Improve database consistency #15677

appgurueu opened this issue Jan 13, 2025 · 6 comments
Labels
Request / Suggestion The issue makes a suggestion for something that should be done but isn't a new feature @ Startup / Config / Util

Comments

@appgurueu
Copy link
Contributor

Our database consistency is bad currently. In case of a hard crash, the different databases can easily become inconsistent. The root of the problem here is that everything lives in a separate database - auth, players, mod storage, map - and sometimes even differs in save logic.

The consequences from this range from duplication bugs (player inventory is already persisted; node inventory is not, server is made to crash) to problems with rideable creatures (#15605) etc.

For consistency, I believe it would be ideal if everything used a single database such that we can properly use database consistency features (transactions) to ensure that either all the changes during a map save interval or none of them are saved.

Of course, there are practical problems with this since we want to support multiple, different backends, but given that by default everything uses SQLite, we could make it work at least for this case.

(Additionally, we should encourage modders to avoid storing data in separate files: They should use mod storage for consistency instead.)

@appgurueu appgurueu added the Request / Suggestion The issue makes a suggestion for something that should be done but isn't a new feature label Jan 13, 2025
@sfan5
Copy link
Collaborator

sfan5 commented Jan 13, 2025

problems with rideable creatures (#15605) etc.

not a good example as that "bug" only involves a single database (the map)

@appgurueu
Copy link
Contributor Author

not a good example as that "bug" only involves a single database (the map)

Does it? Player positions are stored in the player database, entity positions are stored in the map. These can't be updated in the same transaction.

@sfan5
Copy link
Collaborator

sfan5 commented Jan 13, 2025

Does it? Player positions are stored in the player database, entity positions are stored in the map. These can't be updated in the same transaction.

Yes. This is just an edge case of re-save decision logic for entities. Not a consistency issue or race condition.

@lhofhansl
Copy link
Contributor

Unless you write everything in a single transaction you'd have the same problem, no?

@appgurueu
Copy link
Contributor Author

Well, it probably wasn't the best idea to subsum save logic consistency and database consistency problems in this issue.

I think what sfan is saying is that the entity issue is currently so bad primarily because the re-save logic differs.

I bet we have more inconsistent logic since our current architecture doesn't discourage it. Forcing everything into a single transaction would pretty much force consistency.

The issue can't technically be fully fixed without doing everything in the same transaction, but it would probably be far less likely, since you'd need Luanti to be killed while one transaction is in progress and another has already completed, which is unlikely since most of the time the databases should be between save intervals.

@sfan5
Copy link
Collaborator

sfan5 commented Jan 16, 2025

The issue in the opening text is real, I am just here to disagree with one of the examples given.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Request / Suggestion The issue makes a suggestion for something that should be done but isn't a new feature @ Startup / Config / Util
Projects
None yet
Development

No branches or pull requests

4 participants