feature: Add mysql failed query logging + fixed queries that break ONLY_FULL_GROUP_BY #5327

Merged
merged 3 commits into from Jan 7, 2017

Projects

None yet

5 participants

@laf
Member
laf commented Jan 6, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Will inline comment on these. Genuinely shouldn't break anything but fixes a lot of broken queries or ones that don't work with ONLY_FULL_GROUP_BY enabled which is default.

laf added some commits Jan 6, 2017
@laf laf feature: Add mysql failed query logging + fixed queries that break ON…
…LY_FULL_GROUP_BY
f9efc43
@laf laf Merge branch 'master' of github.com:librenms/librenms into mysqlllllll
70e9696
@@ -58,7 +58,6 @@ CREATE TABLE IF NOT EXISTS `ports_vlans` ( `port_vlan_id` int(11) NOT NULL AUTO_
CREATE TABLE IF NOT EXISTS `netscaler_vservers` ( `vsvr_id` int(11) NOT NULL AUTO_INCREMENT, `device_id` int(11) NOT NULL, `vsvr_name` varchar(128) COLLATE utf8_unicode_ci NOT NULL, `vsvr_ip` varchar(128) COLLATE utf8_unicode_ci NOT NULL, `vsvr_port` int(8) NOT NULL, `vsvr_type` varchar(64) COLLATE utf8_unicode_ci NOT NULL, `vsvr_state` varchar(32) COLLATE utf8_unicode_ci NOT NULL, `vsvr_clients` int(11) NOT NULL, `vsvr_server` int(11) NOT NULL, `vsvr_req_rate` int(11) NOT NULL, `vsvr_bps_in` int(11) NOT NULL, `vsvr_bps_out` int(11) NOT NULL, PRIMARY KEY (`vsvr_id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
CREATE TABLE IF NOT EXISTS `cmpMemPool` ( `cmp_id` int(11) NOT NULL auto_increment, `Index` varchar(8) NOT NULL, `cmpName` varchar(32) NOT NULL, `cmpValid` varchar(8) NOT NULL, `device_id` int(11) NOT NULL, `cmpUsed` int(11) NOT NULL, `cmpFree` int(11) NOT NULL, `cmpLargestFree` int(11) NOT NULL, `cmpAlternate` tinyint(4) default NULL, PRIMARY KEY (`cmp_id`), KEY `device_id` (`device_id`)) ENGINE=MyISAM DEFAULT CHARSET=latin1;
CREATE TABLE IF NOT EXISTS `hrDevice` ( `hrDevice_id` int(11) NOT NULL auto_increment, `device_id` int(11) NOT NULL, `hrDeviceIndex` int(11) NOT NULL, `hrDeviceDescr` text NOT NULL, `hrDeviceType` text NOT NULL, `hrDeviceErrors` int(11) NOT NULL, `hrDeviceStatus` text NOT NULL, `hrProcessorLoad` tinyint(4) default NULL, PRIMARY KEY (`hrDevice_id`), KEY `device_id` (`device_id`)) ENGINE=MyISAM DEFAULT CHARSET=latin1;
-CREATE TABLE IF NOT EXISTS `dbSchema` ( `revision` int(11) NOT NULL default '0', PRIMARY KEY (`revision`)) ENGINE=MyISAM DEFAULT CHARSET=latin1;
@laf
laf Jan 6, 2017 Member

We have already created the table so no point trying again.

@@ -788,9 +788,9 @@ function getlocations()
// Fetch regular locations
if ($_SESSION['userlevel'] >= '5') {
- $rows = dbFetchRows('SELECT D.device_id,location FROM devices AS D GROUP BY location ORDER BY location');
@laf
laf Jan 6, 2017 Member

device_id is never used so get rid to fix query

} else {
- $rows = dbFetchRows('SELECT D.device_id,location FROM devices AS D, devices_perms AS P WHERE D.device_id = P.device_id AND P.user_id = ? GROUP BY location ORDER BY location', array($_SESSION['user_id']));
@laf
laf Jan 6, 2017 Member

Same here

@@ -143,7 +143,7 @@
echo '</select></td>';
-$count_query = 'SELECT COUNT(id)';
@laf
laf Jan 6, 2017 Member

Can't use COUNT(id) without using GROUP BY which we don't need.

@@ -152,7 +152,7 @@
$param = array($device['device_id']);
}
-$query = " FROM alert_rules $sql ORDER BY id ASC";
@laf
laf Jan 6, 2017 Member

don't need ORDER BY for count query so moved to full query.

@@ -84,7 +84,7 @@
`D1`.`device_id` != `D2`.`device_id`
$where
$sql
- GROUP BY `P1`.`port_id`,`P2`.`port_id`
@laf
laf Jan 6, 2017 Member

GROUP BY must contain all the fields in the SELECT. Tested and # results are the same.

@@ -125,7 +125,7 @@
`remote_device_id` != 0
$where
$sql
- GROUP BY `P1`.`port_id`,`P2`.`port_id`
@laf
laf Jan 6, 2017 Member

As above.

@@ -25,7 +25,7 @@
$sql .= " AND (`S`.`title` LIKE '%$searchPhrase%' OR `S`.`start` LIKE '%$searchPhrase%' OR `S`.`end` LIKE '%$searchPhrase%')";
}
-$count_sql = "SELECT COUNT(`id`) $sql";
@laf
laf Jan 6, 2017 Member

id doesn't exist, it's schedule_id

@@ -38,7 +38,7 @@
$sql .= " LIMIT $limit_low,$limit_high";
}
-$sql = "SELECT * $sql";
@laf
laf Jan 6, 2017 Member

We only use port_descr_descr so change SELECT to only that.

@@ -26,9 +26,12 @@
$sort = 'last_polled_timetaken DESC';
}
-$sql .= " AND D.status ='1' AND D.ignore='0' AND D.disabled='0' ORDER BY $sort";
@laf
laf Jan 6, 2017 Member

Same as other comment, don't need ORDER BY for count query.

@@ -87,9 +87,9 @@
<?php
if ($_SESSION['userlevel'] >= 5) {
- $results = dbFetchRows("SELECT `device_id`,`hostname` FROM `devices` GROUP BY `hostname` ORDER BY `hostname`");
@laf
laf Jan 6, 2017 Member

GROUP BY hostname is irrelevant here. hostnames are unique anyway. Same for other queries in this file.

@@ -240,14 +240,6 @@
}
}
- $peerhost = dbFetchRow('SELECT * FROM ipaddr AS A, ports AS I, devices AS D WHERE A.addr = ? AND I.port_id = A.port_id AND D.device_id = I.device_id', array($peer['bgpPeerIdentifier']));
@laf
laf Jan 6, 2017 Member

$peername set and never used so removed as query for $peerhost is totally wrong (non-existent table).

@@ -38,7 +38,7 @@
$param[] = $_SESSION['user_id'];
}
-$sql .= " WHERE M.port_id = P.port_id AND P.device_id = D.device_id $where GROUP BY `device_id` ORDER BY `hostname`";
@laf
laf Jan 6, 2017 Member

No need for GROUP BY device_id as they are unique already.

@@ -35,7 +35,7 @@
$param[] = $_SESSION['user_id'];
}
-$sql .= " $where GROUP BY `hostname` ORDER BY `hostname`";
@laf
laf Jan 6, 2017 Member

Same here.

@@ -34,7 +34,7 @@
$param[] = $_SESSION['user_id'];
}
-$sql .= " $where GROUP BY `hostname` ORDER BY `hostname`";
@laf
laf Jan 6, 2017 Member

Same here.

@@ -34,7 +34,7 @@
$param[] = $_SESSION['user_id'];
}
-$sql .= " $where GROUP BY `hostname` ORDER BY `hostname`";
@laf
laf Jan 6, 2017 Member

Same here

@@ -99,10 +99,10 @@
</tr>
<?php
if ($_SESSION['userlevel'] >= '5') {
- $host_sql = 'SELECT * FROM devices AS D, services AS S WHERE D.device_id = S.device_id GROUP BY D.hostname ORDER BY D.hostname';
@laf
laf Jan 6, 2017 Member

Same here

@@ -39,23 +39,14 @@ function dbQuery($sql, $parameters = array())
}
}
- /*
@laf
laf Jan 6, 2017 Member

Removed redundant code

- if($this->logFile) {
- $time_end = microtime(true);
- fwrite($this->logFile, date('Y-m-d H:i:s') . "\n" . $fullSql . "\n" . number_format($time_end - $time_start, 8) . " seconds\n\n");
+ if (!$result) {
@laf
laf Jan 6, 2017 Member

So what we do here is check if the mysql log level is INFO or ERROR AND don't contain duplicate entry in the error (we have these and they need to be fixed but will come back to that) and print out any mysql errors OR if the log level is DEBUG then just print out all failed queries.

I've then used this to find all broken queries.

@@ -420,7 +411,11 @@ function dbMakeQuery($sql, $parameters)
krsort($namedParams);
// split on question-mark and named placeholders
- $result = preg_split('/(\?|:[a-zA-Z0-9_-]+)/', $sql, -1, (PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE));
+ if (preg_match('/(\[\[:[\w]+:\]\])/', $sql)) {
@laf
laf Jan 6, 2017 Member

This is a 'hack' I couldn't get a preg_split regex to work across all cases so this retains existing compatibility but checks for people using mysql regex with [[:SOMETHING:]].

If someone can find a good regex to cover all basis then go for it.

It currently splits on ? and : as they are both placeholders for variable substitution and causes problems with [[:digit:]] as per issue #5118 (so fixes that)

@@ -31,6 +31,8 @@
// MySQL extension to use
$config['db']['extension'] = 'mysqli';//mysql and mysqli available
+// MySQL Debug level
+$config['mysql_log_level'] = 'ERROR';
@laf
laf Jan 6, 2017 Member

Set this as the default so all installs will log errors. We could set it to NONE but imho this is now handy to find non-working things easily and won't spam the log file anyway.

@laf
laf Jan 6, 2017 Member

Well unless someone has an issue.

@@ -53,7 +53,7 @@
// Cycle the list of pseudowires we cached earlier and make sure we saw them again.
foreach ($device['pws_db'] as $pw_id => $pseudowire_id) {
if (empty($device['pws'][$pw_id])) {
- dbDelete('vlans', '`pseudowire_id` = ?', array($pseudowire_id));
@laf
laf Jan 6, 2017 Member

Yeah, lol

@@ -36,34 +36,6 @@
$insert = 1;
}
-// For transition from old system
@laf
laf Jan 6, 2017 Member

I literally can't find where we do anything with revision - I think it's a hang up from the fork so got rid of it.

sql-schema/002.sql
@@ -75,7 +75,6 @@ ALTER TABLE `temperature` ADD `temp_precision` INT(11) NULL DEFAULT '1';
UPDATE temperature SET temp_precision=10 WHERE temp_tenths=1;
ALTER TABLE `temperature` ADD `temp_index` INT NOT NULL AFTER `temp_host` , ADD `temp_mibtype` VARCHAR( 32 ) NOT NULL AFTER `temp_index`;
ALTER TABLE `temperature` DROP `temp_tenths`;
-CREATE TABLE IF NOT EXISTS `dbSchema` ( `revision` int(11) NOT NULL default '0', PRIMARY KEY (`revision`)) ENGINE=MyISAM DEFAULT CHARSET=latin1;
@laf
laf Jan 6, 2017 Member

We've already created this table (then tried 3 more times!)

@laf laf fix all schema errors and update system
9e15bee
@laf
Member
laf commented Jan 6, 2017

Pushed further updates to the base sql build which fixes invalid queries. I have a feeling build.sql contains a lot of what was in 001-006.sql as most of the tidying up was done there.

@scrutinizer-notifier

The inspection completed: 1 updated code elements

@laf
Member
laf commented Jan 6, 2017

Exported the database using this new branch, the old branch with both strict disabled and some defaults in centos. A diff shows the difference below (comparing old with non-strict and new shows only differences are datetime)

[root@mysql01 ~]# diff /tmp/ltest_old_nonstrict.sql /tmp/ltest_new.sql
151,152c151,152
<   `start` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00',
<   `end` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00',
---
>   `start` timestamp NOT NULL DEFAULT '1970-01-01 23:00:01',
>   `end` timestamp NOT NULL DEFAULT '1970-01-01 23:00:01',
895c895,896
<   `version` int(11) NOT NULL
---
>   `version` int(11) NOT NULL DEFAULT '0',
>   PRIMARY KEY (`version`)
1275c1276
<   `datetime` datetime NOT NULL DEFAULT '0000-00-00 00:00:00',
---
>   `datetime` datetime NOT NULL DEFAULT '1000-01-01 00:00:00',
3391c3392
< -- Dump completed on 2017-01-06 15:30:25
---
> -- Dump completed on 2017-01-06 15:29:19
[root@mysql01 ~]# diff /tmp/ltest_old_strict.sql /tmp/ltest_new.sql
142a143,168
> -- Table structure for table `alert_schedule`
> --
>
> DROP TABLE IF EXISTS `alert_schedule`;
> /*!40101 SET @saved_cs_client     = @@character_set_client */;
> /*!40101 SET character_set_client = utf8 */;
> CREATE TABLE `alert_schedule` (
>   `schedule_id` int(11) NOT NULL AUTO_INCREMENT,
>   `start` timestamp NOT NULL DEFAULT '1970-01-01 23:00:01',
>   `end` timestamp NOT NULL DEFAULT '1970-01-01 23:00:01',
>   `title` varchar(255) NOT NULL,
>   `notes` text NOT NULL,
>   PRIMARY KEY (`schedule_id`)
> ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
> /*!40101 SET character_set_client = @saved_cs_client */;
>
> --
> -- Dumping data for table `alert_schedule`
> --
>
> LOCK TABLES `alert_schedule` WRITE;
> /*!40000 ALTER TABLE `alert_schedule` DISABLE KEYS */;
> /*!40000 ALTER TABLE `alert_schedule` ENABLE KEYS */;
> UNLOCK TABLES;
>
> --
869c895,896
<   `version` int(11) NOT NULL
---
>   `version` int(11) NOT NULL DEFAULT '0',
>   PRIMARY KEY (`version`)
1238a1266,1296
> -- Table structure for table `eventlog`
> --
>
> DROP TABLE IF EXISTS `eventlog`;
> /*!40101 SET @saved_cs_client     = @@character_set_client */;
> /*!40101 SET character_set_client = utf8 */;
> CREATE TABLE `eventlog` (
>   `event_id` int(11) NOT NULL AUTO_INCREMENT,
>   `host` int(11) NOT NULL DEFAULT '0',
>   `device_id` int(11) NOT NULL,
>   `datetime` datetime NOT NULL DEFAULT '1000-01-01 00:00:00',
>   `message` text CHARACTER SET latin1,
>   `type` varchar(64) CHARACTER SET latin1 DEFAULT NULL,
>   `reference` varchar(64) CHARACTER SET latin1 NOT NULL,
>   PRIMARY KEY (`event_id`),
>   KEY `host` (`host`),
>   KEY `datetime` (`datetime`),
>   KEY `device_id` (`device_id`)
> ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;
> /*!40101 SET character_set_client = @saved_cs_client */;
>
> --
> -- Dumping data for table `eventlog`
> --
>
> LOCK TABLES `eventlog` WRITE;
> /*!40000 ALTER TABLE `eventlog` DISABLE KEYS */;
> /*!40000 ALTER TABLE `eventlog` ENABLE KEYS */;
> UNLOCK TABLES;
>
> --
3334c3392
< -- Dump completed on 2017-01-06 15:29:54
---
> -- Dump completed on 2017-01-06 15:29:19

@Rosiak
Contributor
Rosiak commented Jan 6, 2017

Doesn't seem to break my test install, haven't reviewed the actual code yet.

@murrant
murrant approved these changes Jan 7, 2017 View changes
@murrant
Contributor
murrant commented Jan 7, 2017 edited

Looks good, I didn't test and compare a fresh db build, been running it for a day without issue. I have a few errors logged, but I'll fix those in a different PR.

@laf
Member
laf commented Jan 7, 2017

We ok for a merge then?

@murrant murrant merged commit da5783d into librenms:master Jan 7, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laf laf deleted the laf:mysqlllllll branch Jan 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment