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

Feature/psql mariadb support #214

Merged
merged 51 commits into from
Sep 28, 2023

Conversation

Sehkah
Copy link
Collaborator

@Sehkah Sehkah commented Sep 21, 2023

Add support for PostgreSQL & MariaDB.
This should also improve support for less common hardware (like armv8) to also be supported, as the SQLite drivers are missing there.

Changes:

  • Breaking: Change SQLite schema definition to be more in line with other engines (i.e. for the most part data type changes)
  • Expose more database settings as environment variables
  • Include sample deployment for MariaDB & PSQL (incl. docker-compose, configs, initial schema definitions)
  • Migrate existing database abstraction to include data readers, which allows the derived classes to have more control if necessary when reading data (e.g. UInt32 can be natively read in MariaDB)
  • Breaking: Change the database settings to include a string-based type identifier, e.g. "sqlite" or "postgresql"
  • Breaking: Rename database setting sqlite folder to a more generic database folder
  • Use built-in transaction handler methods of a database connection rather than manually managing them
  • Ensure all commands and connections are definitely always closed
  • Shift responsibility of closing connections to caller sites
  • Ensure connection pooling is used for all database engines (was off by default in SQLite)
  • Support start-up migration of schema for MariaDB & PSQL by transforming SQLite schema
  • Migrate existing incompatible IGNORE or REPLACE statements to more generic (and slightly less efficient) variants to be engine agnostic
  • Update readme & document how to run docker-compose-based setup
  • Improve Docker build support for different runtimes by exposing an arg to control it

Performed basic tests on all three (SQLite, PSQL, MariaDB) databases on both Windows & Linux (Docker, linux-x64 & linux-arm64), but might have missed some more obscure transactional use cases.

Checklist:

  • The project compiles
  • The PR targets develop branch

Arrowgene.Ddon.Database/DdonDatabaseBuilder.cs Outdated Show resolved Hide resolved
Arrowgene.Ddon.Database/Sql/DdonMariaDb.cs Outdated Show resolved Hide resolved
Arrowgene.Ddon.config.mariadb.local_dev.json Outdated Show resolved Hide resolved
Arrowgene.Ddon.config.mariadb.local_dev.json Outdated Show resolved Hide resolved
@alborrajo
Copy link
Collaborator

If you feel it's ready, over the next days I'll test a little bit both a fresh sqlite database and an old sqlite one to ensure everything is compatible with previous setups, and then if all looks good I'll merge it. Thanks a lot for your contributions I'm sure people running a sizable server will greatly appreciate this one

@Sehkah
Copy link
Collaborator Author

Sehkah commented Sep 25, 2023

If you feel it's ready, over the next days I'll test a little bit both a fresh sqlite database and an old sqlite one to ensure everything is compatible with previous setups, and then if all looks good I'll merge it. Thanks a lot for your contributions I'm sure people running a sizable server will greatly appreciate this one

I'll perform some more manual integration tests on MariaDB & PSQL as well some time this week. I'll also run a test on one of my arm-based systems, that was my original goal, before this spiraled out of control.

A lot of the changes I've made to the replace logic I have mostly tested with SQLite, so I don't expect much to pop up there.

Thanks a lot for the reviews! It's easy to miss stuff especially with an unknown code base & programming language.

@Sehkah
Copy link
Collaborator Author

Sehkah commented Sep 26, 2023

Tested the following with PSQL/MariaDB on Windows via the IDE setup and via containers:

  • Create character
  • Log in / out
  • Open profile
  • Register shortcut phrase
  • Consume item
  • Buy / sell item
  • Buy item to bag & storage
  • Move item bag <-> storage
  • Kill stuff
  • Level up
  • Buy custom skill
  • Set custom skill
  • Change vocation
  • Spawn pawns
  • Equip item
  • Warp

Haven't tested anything on ARM yet.

@Sehkah
Copy link
Collaborator Author

Sehkah commented Sep 27, 2023

Hi @alborrajo
Tested on ARM as well now & improved the docker build handling. I'm satisfied with everything now. Unless you encountered some bugs in your testing.

@alborrajo
Copy link
Collaborator

I played a bit with SQLite lately.
The old config json is no longer compatible. This is of course to be expected since its format changed, and all it takes is to remove the old one for a new one to be generated; but since the default value is for the database to be wiped on startup, it can be very easy to wipe your old database like this. I think it's time to finally switch wipe on startup to false by default.
Second, while working with a new database everything was smooth, working with an existing one where i had already played and leveled up a few classes, everything seemed MUCH slower, so slow that selecting a character ended up in a time out. Maybe transactions in SQLite broke?

@alborrajo
Copy link
Collaborator

Second, while working with a new database everything was smooth, working with an existing one where i had already played and leveled up a few classes, everything seemed MUCH slower, so slow that selecting a character ended up in a time out. Maybe transactions in SQLite broke?

It was a mistake on my setup regarding IP addresses in GameServerList.csv, my bad 🙏

@alborrajo alborrajo merged commit b918053 into sebastian-heinz:develop Sep 28, 2023
1 check passed
@Sehkah Sehkah deleted the feature/psql-mariadb-support branch December 30, 2023 22:35
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