Fix Postgresql

[LuAPi]

Add patch https://sourceforge.net/p/mumble/patches/368/
Update postgresql create user table function to include salt and kdfiterations.
Fix dblog function to work for both PostgreSQL and other database types.

modified: src/murmur/ServerDB.cpp
modified: src/murmur/ServerDB.h

Addresses: #2187
Tested with Postgresql 9.5

[mkrautz]

Thanks for reviving this.

I've written down a few notes from my first read through of the patch:

• In some places, you use != MYSQL && != SQLITE. Please use == PSQL instead.

• Honestly, I'd prefer if we just mimicked MySQL and SQLite's REPLACE INTO semantics, and
  did

  DELETE FROM table WHERE pk=?
  INSERT INTO table VALUES (?,?,?) ...
  

  for Postgres where the other DB engines do REPLACE INTO.
  That's what the other DB engines do under the hood.

  Just ensure we're in a transaction when doing the operation.

  I'm fine with replacing REPLACE INTO with the above syntax.

  If possible, I'd prefer it if we could fit it into a single
  "SQLPREP", but I'm not sure if we can massage the syntax enough
  for all the DB engines to accept it.

• The commit author is different from your GitHub username. I'm confused.

• I'd prefer it if the original patch author was credited in the commit
  message. (Arne derhirni2@users.sourceforge.net)

• Please write a more descriptive commit message, including the changes
  made:

  ServerDB: add PostgreSQL support.
  
  This commit adds Postgres support to Murmur's ServerDB class.
  
  To accomplish this, the commit adds ServerDB::query(), which
  can preprocess queries for targetting a particular DBMS
  (` -> " for Postgres, for example). It also bla bla bla...
  
  This commit replaces all REPLACE INTO queries. Those queries
  have been replaced by a transaction where a DELETE followed by an
  INSERT. This mimicks MySQL's behavior for "REPLACE INTO" queries.
  
  Bla bla bla....
  

• The reconnection logic in ::query() is, in general, not safe.
  (Though I haven't checked all SQLQUERY invocations...).

  Here's why: consider a method that performs multiple operations.
  Because of that, it takes a TransactionHolder and assigns
  QSqlQuery &query = *th.qsqQuery;. If the database goes poof
  during that transaction, and ::query() reconnects -- what happens
  to the transaction? I don't know for sure -- but I would think that
  the transaction is no longer active. Thus, if ::query() is used in
  such a method -- and it reconnects to the DB server, we'll lose
  transactional consistency. Bad.

  It's something we're aware of, and ServerDB.cpp needs some refactoring
  to be able to work correctly in this scenario.

  I'd suggest you just remove the reconnect logic for now.

[LuAPi]

The commit author is different from your GitHub username. I'm confused.

Oops! I have a private bitbucket repo I use for code I share with a gaming guild, and I commit to that using my gaming username. Seems I forgot to switch back to my normal name when I made the commit.

I'd prefer it if the original patch author was credited in the commit message. (Arne derhirni2@users.sourceforge.net)

I should explain. I tried to use postgresql with mumble and found it didn't work. I noticed it needed a code change and I also found the patch with the relevant code change. I cloned the mumble repository, applied the patch and built murmurd. I found that it didn't work because the table layout had changed since the patch was made. I fixed the code and tested the server and it worked. I decided to contribute the fixed patch so I forked the repo, cloned my fork and made a new branch, copied the fixed files from the clone of the official repo to my fork, pushed and submitted a pull request crediting the patch with a link.
Then I discovered that the patch also contains author information (I'm fairly new to using Git) and wondered if the link was insufficient credit, and perhaps the pull request should contain multiple commits (one with the patch merge, and then my changes on top) instead of a single commit (I've heard squashing to single commits is preferred because it's neater.
Should I make the next pull request contain two commits - one with the patch merge, and one with my changes on top?

In some places, you use != MYSQL && != SQLITE. Please use == PSQL instead

Ok, I can do that. I didn't actually write those parts. I think the authors intention was that those bits should be used by any of the databases that aren't officially supported.

Honestly, I'd prefer if we just mimicked MySQL and SQLite's REPLACE INTO semantics, and
did
DELETE FROM table WHERE pk=?
INSERT INTO table VALUES (?,?,?) ...

Would using Postgresql's UPSERT (INSERT ... ON CONFLICT UPDATE) feature be acceptable? It would achieve the same thing, but do so in a single SQL statement meaning that setting up transactions wouldn't be needed. It would mean that only PostgreSQL 9.5 and upwards would work, but that's still an improvement on the current situation.

The reconnection logic in ::query() is, in general, not safe.
I'd suggest you just remove the reconnect logic for now.

That's also code that came from the patch. Since it worked I didn't look at it in much detail. I'll look at it some more and see about removing the reconnection logic.

[mkrautz]

The commit author is different from your GitHub username. I'm confused.

Oops! I have a private bitbucket repo I use for code I share with a gaming guild, and I commit to that using my gaming username. Seems I forgot to switch back to my normal name when I made the commit.

Fair enough :-)

I'd prefer it if the original patch author was credited in the commit message. (Arne derhirni2@users.sourceforge.net)

I should explain. I tried to use postgresql with mumble and found it didn't work. I noticed it needed a code change and I also found the patch with the relevant code change. I cloned the mumble repository, applied the patch and built murmurd. I found that it didn't work because the table layout had changed since the patch was made. I fixed the code and tested the server and it worked. I decided to contribute the fixed patch so I forked the repo, cloned my fork and made a new branch, copied the fixed files from the clone of the official repo to my fork, pushed and submitted a pull request crediting the patch with a link.
Then I discovered that the patch also contains author information (I'm fairly new to using Git) and wondered if the link was insufficient credit, and perhaps the pull request should contain multiple commits (one with the patch merge, and then my changes on top) instead of a single commit (I've heard squashing to single commits is preferred because it's neater.
Should I make the next pull request contain two commits - one with the patch merge, and one with my changes on top?

First applying the patch as one commit, and then working on with consecutive commits would be nice.
Just ensure that each subsequent commit is one logical change. That way, it's easier us to review.

Since you're not the original author of the patch, I think this approach is fine!
We have the baseline patch, and we morph that into something mergeable.

In some places, you use != MYSQL && != SQLITE. Please use == PSQL instead

Ok, I can do that. I didn't actually write those parts. I think the authors intention was that those bits should be used by any of the databases that aren't officially supported.

We don't really want to encourage that.
After PSQL support is merged, I propose that we put a big fat warning such as (in Pseudo-C++):

QStringList supported;
supported << "QSQLITE";
supported << "QMYSQL";
supported << "PSQL";
if (!supported.contains(currentDBBackend)) {
    qFatal("Murmur only supports the following SQL backends: %s", qPrintable(supported.join(QLatin1String(" "))));
}

Honestly, I'd prefer if we just mimicked MySQL and SQLite's REPLACE INTO semantics, and did
DELETE FROM table WHERE pk=?
INSERT INTO table VALUES (?,?,?) ...

Would using Postgresql's UPSERT (INSERT ... ON CONFLICT UPDATE) feature be acceptable? It would achieve the same thing, but do so in a > single SQL statement meaning that setting up transactions wouldn't be needed. It would mean that only PostgreSQL 9.5 and upwards would > work, but that's still an improvement on the current situation.

I took a look at using 9.5's upsert support to accomplish this, and I couldn't figure it out.
But I am not very familiar with Postgres, so maybe I just misunderstood the docs.

The operation we're looking for is "update the record with primary key X to have these values: A, B, C, D".

I couldn't figure out how to accomplish that via the upsert syntax. It seemed to require me to spell out, in the
ON UPDATE DO section, which fields should be updated on PK conflict, which seemed unnecessarily verbose to me.

Feel free to use upsert if you can make it readable. :-) If you do, please put the PSQL-specific parts into

if (... == "PSQL") {

sections.

The reconnection logic in ::query() is, in general, not safe.
I'd suggest you just remove the reconnect logic for now.

That's also code that came from the patch. Since it worked I didn't look at it in much detail. I'll look at it some more and see about > removing the reconnection logic.

Yep, I know.

The reconnection thing IS very important. But the current structure of ServerDB doesn't really allow for it to work safely at the moment.

I hope we can refactor ServerDB's methods that use transactions into separate functions, and then do (in pseudo-code):

// Implementation of the SQL-level logic for a "user update" transaction.
// Private.
bool updateUserImpl(int id, QString name, QString comment) {
    TransactionHolder th;

    SQLQUERY("REPLACE INTO users VALUES (?,?,?)", )
}

// Public API for updating a user.
void updateUser(int id, QString name, QString comment) {
    bool ok;
    do {
        ok = updateUserImpl(id, name, comment);
        if (!ok) {
            reconnectToDB();
        }
    } while (!ok);
}

across the board.
But that's not really relevant to this PR at the moment. :-)