Add support for MySQL Port / Unix Socket #1498

josh4trunks · 2016-05-22T16:29:07Z

Logic is added to handle specified port / unix file socket for database connections.
For example, the following now all work as expected.

ZM_DB_HOST=localhost
ZM_DB_HOST=REMOTE_HOST:3307
ZM_DB_HOST=localhost:/path/to/non-default.sock

Fixes #901

josh4trunks · 2016-05-22T16:36:42Z

@knnniggett @connortechnology
Sorry to keep bringing this up, but if we could get this merged at some point I would appreciate it. Then I would not need to keep reapplying these fixes to get my production system working.

This fixes the incomplete logic linked that does not work because it is not implemented in all the places it needs to be.
https://github.com/ZoneMinder/ZoneMinder/blob/master/scripts/ZoneMinder/lib/ZoneMinder/Database.pm#L82
https://github.com/ZoneMinder/ZoneMinder/blob/master/scripts/ZoneMinder/lib/ZoneMinder/Logger.pm#L463

siigna · 2016-05-26T22:05:58Z

👍 Would be awesome for this to get some traction. I was in the IRC channel a while back asking about it.

josh4trunks · 2016-07-28T23:58:35Z

It looks like none of the files this commit touches have changed in a way that will affect this PR.
Can someone please review and comment when time is available.

Thanks!

SteveGilvarry · 2016-07-29T14:26:39Z

@josh4trunks this was literally 19mins after 1.30.0 release, so I take it you are keen to get it merged 😄. I did promise it would go in next so if @connortechnology or @knnniggett have no objections I will merge this weekend.

knight-of-ni · 2016-07-29T14:33:18Z

Well, I was hoping to give the new release a few days out in the wild before we start merging the open pull requests. If something crops up, we won't have a bunch of recent merges in the way if we have to troubleshoot.

josh4trunks · 2016-07-29T14:39:44Z

Yeah, I'd love to get this in so I don't need to keep mergeing in my changes for my production system =]

But, no need to rush this in before making sure we are good with 1.30.0

connortechnology · 2016-07-29T15:06:58Z

Wouldn't fixes to 1.30 go on the 1.30 branch?

I've been running with this code merged for a while. It's pretty solid.

siigna · 2016-07-30T21:10:10Z

I've been running with this code merged for a while. It's pretty solid.

Ditto. Been running this since 1.30.0-RC1 and haven't seen any issues.

knight-of-ni · 2016-08-06T14:32:29Z

I did a quick built and test on Fedora 24 using a single camera and the normal database configuration. Everything worked. Since @connortechnology mentioned this was working for him, I will trust this does not (somehow) negatively affect multiserver. Not sure how it could. In any case, merging....

connortechnology · 2016-08-08T20:39:39Z

src/zm_db.cpp

@@ -37,23 +37,35 @@ void zmDbConnect()
  my_bool reconnect = 1;
  if ( mysql_options( &dbconn, MYSQL_OPT_RECONNECT, &reconnect ) )
    Fatal( "Can't set database auto reconnect option: %s", mysql_error( &dbconn ) );
-  std::string::size_type colonIndex = staticConfig.DB_HOST.find( ":/" );
-  if ( colonIndex != std::string::npos )
+  std::string::size_type colonIndex = staticConfig.DB_HOST.find( ":" );


I think there is a problem in here. So, the new logic is, if we found a colon, then we look for a socket path? That will never happen. The old code looked for either a colon or a slash, and then looked for a /. (By old code I mean some other patch that I merged into storageareas).

See my reply here #1498 (comment).

I put it in the wrong spot =/

connortechnology · 2016-08-08T20:59:06Z

Does this have any practical benefit? Does it have a noticeable performance impact?

josh4trunks · 2016-08-08T22:24:10Z

@connortechnology the line you are looking at finds the length of the hostname/IP part of the DB_HOST config value. The part which decides if it is a TCP or file(unix) connection is at line 53.

josh4trunks · 2016-08-08T22:29:10Z

The practical benefit are you can now specify a socket path to connect to a mysql server. For example if your mysql server has a file socket at /non/default/path/mysql.sock you could enter ZM_DB_HOST=localhost:/non/default/path/mysql.sock
I also believe non-standard ports were not working before because of inconsistencies with this was implemented.

This makes the logic consistent amounts each file for a given language (1 PHP, 2 C++, and 3 perl). Though the way I implemented it is different for each language because I tried to use the most efficient method given the available functions for that language.

josh4trunks · 2016-08-08T22:39:07Z

I assume there might be some small unnoticeable performance impacts because more checking/matching is done before connecting to mysql.

But I did my best to use keep things efficient by using character matching like string[0] == / instead of string matching

connortechnology · 2016-08-08T22:41:12Z

Yes but it will only run line 53 if there was a colon. the socket path doesn't have a colon so that code won't get run.

I think the real line we are talking about is 383. Where it looks for the /. That only gets run if a colon was found.

josh4trunks · 2016-08-08T22:42:39Z

yeah, I assume values are in the format mentioned here.
#1498 (comment)

josh4trunks · 2016-08-08T22:47:00Z

I've implemented this in a few different programs, and usually this is the format assumed. But if we want to also support '/path/to/socket' we could change the logic more. We'd need to also implement this in "web/api/app/Config/database.php.default" before passing to Cake. Not sure if that would be difficult but that logic also relieson the ":"

connortechnology · 2016-08-08T22:49:13Z

Oh, I missed that.. I don't understand why we would specify a host if it is a local socket. Huh. I guess I've just never seen that before. Anyone else care to weigh in?

josh4trunks · 2016-08-08T22:51:26Z

You really don't need to, but that's how alot of programs I've worked with do it. owncloud/nextcloud, pydio, I think even wordpress.
you could just put ':/path/to/socket' and that should work.

josh4trunks added 4 commits May 22, 2016 08:08

Add back changes

2ab7982

Reapply changes to Database.pm

38e1cc9

Apply changes to Logger.pm

a3fe6ea

Add logic to Config.pm

57a2537

josh4trunks mentioned this pull request May 22, 2016

Add support for port/socket #1492

Closed

josh4trunks mentioned this pull request Jun 23, 2016

Release 1.30.0 #1485

Closed

SteveGilvarry added this to the 1.30.1 milestone Jun 24, 2016

knight-of-ni merged commit 382896d into ZoneMinder:master Aug 6, 2016

connortechnology reviewed Aug 8, 2016
View reviewed changes

josh4trunks mentioned this pull request Apr 21, 2017

Api improvements #1470

Merged

connortechnology mentioned this pull request Jun 9, 2017

Fix 1498 #1911

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for MySQL Port / Unix Socket #1498

Add support for MySQL Port / Unix Socket #1498

josh4trunks commented May 22, 2016 •

edited

Loading

josh4trunks commented May 22, 2016 •

edited

Loading

siigna commented May 26, 2016

josh4trunks commented Jul 28, 2016

SteveGilvarry commented Jul 29, 2016

knight-of-ni commented Jul 29, 2016

josh4trunks commented Jul 29, 2016

connortechnology commented Jul 29, 2016

siigna commented Jul 30, 2016

knight-of-ni commented Aug 6, 2016

connortechnology Aug 8, 2016

josh4trunks Jun 9, 2017

connortechnology commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

connortechnology commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

connortechnology commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

Add support for MySQL Port / Unix Socket #1498

Add support for MySQL Port / Unix Socket #1498

Conversation

josh4trunks commented May 22, 2016 • edited Loading

josh4trunks commented May 22, 2016 • edited Loading

siigna commented May 26, 2016

josh4trunks commented Jul 28, 2016

SteveGilvarry commented Jul 29, 2016

knight-of-ni commented Jul 29, 2016

josh4trunks commented Jul 29, 2016

connortechnology commented Jul 29, 2016

siigna commented Jul 30, 2016

knight-of-ni commented Aug 6, 2016

connortechnology Aug 8, 2016

Choose a reason for hiding this comment

josh4trunks Jun 9, 2017

Choose a reason for hiding this comment

connortechnology commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

connortechnology commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

connortechnology commented Aug 8, 2016

josh4trunks commented Aug 8, 2016

josh4trunks commented May 22, 2016 •

edited

Loading

josh4trunks commented May 22, 2016 •

edited

Loading