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

Implement a PostgreSQL backend #4124

Closed
wants to merge 0 commits into from

Conversation

Projects
None yet
9 participants
@nerzhul
Copy link
Member

commented May 14, 2016

PostgreSQL is better than redis for huge maps because redis will use many memory and for maps > server memory redis cannot handle it properly (in recent versions, since redis 3) because redis will load all in memory.
I use postgresql on my fork (epixel) since 1 year with a very larger backend (not also map but all players and many content) and it works very well.

I decided to backport it to MT for next release

Note: original version was here: https://gitlab.com/epixelgame/epixel/blob/master/src/server/database-postgresql.cpp

@nerzhul nerzhul force-pushed the nerzhul:postgresql branch 2 times, most recently May 14, 2016

@nerzhul nerzhul added this to the 0.4.15 milestone May 14, 2016

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented May 17, 2016

@est31 @sfan5 @Ekdohibs @Zeno- can you review it for merge ?
I add my own approval as i use this backend on AppleTree server since 1 year without problems

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented May 17, 2016

Hmm in need to rebase before, since one of my commit has been merged

@kwolekr

View changes

src/database-postgresql.cpp Outdated
"use the postgresql backend\n"
"Notes:\n"
"pgsql_connection has the following form: \n"
"\tpgsql_connection = host=127.0.0.1 port=5432 user=mt_user password=mt_password dbname=minetest_amazingworld\n"

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

amazingworld...?

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

Fixing it

@Zeno-

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

I don't think "merge for testing" is an appropriate label for a person to self-apply. It's meant more for something that has been reviewed and tested by the appropriate number of other devs and the PR is technically approved to be merged but for some reason has been delayed (e.g. because of a feature freeze, but there are other reasons I can think of as well).

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

It appears that you forgot to add database-postgresql.cpp to the Android makefile.

@kwolekr

View changes

src/database-postgresql.cpp Outdated
Database_PostgreSQL::Database_PostgreSQL(const Settings &conf):
m_connect_string(""), m_conn(NULL), m_pgversion(0)
{
try {

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

If you're going to catch an exception just to throw it again, why not use Settings::getNoEx()?

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

Fixing it

@kwolekr

View changes

src/database-postgresql.cpp Outdated

// If table doesn't exist, create it
if (!PQntuples(result)) {
const std::string dbcreate_sql = "CREATE TABLE blocks ("

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Why do you make this a const std::string just to use .c_str() a statement later?

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

const std::string != const char*

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Certainly, it's just that I don't see the point of using a std::string here at all.

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

Yes this is right, we can use directly a const char*

@kwolekr

View changes

src/database-postgresql.cpp Outdated
* If you don't use this intermediate, the conversion
* to std::string followed by conversion to const char*
* could have bad issues, refering to bad memory areas.
*/

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

What?

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

If you have an idea how to fix this issue, i tried to do itos(string).c_str() and that failed, the postgresql connector doesn't like it

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Of course itos(string).c_str() will give you problems, itos() returns a temporary std::string object whose lifetime ends before the next statement. This is obvious and I can't understand why anybody would need a comment warning them about it.

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

Would be nice to have a unit test for this.

@kwolekr

View changes

src/exceptions.h Outdated
@@ -65,6 +65,11 @@ class FileNotGoodException : public BaseException {
FileNotGoodException(const std::string &s): BaseException(s) {}
};

class DatabaseException : public BaseException {

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Why?

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

DatabaseException sounds better than FileNotGoodException. PostgreSQL is not a file.

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Don't you think this is a change that ought to be done in a separate commit?

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

I can, but then we should merge it before this commit to use it. Did you agree with this ? If yes i will do a PR in ~15 min

@kwolekr

View changes

src/database-postgresql.cpp Outdated
* @brief Database_PostgreSQL::saveBlock
* @param pos
* @param data
* @return always true

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Why do you have doxygen comments in the source file..?

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

It's historical imports from my fork, but i can remove it :)

@kwolekr

View changes

src/database-postgresql.cpp Outdated
ExecStatusType statusType = PQresultStatus(result);

if ((statusType != PGRES_COMMAND_OK && statusType != PGRES_TUPLES_OK) ||
statusType == PGRES_FATAL_ERROR) {

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

This is confusing, I think it could be made better by converting it into a switch statement.

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

do you prefer

switch (statusType) {
case PGRES_COMMAND_OK:
case PGRES_TUPLES_OK:
break;
case PGRES_FATAL_ERROR:
default:
throw
}

It seems difficult to handle the same behaviour

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Yes, that would be preferable to a mishmash of logical operators and parentheses.

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

I'm not sure this is as quite easy to handle it with a switch case, especially if postgresql api changes

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

How? You have two cases as far as I can tell: Return values that are failures and return values that are not failures.

@kwolekr

View changes

src/database-postgresql.cpp Outdated
"PostgreSQL database error: ") +
PQerrorMessage(m_conn));
}
return true;

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

I don't get what the point of returning a boolean is if you're just going to throw an exception...

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

right

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

fixing it

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

Please review the Minetest code style guidelines.

@kwolekr

View changes

README.txt Outdated
@@ -170,6 +170,7 @@ ENABLE_FREETYPE - Build with FreeType2; Allows using TTF fonts
ENABLE_GETTEXT - Build with Gettext; Allows using translations
ENABLE_GLES - Search for Open GLES headers & libraries and use them
ENABLE_LEVELDB - Build with LevelDB; Enables use of LevelDB map backend (faster than SQLite3)
ENABLE_POSTGRESQL - Build with libpq; Enables use of PostgreSQL map backend (faster than LevelDB and less memory consuming than Redis)

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

I'm not sure if "faster than LevelDB" is a completely accurate statement.

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

PostgreSQL is faster than leveldb for big databases (> 100k blocks)

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Maybe the virtues of each DB should be explained in a separate section of the README, not inline with the build flags.

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

Right, i removed it from leveldb too then. Fixed

@kwolekr

View changes

src/database-postgresql.cpp Outdated
const int valLen[] = {sizeof(int), sizeof(int), sizeof(int), (int)data.size()};
const int valFormats[] = { 0, 0, 0, 1 };

resultsCheck(PQexecPrepared(m_conn, "write_block", 4,

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Why not use execPrepared here?

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

because execPrepared doesn't handle valLen and valFormats, it's a more consise expression not dedicated to this special form of call

@est31

View changes

src/database-postgresql.cpp Outdated
return true;
}

void Database_PostgreSQL::listAllLoadableBlocks(std::vector<v3s16> &dst)

This comment has been minimized.

Copy link
@est31

est31 May 17, 2016

Contributor

non const reference

This comment has been minimized.

Copy link
@est31

est31 May 17, 2016

Contributor

Eww thats part of the official API ... probably needs fixing in a separate PR.

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

@est31 yes :)

@nerzhul nerzhul force-pushed the nerzhul:postgresql branch May 17, 2016

@kwolekr

View changes

src/CMakeLists.txt Outdated
if(PG_LIBRARY AND PG_CONFIG_EXECUTABLE)
set(USE_POSTGRESQL TRUE)
message(STATUS "PostgreSQL backend enabled")
include_directories(${PostgreSQL_INCLUDE_DIR})

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Is there any reason why PostgreSQL_INCLUDE_DIR does not follow the convention of ALL_CAPS like the rest of the build variables?

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

Fixing it

@nerzhul nerzhul force-pushed the nerzhul:postgresql branch May 17, 2016

@kwolekr

View changes

src/database-postgresql.cpp Outdated
resultsCheck(PQprepare(m_conn, name.c_str(), sql.c_str(), 0, NULL));
}

PGresult* Database_PostgreSQL::resultsCheck(PGresult* result, bool clear /*= true*/)

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Controlling whether or not the result is freed using an optional boolean parameter makes me a bit nervous.
Would you mind splitting resultsCheck into two separate functions for clear/non-clear?

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Usually method names follow the convention of verbObject, not objectVerb. Do you mind changing resultsCheck to checkResults()? It sounds better too.

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Pointer asterisks need to be on the right side, like PGresult *result.

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

fixing the two mentionned issues (asterisk + verb)

@nerzhul nerzhul force-pushed the nerzhul:postgresql branch May 17, 2016

@kwolekr

View changes

src/database-postgresql.cpp Outdated

if (PQntuples(results)) {
size_t rd_s;
unsigned char* rd = PQunescapeBytea((unsigned char*)PQgetvalue(results, 0, 0), &rd_s);

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

If you loaded bytea data in binary format to begin with, you wouldn't need to use PQunescapeBytea at all.
Also you don't check the return value of this function before using it.

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

The PQunescapeBytea is needed for this call else it will not work properly.

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

Right but if you look at the documentation for PQunescapeBytesa you'll see that it's only needed when handling data in text mode instead of binary mode. If you were using binary mode to begin with, the official documentation states that you wouldn't need this step.

Please review the documentation for PostgreSQL's C/++ interface.

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

You are right, i remember now why i use this: our string causes postgresql to trigger in some binary string encoding errors (UTF-8 invalid characters). It's the only method i found to have a proper serialized string stored into database and read from database

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 17, 2016

Contributor

I don't understand. If you were storing and loading the data in a binary format, why would you ever have a UTF-8 encoding error? It should be a byte-for-byte copy of the data.

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 17, 2016

Author Member

I agree with you at this point, but it seems its not what happened on insertion.
I should test the reading and fixing it to use binary mode to see if it works properly without unescaping, this part was quite old (> 1 year)

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 20, 2016

Contributor

Maybe your problem is passing false to 'nobinary' in execPrepared.

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 21, 2016

Author Member

Yes this was that, but if you want bytea + other things it doesn't work without this mode. Here we only need a bytea this works perfect

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 21, 2016

Author Member

Then fixed.

@kwolekr

View changes

src/database-postgresql.h Outdated
}

// Conversion helpers
inline const int pg_to_int(PGresult *res, int row, int col) {

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 22, 2016

Contributor

Why do these two conversion helpers return a const value?

@kwolekr

View changes

src/database-postgresql.h Outdated

// Attributes
std::string m_connect_string;
PGconn* m_conn;

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 22, 2016

Contributor

The asterisk needs to be on the right side.

@kwolekr

View changes

src/database-postgresql.h Outdated
bool clear = true, bool nobinary = true)
{
return checkResults(PQexecPrepared(m_conn, stmtName, paramsNumber,
(const char**) params, paramsLengths, paramsFormats,

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 22, 2016

Contributor

params should be const char *const *

@kwolekr

View changes

src/database-postgresql.h Outdated
void connectToDatabase();
void initStatements();
void createDatabase();
inline void prepareStatement(const std::string &name, const std::string &sql)

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 22, 2016

Contributor

There should be a newline in between void createDatabase(); and this line.

@nerzhul nerzhul force-pushed the nerzhul:postgresql branch 2 times, most recently May 22, 2016

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented May 22, 2016

@kwolekr mentionned style issues fixed

@kwolekr

View changes

src/database-postgresql.h Outdated
// Database usage
PGresult *checkResults(PGresult *res, bool clear = true);

inline PGresult* execPrepared(const char* stmtName, const int paramsNumber,

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 22, 2016

Contributor

Dammit nerzhul these * need to be fixed too

@nerzhul nerzhul force-pushed the nerzhul:postgresql branch May 22, 2016

@kwolekr

View changes

src/database-postgresql.h Outdated
PGresult *checkResults(PGresult *res, bool clear = true);

inline PGresult* execPrepared(const char* stmtName, const int paramsNumber,
const void** params,

This comment has been minimized.

Copy link
@kwolekr

kwolekr May 22, 2016

Contributor

And here

@nerzhul nerzhul force-pushed the nerzhul:postgresql branch 4 times, most recently May 22, 2016

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented May 22, 2016

@nerzhul nerzhul force-pushed the nerzhul:postgresql branch 5 times, most recently May 22, 2016

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented May 22, 2016

@nerzhul nerzhul closed this May 22, 2016

@nerzhul nerzhul force-pushed the nerzhul:postgresql branch to ce42ff9 May 22, 2016

@0-afflatus

This comment has been minimized.

Copy link

commented May 23, 2016

I'm very tempted to see if I can migrate an existing (fairly large) world to this and put it through its paces on a live multiplayer server. Can you advise where potential problems may be with that?

@nerzhul

This comment has been minimized.

Copy link
Member Author

commented May 23, 2016

yes you can do it without problems. PostgreSQL is a very huge backend, used by yandex for example.
There is no problem with postgresql, just think to tune the postgresql sharedbuffers properly depending of your available memory (1GB or 2GB if you can), increase a little bit the workmem and create a dedicated user,password,database for each world.

@ShadowNinja

This comment has been minimized.

Copy link
Member

commented May 23, 2016

@nerzhul: The positions are block coordinates. With 16 bits the world can already be extended to 1,048,576 nodes across. 32 bits is just a waste.

@nerzhul nerzhul deleted the nerzhul:postgresql branch Jun 3, 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.