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

Refactored how sql exec and prepare errors are handled in murmur #119

Open
wants to merge 1 commit into
base: master
from

Conversation

@stevenh
Copy link

stevenh commented Sep 18, 2012

  • By default now errors aren't fatal, to try and ensure the best uptime of master servers which could be running many virtual servers.
  • "disconnect" errors are now detected and the command retried where possible.

Note: Due to the lack of Qt driver agnostic method for detecting disconnections, this is done the hard way.

For additional info see:-
https://sourceforge.net/tracker/?func=detail&atid=768005&aid=3566322&group_id=147372

* By default now errors aren't fatal, to try and ensure the best uptime of master servers which could be running many virtual servers.
* "disconnect" errors are now detected and the command retried where possible.

Note: Due to the lack of Qt driver agnostic method for detecting disconnections, this is done the hard way.
@hacst

This comment has been minimized.

Copy link
Member

hacst commented Sep 24, 2012

As we are planning to release a beta for our upcoming release 1.2.4 shortly we decided to postpone patches touching critical areas of code like this one till after the release. Sorry for the delay.

@stevenh

This comment has been minimized.

Copy link
Author

stevenh commented Sep 24, 2012

Thanks for the feedback however I would strongly advise this be included in the release as currently the code causes the master server to die if there's a single SQL disconnect, which for a hosting environment is VERY destructive.

If its any help we've been running this patch in production on a machine running over 500 servers without issue for over 2 weeks now.

@stevenh

This comment has been minimized.

Copy link
Author

stevenh commented Feb 4, 2013

Any news on this, it does includes fixes critical for the stability of the server.

In terms of testing, we've been running with this patch in production on over 30,000 servers for more than 3months so we're confident in its quality.

@mkrautz

This comment has been minimized.

Copy link
Member

mkrautz commented Feb 4, 2013

While it's very, very unfortunate that our release process has stretched this long, it's still too late for us to integrate it this close to release.

We'll integrate it as soon as we've tagged 1.2.4, which is any day now. While it's an inconvenience, people who really need this can, and probably should already be patching this in themselves.

It's not great, I don't really like it, but that's how it is.

@stevenh

This comment has been minimized.

Copy link
Author

stevenh commented Feb 4, 2013

Appreciated guys ;-)

In the interest of helping others know this issue exists could be it mentioned in release notes as an errata?

@mkrautz

This comment has been minimized.

Copy link
Member

mkrautz commented Feb 4, 2013

Yep. There is errata for the Windows client as well (Windows 7+ can disable shortcuts for users). I'll make sure this is added to a Known Issues page for the 1.2.4 release, and mentioned prominently in our release material.

@Zuko

This comment has been minimized.

Copy link
Contributor

Zuko commented Oct 1, 2013

Why this is still not merged?

@ghost ghost assigned mkrautz Dec 7, 2013
@mkrautz

This comment has been minimized.

Copy link
Member

mkrautz commented Jan 21, 2014

Hi @stevenh,

Could you perhaps describe how this change deals with transactions? I believe all of our SQL queries in ServerDB.cpp are backed by a TransactionHolder.

If the connection is lost during a transaction, it seems to me that attempting to reconnect to the server (like is proposed in this PR) will break things, unless it's done on a per-transaction basis.

For example, if the client library's connection is lost in the middle of a transaction, all the changes up until that point will presumably be rolled back by the server. If we then try to re-establish the connection, the worst-case scenario is that client will no longer be in a transaction and will presumably just execute the rest of queries (that we intended to be a single transaction) in a non-transactional manner.

That's obviously bad, because we lose data that's rolled back, and we lose the consistency of our database, again because of the rolled back data, but also because the queries we expected to be performed as a single transaction were not.

It's possible I'm missing something, because I'm not very well-versed in QtSql. So I'd like to hear your take on this.

Thanks!

@davidebeatrici davidebeatrici force-pushed the mumble-voip:master branch from 55a6687 to 501651b Dec 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.