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

Player data to Database #5475

Merged
merged 2 commits into from Apr 23, 2017

Conversation

Projects
8 participants
@nerzhul
Copy link
Member

commented Mar 28, 2017

Add player data into databases (SQLite3 & PG only)

This is a WIP

ATM data is saved in Sqlite3 db and in file together, no reading is done
No pg write atm

  • SQLite3: write player to DB
  • SQLite3: load player from DB
  • PostgreSQL: write player to DB
  • PostgreSQL: load player from DB
  • Add a migration processus into world loading (read file, save to db, move file to backup directory)
  • Move player migrated files to backup dir
  • Add migration between SQLite3 & Pg like map migration model
  • Add a builtin command to remove a player (only if not connected)
  • Re-add old loading method and use it by default
  • When using old loading method add a warningstream saying they should use a new backend
  • Refactor DatabaseSQLite/PG
  • Migration documentation
  • creation_date and modification_date fields on player table

@nerzhul nerzhul force-pushed the nerzhul:players_to_db branch from 1f402ab to 4dd151b Mar 29, 2017

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2017

Okay it's now working with PG properly without migration model. The majority of emerge refactor has been done, cleanup is required on save. I will do sqlite tomorrow & start the auto migration processus

src/map.h Outdated
@@ -430,8 +430,6 @@ class ServerMap : public Map
Database functions
*/
static Database *createDatabase(const std::string &name, const std::string &savedir, Settings &conf);
// Verify we can read/write to the database
void verifyDatabase();

This comment has been minimized.

Copy link
@nerzhul

nerzhul Mar 29, 2017

Author Member

note: unimplemented old func

@nerzhul nerzhul added the WIP label Mar 30, 2017

"DELETE FROM player_metadatas WHERE player = $1");

prepareStatement("save_player_metadata",
"INSERT INTO player_metadatas (player, attr, value) VALUES ($1, $2, $3)");

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Mar 30, 2017

Member

data is already plural, this sounds wrong. Should be player_metadata IMO.

This comment has been minimized.

Copy link
@nerzhul

nerzhul Mar 30, 2017

Author Member

i will fix it later this evening

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2017

We are nearly complete ! only migration between databases is missing :)

@orwell96

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2017

You swapped the return codes of minetest.remove_player() in lua_api.txt. 2 is err_player_connected and 1 is not found.

return false, "Name field required"
end

local rc = core.remove_player(toname, '')

This comment has been minimized.

Copy link
@red-001

red-001 Mar 31, 2017

Contributor

the empty string doesn't seem to do anything

This comment has been minimized.

Copy link
@nerzhul

nerzhul Mar 31, 2017

Author Member

exact mismatch copy paste

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 11, 2017

Author Member

fixed

@nerzhul nerzhul modified the milestone: 0.4.16 Apr 6, 2017

@nerzhul nerzhul force-pushed the nerzhul:players_to_db branch from ce2b14c to ca2f5fa Apr 11, 2017

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2017

@sofar @sfan5 i restored the player files backend but i used a new interface to load/save them, which makes it consistent with new backends

@sfan5
Copy link
Member

left a comment

haven't reviewed database-sqlite3.cpp or database-postgresql.cpp

bool loadPlayer(RemotePlayer *player, PlayerSAO *sao) { return true; }
bool removePlayer(const std::string &name) { return true; }

bool supportsMigration() const { return false; }

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

is this migration from dummy or migration to dummy?

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 12, 2017

Author Member

supportsMigration is used when starting ServerEnvironment, if backend supports migration the migration process is launched from files

@@ -2580,6 +2580,8 @@ These functions return the leftover itemstack.
and `reconnect` == true displays a reconnect button.
* `minetest.get_server_status()`: returns server status string
* `minetest.get_server_uptime()`: returns the server uptime in seconds
* `minetest.remove_player(name)`: remove player from database (if he is not connected).
* Returns a code (0: removed, 1: no_such_player, 2: error_connected)

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

Lua can check whether the player is online beforehand, no need for 3 different return codes

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 12, 2017

Author Member

having a more intelligent call permit to reduce the Lua errors when you use it

infostream << "Failed to write " << path << std::endl;
}
player->setModified(false);
return true;

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

from // Open file and serialize to here: code duplication

path = savedir + player->getName() + itos(i);
}

infostream << "Didn't find free file for player " << player->getName() << std::endl;

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

errorstream


if (player->getName() == player_to_load) {
return true;
}

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

brace removal

try {
m_player_database->savePlayer(*it);
}
catch (DatabaseException &e) {

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

code style?

try {
m_player_database->savePlayer(player);
}
catch (DatabaseException &e) {

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

code style?

<< "dbname=<pgdb>' to your world.mt." << std::endl
<< "After configuring new player_backend setting an automatic migration will be "
<< "executed by " << PROJECT_NAME << " on startup. Your old player files will be "
<< "moved to players.old folder inside world folder." << std::endl;

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

this is waay to long, it should instead link to e.g. a page on http://dev.minetest.net

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 12, 2017

Author Member

@sfan5 it's for developers not end users, for a such thing i can add it to wiki.minetest.net

// list files into players directory
for (std::vector<fs::DirListNode>::const_iterator it = files.begin(); it != files.end(); ++it) {
// Ignore directories
if ((*it).dir)

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

it->dir

actionstream << "Migrated " << players_migrated << " players." << std::endl;
if (failed_migrations > 0)
errorstream << "Failed to migrate " << failed_migrations << " players." << std::endl;
}

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

generally this process should IMO work similar to the --migrate for the mapdb

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 12, 2017

Author Member

will work on the last point and move the migration mode to this new param

try {
if (!m_player_database->savePlayer(*it)) {
errorstream << "Unable to save player " << (*it)->getName()
<< ", SAO was not found." << std::endl;

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

can't a false also indicate other errors?
if not then consider making savePlayer a void method

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 12, 2017

Author Member

it's not so simple, DatabaseExceptions are triggered at request time, false is only triggered when SAO is not available, but i can return and we can ignore error, SAO unavailable on save is not possible with the refactor, i will do this

// Query verifiers helpers
inline void sqlite3_vrfy(int s, const std::string &m = "", int r = SQLITE_OK) const
{
if (s != r) { \

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

you left the \, also braces

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 12, 2017

Author Member

oh yes

sqlite3_vrfy(sqlite3_bind_int64(s, iCol, (sqlite3_int64) val));
}

inline void double_to_sqlite(sqlite3_stmt *s, int iCol, const double val) const

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

useless const

sqlite3_vrfy(sqlite3_bind_int(s, iCol, val));
}

inline void int64_to_sqlite(sqlite3_stmt *s, int iCol, const s64 val) const

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 12, 2017

Member

useless const

@sofar

This comment has been minimized.

Copy link
Member

commented Apr 15, 2017

2017-04-14 22:42:24: ACTION[Main]: Migrating player file 'sofar2' ....... Failed !!
2017-04-14 22:42:24: ACTION[Main]: Migrating player file 'sofar' ....... Failed !!
2017-04-14 22:42:24: ACTION[Main]: Migrating player file 'sofar5' ....... Failed !!
2017-04-14 22:42:24: ACTION[Main]: Migrating player file 'singleplayer' ....... Failed !!

singleplayer file as example:

extended_attributes = """
{"stamina:exhaustion":"46.5","stamina:hud_id":"1","stamina:level":"18"}

"""
breath = 11
yaw = 211.99
pitch = 13.59
version = 1
name = singleplayer
hp = 20
position = (-71.3,205,244.99)
PlayerArgsEnd
List main 32
Width 0
Item default:sand
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
EndInventoryList
List hand 1
Width 0
Empty
EndInventoryList
List craft 9
Width 3
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
Empty
EndInventoryList
List craftpreview 1
Width 0
Empty
EndInventoryList
List craftresult 1
Width 0
Empty
EndInventoryList
EndInventory
@sofar

This comment has been minimized.

Copy link
Member

commented Apr 15, 2017

game subsequently crashes on game start with:

2017-04-14 22:44:02: ERROR[Server]: Failed to save player sofar exception: Failed to start SQLite3 transaction: cannot start a transaction within a transaction
terminate called after throwing an instance of 'DatabaseException'
  what():  Failed to start SQLite3 transaction: cannot start a transaction within a transaction
Aborted (core dumped)
@sofar

This comment has been minimized.

Copy link
Member

commented Apr 15, 2017

Plain file backend still works though.

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2017

hmmm very strange, double transaction ? i think it can be interesting to understand why there is two begin. Will look at this, but i'm changing the migration process to manual as requested by @sfan5

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2017

@sofar just for my information your database exception is after automated migration in failure case ?

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2017

@sofar your problem is due to a thing done regarding sfan5 comment, merging sqlite3_vrfy(sqlite3_bind_text(s, iCol, str, strlen(str), NULL)); using std::string handler, which doesn't work with our player name storage.

@nerzhul nerzhul changed the title WIP: player data to Database Player data to Database Apr 15, 2017

@nerzhul nerzhul removed the WIP label Apr 15, 2017

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2017

This is nolonger a WIP. @sofar all is fixed, @sfan5 bi directional migration is okay

@nerzhul nerzhul force-pushed the nerzhul:players_to_db branch from bb14720 to 14644e2 Apr 15, 2017

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2017

Just a question, are you okay if we set sqlite3 by default on new worlds ?

@sofar sofar added the One approval label Apr 18, 2017

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2017

Thanks sofar. I will merge this PR on 23 Apr if no more objections

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2017

A trivial todo: add last_modification field on players tables permitting purge it easily

local rc = core.remove_player(toname)

if rc == 0 then
core.log("action", name .. " removed " .. toname .. " data.")

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 18, 2017

Member

name .. " removed player data of " .. toname .. "."

return true, "Player \"" .. toname .. "\" is connected, cannot remove."
end

return false, "Unhandled remove_player return code " .. rc .. ""

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 18, 2017

Member

indent and useless .. ""

@@ -2584,6 +2584,8 @@ These functions return the leftover itemstack.
* `minetest.cancel_shutdown_requests()`: cancel current delayed shutdown
* `minetest.get_server_status()`: returns server status string
* `minetest.get_server_uptime()`: returns the server uptime in seconds
* `minetest.remove_player(name)`: remove player from database (if he is not connected).
* Returns a code (0: removed, 1: no_such_player, 2: error_connected)

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 18, 2017

Member

* Returns a code (0: successful, 1: no such player, 2: player is connected)

args.setS32("version", 1);
args.set("name", player->getName());

// This should not happen

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 18, 2017

Member

comment is quite useless

args.set("name", player->getName());

// This should not happen
assert(player->getPlayerSAO());

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 18, 2017

Member

should be a sanity_check


while (sqlite3_step(m_stmt_player_list) == SQLITE_ROW) {
res.push_back(sqlite_to_string(m_stmt_player_list, 0));
}

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 18, 2017

Member

brace removal

{
if (!playerDataExists(name)) {
return false;
}

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 18, 2017

Member

brace removal

return migrate_database(game_params, cmd_args);
return migrate_map_database(game_params, cmd_args);
else if (cmd_args.exists("migrate-players"))
return ServerEnvironment::migratePlayersDatabase(game_params, cmd_args);

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 18, 2017

Member

why is this part of ServerEnvironment and not here?

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 18, 2017

Author Member

i moved this to ServerEnvironment as players are part of the environment and make access to database factory private. I can re-set it to main if needed, but i don't think in CPP a main should have many anonymous functions if not needed

warningstream << "/!\\ You are using old player file backend. "
<< "This backend is deprecated and will be removed in next release /!\\"
<< std::endl
<< "You should switch soon to SQLite3 or PostgreSQL."

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 18, 2017

Member

can be compacted into:

  			<< "Switching to SQLite3 or PostgreSQL is advised, please read http://wiki.minetest.net/Database_backends."
 			<< std::endl;
+ "players.bak";

if (backend == "files") {
// Create backup directory

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 18, 2017

Member

I get that this is useful but isn't it on the server/world admin to create backups when migrating?

(just a question, i'm okay with keeping this in the end)

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 18, 2017

Author Member

We can trust admins, but i don't think it's a good idea, especially if we really remove files. I prefer to have a full process and not trust admins :)

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2017

Date for creation and modification has been added to sqlite & pg

Here is a sample

sqlite> select * from player;
singleplayer|19|284.600006103516|-190|35|-220|20|11|2017-04-18 14:19:40|2017-04-18 14:30:55

@nerzhul nerzhul force-pushed the nerzhul:players_to_db branch from 7680c6c to 6289a8b Apr 18, 2017

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2017

PostgreSQL is okay, i just push a little fix for numeric precision when strange cases occurs (cf VanessaE files)

capture d ecran de 2017-04-18 20-07-35

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2017

All is right, sfan5 waiting for you, when i get your approval i will merge :)

Mentioned points are fixed and questions answered

@nerzhul nerzhul force-pushed the nerzhul:players_to_db branch from 5af7947 to 18f11ba Apr 21, 2017

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2017

as i said 1 week ago, and because sofar approved it, it will be merged on sunday 23th april

errorstream << "Failed to write " << path << std::endl;
}
player->setModified(false);
return;

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 22, 2017

Member

this is still unsolved (duplicate code)

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2017

i will fix remaining point tomorrow

@nerzhul nerzhul force-pushed the nerzhul:players_to_db branch 2 times, most recently from d205ba6 to 15eb02e Apr 23, 2017

Player data to Database
Add player data into databases (SQLite3 & PG only)

PostgreSQL & SQLite: better POO Design for databases

Add --migrate-players argument to server + deprecation warning

@nerzhul nerzhul force-pushed the nerzhul:players_to_db branch from 15eb02e to 8d51df0 Apr 23, 2017

Remaining point fixed

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2017

@nerzhul nerzhul merged commit 29ab20c into minetest:master Apr 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nerzhul nerzhul added this to Done in Minetest 0.4.16 Apr 29, 2017

@JustinLaw64

This comment has been minimized.

Copy link

commented Jun 1, 2017

Is there a way to access the player database when one has to delete their singleplayer data, or when someone on a server has a name they shouldn't have, like "Admin"?

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Jun 1, 2017

@ForbiddenJ

core.register_chatcommand("remove_player", {
	params = "<name>",
	description = "Remove player data",
	privs = {server=true},
	...
})

Sauce: https://github.com/minetest/minetest/blob/master/builtin/game/chatcommands.lua#L282

@JustinLaw64

This comment has been minimized.

Copy link

commented Jun 1, 2017

@SmallJoker
I did it the old fashioned way (deleting lines from auth.txt and respective files from the players directory) until this commit came. This resolves my issue. Thank you!

@nerzhul nerzhul deleted the nerzhul:players_to_db branch Jun 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.