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

Move to PostgreSQL as our primary database engine #276

Open
Beyley opened this issue Nov 16, 2023 · 4 comments
Open

Move to PostgreSQL as our primary database engine #276

Beyley opened this issue Nov 16, 2023 · 4 comments
Assignees
Labels
accepted-proposal This proposal has been accepted, and is awaiting implementation enhancement New feature or request proposal A proposal for a feature that is not a missing feature from the game
Milestone

Comments

@Beyley
Copy link
Member

Beyley commented Nov 16, 2023

Overview

As Refresh grows as a project, we are going to (and have) hit some walls with our current database engine, Realm. This proposal if implemented would replace the primary database engine with PostgreSQL, leaving Realm as a cut-down secondary option for smaller deployments.

Benefits of Realm

Simplicity

Everything being contained in a single process and packed with the executable means would-be server hosters do not need to worry about spinning up a separate database and dealing with hooking the two together (setting up users and permissions can be quite the pain in standard databases). This means Refresh is more accessible and more usable for smaller-scale operations.

Code readability

Realm is the end-game of ORMs, it is extremely simple to use, and makes database operations almost completely native to the language. Meaning theres less glue code for the server code to the database code

Problems with Realm

Scalability

Realm simply cannot scale, without a practical separate-process model, we are unable to fully separate the workload of running the server between multiple machines, if ever necessary in the future.

Stability

With the server running in the same process as the database engine, any error that brings down one is guaranteed to fully bring down the other. While they generally rely on each other, this leads to concerns about data-loss, say the server encounters a fatal error and the process dies, its possible data corruption may occur (although Realm is generally very safe about that).

Performance

As tables scale to >100k rows, realm slows down considerably, with database insertions sometimes hitting >500ms, which is simply unacceptable performance with multiple players uploading levels.
Realm also heavily limits our ability to use async in Refresh code, since if you are not careful, you'll get "object accessed from incorrect thread" errors when a context switch happens in an async function. This has already bitten us with tests.

Maintainability

While Realm code is generally very readable, theres many pitfalls which have made their way into production. One very common one is the limitations on Count/Where. All accesses in methods like those on IQueryable<T> must be on direct fields, which is a foot-gun with enums, which are always properties, due to Realm being unable to directly store enums on a database object.

Benefits of PostgreSQL

Stability

PostgreSQL is industry standard tooling, meaning we can be more trusting about its stability and long-term performance as a Refresh instance scales to hundreds of players. It also has much more mature replication abilities, which is very important for low downtime.

Scalability

PostgreSQL has many more features to aid in scaling, many of which we would be able to take advantage of without greatly refactoring any of the server code. PostgreSQL would not hit the same performance/scalability issues we are experiencing today with Realm.

Problems with PostgreSQL

Complexity

Having to spin up a separate database server can be a lot for simple contributors wanting to dip their toes into the project.
The implementation of using PostgreSQL is also much more complex compared to Realm, given existing ORMs wont be as simple to integrate as Realm currently is.

Solution

We should maintain 2 implementations of GameDatabaseContext, a full complete implementation using PostgreSQL, and a cut-down implementation using Realm.

Benefits

This would give us the best of both worlds, people looking to deploy a small server wont have the burden of a full separate out of process database, and people looking to make something that scales wont have to deal with the issues that Realm brings.

Open questions

Testing

How do we go about testing a PostgreSQL implementation, and how to we make it simple for contributors to test on it locally?

Maintainability

Will a cut down database implementation genuinely be useful to small servers? Or will the fact its limited at all push people away from it outside of testing?

Contributor-friendliness

Will this change be friendly to new code contributors? We want to encourage people to join in and contribute to the project, and any difficult setup process for the project will push away a majority of people looking to dip their toes into the project.
We need to ensure that a more complex database engine does not degrade the experience for new contributors.

Implementation

I propose GameDatabaseContext becomes an abstract class, inheriting ontop of IDatabaseContext, with 2 implementations

  • class RealmGameDatabaseContext : GameDatabaseContext
  • class PostgresGameDatabaseContext : GameDatabaseContext

GameDatabaseContext has any shared constants and implementations, and defines the external interface that server code will interact with, it is what is passed to endpoints.

GameDatabaseProvider will also have to be split into 2 separate classes,

  • class RealmGameDatabaseProvider : RealmDatabaseProvider<RealmGameDatabaseContext>
  • class PostgresGameDatabaseProvider : IDatabaseProvider<PostgresGameDatabaseContext>
    RefreshGameServer will pick which to use based on configuration options.

Put/Update methods which are not supported by RealmGameDatabaseContext should throw a custom NotImplementedByRealmException, this is to allow differentiating with a true NotImplementedException.

This exception would be caught by a custom Middleware and return a 501 Not Implemented error on API endpoints, and a 200 OK on game endpoints.

On RealmGameDatabaseContext all Get methods should always succeed, returning no data/not found where appropriate.

The main Refresh server code should not need to be aware of what database engine it is running under.

We should attempt to share logic as much as possible, see this function as example

public GameLevel GetStoryLevelById(int id)
{
    GameLevel? level = this._realm.All<GameLevel>().FirstOrDefault(l => l.StoryId == id);

    if (level != null) return level;
    
    //Create a new level for the story level
    level = new()
    {
        Title = $"Story level #{id}",
        Publisher = null,
        Location = GameLocation.Zero,
        Source = GameLevelSource.Story,
        StoryId = id,
    };
        
    //Add the new story level to the database
    long timestamp = this._time.TimestampMilliseconds;
    this.AddSequentialObject(level, () =>
    {
        level.PublishDate = timestamp;
        level.UpdateDate = timestamp;
    });
    
    return level;
}

Most of this logic can be fully shared by Realm and PostgreSQL, the only logic which truly needs to be different is the first line and the AddSequentialObject implementation.

Where possible, new methods should be added where previously we would call directly into Realm, in the case of the above function, this can be done with a refactor of the first line to the below

GameLevel? level = this.GetAllLevels().FirstOrDefault(l => l.StoryId == id);

Similar changes can be done all throughout, and i feel most of the logic can be fully shared, with a "lower level" interface exposed through protected abstract functions inside of GameDatabaseContext.

@Beyley Beyley added the enhancement New feature or request label Nov 16, 2023
@jvyden jvyden pinned this issue Nov 16, 2023
@jvyden
Copy link
Member

jvyden commented Nov 16, 2023

Response to concerns

How do we go about testing a PostgreSQL implementation, and how to we make it simple for contributors to test on it locally?

While I don't see Docker as suitable for production use, it's very good for development use. Spinning up a postgres container with a default username and password, then exposing it on the local machine would go a long way. Docker should work on all platforms via Docker Desktop - I believe it uses a Linux VM underneath on macOS and Windows.

As for integration testing, ideally, NUnit would spawn a fresh container for each testing session. I believe I saw Nick Chapsas cover a package to help with this at one point. Then, it's a matter of just testing for each database type, similar to how different IDataStores are tested in Bunkum.

Will a cut down database implementation genuinely be useful to small servers? Or will the fact its limited at all push people away from it outside of testing?

Small servers are probably disinterested in the more advanced features Refresh has. They likely just want to play LBP with their friends. Bigger instances will want more advanced features like email verification, a reverse proxy, and more, all of which require extra setup.

In my opinion, Postgres is just one more step to take to set up a bigger instance to enable more advanced features.

Will this change be friendly to new code contributors? We want to encourage people to join in and contribute to the project, and any difficult setup process for the project will push away a majority of people looking to dip their toes into the project.
We need to ensure that a more complex database engine does not degrade the experience for new contributors.

This depends on what said contributors will work on. Realm will still be the default going forward with Postgres as an upgrade option. Worst case scenario is the contributor will have to install Docker, but we can provide guides for operating it for the simple use case of development.

I hope that for the vast majority of good first issues, you won't have to use Postgres for development purposes.

Random thoughts

  • An implementation of Postgres support should definitely be moved to Bunkum once complete in Refresh.
  • Another thing to think about is how the migration process from existing Realm databases (not future Realm databases) will look like, and how long we should keep that code for.
  • New Realm databases should always be able to migrate to Postgres databases
  • How should the database configuration look?

@jvyden jvyden added the proposal A proposal for a feature that is not a missing feature from the game label Nov 19, 2023
@jvyden
Copy link
Member

jvyden commented Nov 19, 2023

@Beyley Did we have any other loose ends to wrap up or are we good to mark this as ready to implement?

@Beyley
Copy link
Member Author

Beyley commented Nov 19, 2023

@Beyley Did we have any other loose ends to wrap up or are we good to mark this as ready to implement?

i have no gripes with the current proposal, so if you are happy with it as well, we can mark it as accepted, and begin working on it

@jvyden
Copy link
Member

jvyden commented Nov 19, 2023

Sure, sounds good.

@jvyden jvyden added the accepted-proposal This proposal has been accepted, and is awaiting implementation label Nov 19, 2023
@jvyden jvyden changed the title RFC: Move to PostgreSQL as our primary database engine Move to PostgreSQL as our primary database engine Nov 19, 2023
@jvyden jvyden added this to the v3.0 milestone Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted-proposal This proposal has been accepted, and is awaiting implementation enhancement New feature or request proposal A proposal for a feature that is not a missing feature from the game
Projects
None yet
Development

No branches or pull requests

2 participants