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

Murmur postgresql support #2541

Merged
merged 7 commits into from Sep 7, 2016

Conversation

@LuAPi
Copy link
Contributor

commented Aug 30, 2016

This is the new and improved version of #2383 and addresses issue #2187

This pull request uses the UPSERT functionality of PostgreSQL. If QPSQL is chosen as the database engine in mumble-server.ini then the version of PostgreSQL used needs to be 9.5 or higher.

Key changes this pull request makes:

  • It adds the patch from https://sourceforge.net/p/mumble/patches/368/
  • It updates the create user table function to include salt and kdfiterations since they weren't used when the patch was made.
  • It fixes the dblog function so it works for both PostgreSQL and other database types.
  • The patch used the same method to insert or update rows for all database engines. This has been changed so that PostgreSQL uses UPSERT (INSERT ... ON CONFLICT (...) DO UPDATE ...) and all other engines use the same REPLACE INTO statement as they currently do.
  • The reconnection logic from the patch has been removed as there were issues with it.
  • There are some white space fixes too.

Example settings for mumble-server.ini to use PostgerSQL:
database=localdb
dbDriver=QPSQL
dbUsername=murmur
dbPassword=murmurpass
dbHost=localhost
dbPort=5432
dbPrefix=murmur_

These settings will cause murmur to try and connect to a PostgreSQL database called "localdb" on the server with the address "localhost" and port 5432.
It will connect using the username "murmur" and the password "murmurpass". All the tables and sequences automatically created used will have the prefix "murmur_".
If there is a schema with the same name as the user name then the murmur tables and sequences will be created in that schema (this is the default PostgreSQL behavior).

Arne Fenske and others added 6 commits Aug 8, 2013
Murmur now supports PostgreSQL. The PostgreSQL plugin for Qt 4 ('libq…
…t4-sql-psql' package in Debian) is required. To enable this feature, use the 'dbDriver=QPSQL' option in your 'mumble-server.ini'.
Minimum changes to make previously applied postgresql pacth work with…
… current version of mumur.

Change users table to have new column format.
Change slog table to have a default value of "now()" for column msgtime as other databases use triggers to add this value.
Revert database logging statement so that the timestamp is assigned by triggers or column default values.
Use UPSERT for PostgreSQL, REPLACE INTO for other databases.
Used place holder marks for PostgreSQL UPSERT values instead of positional binding since the statements require the values twice (once for the INSERT, and once for the UPDATE should the insert fail).
The values to use for the ON CONFLICT DO UPDATE part of the UPSERT have been prefixed with u_ .

This commit reverts to using REPLACE INTO for non-PostgreSQL databases and uses the same code for them as the upstream master mumble-voip/mumble.
Previously this branch used the same code for all databases. This commit uses if statements to treat PostgreSQL differently from other databases where it needs to use UPSERT instead of REPLACE INTO.
@LuAPi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2016

I made a mess of my old branch and the rebasing onto all the new commits seemed to make the commit sequence look weird. To fix that I made a new branch and manually did all the edits and commits in sequence. Hopefully all's well with this one. I think I've addressed most of the issues raised about the last pull request.

The only things I'm not sure about are the lines
if (Meta::mp.qsDBDriver != "QSQLITE" && Meta::mp.qsDBDriver != "QPSQL")
and whether they should be changed to
if (Meta::mp.qsDBDriver != "QMYSQL")
If we're only supporting SQLITE, MYSQL and PostgreSQL then that change would achieve the same thing; but then again, I don't want to be the guy who breaks an unsupported DB that happens to be working fine for someone.

I've closed #2383 as this is the new improved version.

I noticed that most of the tables created by murmur in PostgreSQL don't actually have primary keys. It seems a bit odd. I was surprised that the UPSERT was working without the PKEYs being present, and that I didn't need to add a bunch of alter table statements in different places to add unique constraints to trigger the conflicts so that the UPSERT would update instead of insert duplicates like:

if (Meta::mp.qsDBDriver == "QPSQL")
    SQLDO("ALTER TABLE `%1users` ADD UNIQUE (`server_id`, `name`)");
if (Meta::mp.qsDBDriver == "QPSQL")
    SQLDO("ALTER TABLE `%1users` ADD UNIQUE (`server_id`, `user_id`)");
if (Meta::mp.qsDBDriver == "QPSQL")
    SQLDO("ALTER TABLE `%1user_info` ADD UNIQUE (`server_id`, `user_id`, `key`)");
if (Meta::mp.qsDBDriver == "QPSQL")
    SQLDO("ALTER TABLE `%1channel_info` ADD UNIQUE (`server_id`, `channel_id`, `key`)");
if (Meta::mp.qsDBDriver == "QPSQL")
    SQLDO("ALTER TABLE `%1config` ADD UNIQUE (`server_id`, `key`)");

Seems the indexes served that purpose however as I didn't need to add the above bits to get it to work.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Aug 31, 2016

Thanks a bunch for doing this.

I haven't looked closely yet... Just a few drive-by comments.

The only things I'm not sure about are the lines
if (Meta::mp.qsDBDriver != "QSQLITE" && Meta::mp.qsDBDriver != "QPSQL")
and whether they should be changed to
if (Meta::mp.qsDBDriver != "QMYSQL")
If we're only supporting SQLITE, MYSQL and PostgreSQL then that change would achieve the same thing; but then again, I don't want to be the guy who breaks an unsupported DB that happens to be working fine for someone.

I'd prefer if (Meta::mp.qsDBDriver == "QMYSQL").

I noticed that most of the tables created by murmur in PostgreSQL don't actually have primary keys. It seems a bit odd. I was surprised that the UPSERT was working without the PKEYs being present, and that I didn't need to add a bunch of alter table statements in different places to add unique constraints to trigger the conflicts so that the UPSERT would update instead of insert duplicates like:

if (Meta::mp.qsDBDriver == "QPSQL")
SQLDO("ALTER TABLE %1users ADD UNIQUE (server_id, name)");
if (Meta::mp.qsDBDriver == "QPSQL")
SQLDO("ALTER TABLE %1users ADD UNIQUE (server_id, user_id)");
if (Meta::mp.qsDBDriver == "QPSQL")
SQLDO("ALTER TABLE %1user_info ADD UNIQUE (server_id, user_id, key)");
if (Meta::mp.qsDBDriver == "QPSQL")
SQLDO("ALTER TABLE %1channel_info ADD UNIQUE (server_id, channel_id, key)");
if (Meta::mp.qsDBDriver == "QPSQL")
SQLDO("ALTER TABLE %1config ADD UNIQUE (server_id, key)");
Seems the indexes served that purpose however as I didn't need to add the above bits to get it to work.

Hmmm. Seems odd.

@LuAPi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2016

I'd prefer if (Meta::mp.qsDBDriver == "QMYSQL").

Yup == not != , copy and paste error in the comment :)
I'll see about changing those two if statements then.

Regarding the PKEYs on the tables, these tables have them:

  • groups
  • meta
  • servers

And these do not:

  • acl
  • bans
  • channel_info
  • channel_links
  • channels
  • config
  • group_members
  • slog
  • user_info
  • users

Attached is a zip file containing text files with SQL commands which would result in same tables that murmur makes in PostgreSQL (I copied them from the table SQL pane in pgAdmin3)
Database.zip

Edit: It seems the job of the PKey is kinda being done with unique indexes in those tables.
The lack of primary keys isn't restricted to PostgreSQL. I took a look into the SqLite file and it seems to have the same issues.

@LuAPi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

Are there any more changes that need to be made? After this next week I'm not sure when I'll get the time to look at this again.
I think the tables lacking primary keys is more a general issue than a Postgres issue so should be handled in a different patch (maybe I should open up an issue ticket about it?).

@mkrautz

This comment has been minimized.

Copy link
Member

commented Sep 4, 2016

Patch looks pretty good.

@LuAPi It seems to me that as of the current PR, ::query() is now equivalent to ::exec().
Is that correct? If so, can we just get rid of it, and the SQLQUERY macro?

@mkrautz

This comment has been minimized.

Copy link
Member

commented Sep 4, 2016

@LuAPi BTW, did you mean this week? Or the upcoming week?

I can make the change with query() -> exec() if we agree that it's OK. No problem.

@LuAPi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

If you're suggesting replacing instances of SQLQUERY(x) with SQLDO(x) then that won't work.
The problem is that ServerDB::exec tries to prepare statements and in PostgreSQL a statement like "PREPARE CREATE TABLE ..." is invalid and causes an error.
That's the reason all the statements that modify tables in the PostgreSQL patch changed from SQLDO to SQLQUERY.

In PostgreSQL PREPARE may only appear before

Any SELECT, INSERT, UPDATE, DELETE, or VALUES statement.

https://www.postgresql.org/docs/9.5/static/sql-prepare.html

I suppose it might be clearer if exec was renamed prepAndExec or something, and then query was renamed to exec.

I meant this week, really just the next 5 days actually.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Sep 5, 2016

My apologies. I missed the prepare difference between the two when I looked through.

@mkrautz mkrautz merged commit 9be606e into mumble-voip:master Sep 7, 2016

@mkrautz

This comment has been minimized.

Copy link
Member

commented Sep 7, 2016

Thanks once again for making this mergable! :-)

@LuAPi

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2016

No problem. Thanks for the feedback with things to change. I just hope that there isn't something I missed when testing it that breaks something.
Does #2187 get closed now or when the next build is released?

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