[ZBX-6407] Slow query in Last 20 issues Created: 2013 Mar 18 Updated: 2017 May 30 Resolved: 2013 Apr 22 |
|
Status: | Closed |
Project: | ZABBIX BUGS AND ISSUES |
Component/s: | API (A) |
Affects Version/s: | 2.0.6rc1, 2.1.0 |
Fix Version/s: | 2.0.7rc1, 2.1.0 |
Type: | Incident report | Priority: | Major |
Reporter: | Alexey Pustovalov | Assignee: | Unassigned |
Resolution: | Fixed | Votes: | 0 |
Labels: | performance, sql | ||
Remaining Estimate: | Not Specified | ||
Time Spent: | Not Specified | ||
Original Estimate: | Not Specified |
Issue Links: |
|
Description |
Last 20 issues displays only 20 issues (of course it can be configured, but default is 20). |
Comments |
Comment by Alexey Pustovalov [ 2013 Mar 18 ] |
For example (without started zabbix_server): SQL (0.773198): SELECT t.triggerid,t.value_flags,t.error,t.url,t.expression,t.description,t.priority,t.type,t.lastchange FROM triggers t WHERE EXISTS (SELECT NULL FROM functions f,items i,hosts_groups hgg JOIN rights r ON r.id=hgg.groupid AND r.groupid IN ('118','239','236') WHERE t.triggerid=f.triggerid AND f.itemid=i.itemid AND i.hostid=hgg.hostid GROUP BY f.triggerid HAVING MIN(r.permission)>=2) AND NOT EXISTS (SELECT NULL FROM functions f,items i,hosts h WHERE t.triggerid=f.triggerid AND f.itemid=i.itemid AND i.hostid=h.hostid AND (i.status<>0 OR h.status<>0)) AND t.status=0 AND t.value='1' AND t.flags IN ('0','4') AND t.triggerid BETWEEN 000000000000000 AND 099999999999999 ORDER BY lastchange DESC,t.lastchange DESC make_latest_issues() -> CAPIObject->get() -> CAPIObject->__call() -> czbxrpc::call() -> czbxrpc::callAPI() -> call_user_func() -> CTrigger->get() -> DBselect() So we can try get required triggers by parts. For example, frontend gets 20 triggers in PROBLEM state, if some triggers have parent dependencies in PROBLEM state frontend gets other missing PROBLEM triggers. |
Comment by Toms (Inactive) [ 2013 Apr 09 ] |
Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-6407 |
Comment by richlv [ 2013 Apr 11 ] |
(1) could you please describe the new algorithm ? tomtom We don't write such low level implementation details here, but if you like to know how this works now you can check commit message for r35028 <richlv> the description would go in whatsnew, similar how we have described several new mechanisms before. it would have to be so that one would not have to be ashamed to show it to people |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 11 ] |
(2) In the fix you've also fixed the number of problems to work correctly with trigger dependencies. This made the widget performance worse. I suggest we leave the number of issues as is for now. tomtom RESOLVED in r35028 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 11 ] |
(3) Wouldn't it be better to select triggers by, lets say, 40 instead of 1000? tomtom RESOLVED in r35028 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 15 ] |
(4) CTrigger, line 602: $hasMore = (count($triggers) === $limit); Don't use strict comparison, limit could also be a string. tomtom RESOLVED jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 15 ] |
(5) It would be great to move the post filter logic to the parent class so we could use it in other APIs. Here's what I suggest. Our usual data-retrieval loop looks approximately like this (copied from CService): // build and execute query $sql = $this->createSelectQuery($this->tableName(), $options); $res = DBselect($sql, $options['limit']); // fetch results $result = array(); while ($row = DBfetch($res)) { // a count query, return a single result if ($options['countOutput'] !== null) { $result = $row['rowscount']; } // a normal select query else { $result[$row[$this->pk()]] = $row; } } We could modify it to look like this: // build and execute query $sql = $this->createSelectQuery($this->tableName(), $options); // fetch results $result = array(); foreach ($this->fetchPostFiltered($sql, $options) as $row) // a count query, return a single result if ($options['countOutput'] !== null) { $result = $row['rowscount']; } // a normal select query else { $result[$row[$this->pk()]] = $row; } } Where the CZBXAPI::fetchPostFiltered() method will return already filtered rows of data. The method itself could look something like this: protected function fetchPostFiltered($sql, $options) { // some method to determine, whether post filtering is required if (!$this->requiresPostFiltering($options)) // if no post filtering is required, return the data straight from the DB return DBFetchArray(...); } // if post filtering is required else { $result = array(); $fetchMore = true; while($fetchMore) { // fetch first batch of data $data = fetchFirstBatch($sql, $limit, $offset); // filter the new rows $filteredData = $this->applyPostFilters($data, $options); // append the data to the result $result = array_merge($result, $filteredData); // determine whether we need to continue // we stop if we either the number of results requested in $options['limit'] // or count($data) < $limit $fetchMore = doWeNeedMore() // calculate new $limit and $offset } if ($options['countOutput'] !== null && $options['groupCount'] === null) { return count($result); } else { return $result; } } The CZBXAPI::applyPostFilters() will be implemented in each API separately and method could look like this: protected function applyPostFilters($data, $options) { if ($options['myPostFilter'] !== null) { // apply the filter to $data } return $data; } tomtom RESOLVED in r35086 jelisejev Awesome. CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 19 ] |
(6) I have three triggers on a host, but the following request returns an empty result { "hostids": "30044", "skipDependent": true } Same thing if I use the withLastEventUnacknowledged parameter. Works correctly if I add limit. tomtom RESOLVED in r35144 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 19 ] |
(7) In CTrigger.php on line 589: $triggerids = zbx_toHash(array_keys($triggers)); There's no need to convert trigger IDs to a hash. tomtom RESOLVED in r35145 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 19 ] |
(8) The CTrigger::requiresPostSqlFiltering() and CTrigger::applyPostSqlFiltering() methods should be implemented in CZBXAPI class as empty stub methods. tomtom RESOLVED in r35145 jelisejev I've added some minor changes so that the methods would perform as commented in r35153. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 19 ] |
(9) In CZBXAPI::customFetch() 1. Instead of // truncate element set after post SQL filtering, if enough elements or more retrieved via SQL query if ($options['limit'] && count($allElements) + count($elements) >= $options['limit']) { $allElements += array_slice($elements, 0, $options['limit'] - count($allElements), true); break; } $allElements += $elements; It would be easier to first add the new element to all elements and then just compare it's length to $options['limit'] tomtom It would perform unnecessary actions and make performance worse. 2. To avoid running count() twice you can replace $elemCount = count($elements) ? count($elements) : 1; with $elemCount = min(count($elements), 1); tomtom You can't use min as it will take 0 as min value and cause division by zero. Counts for PHP arrays are cached so no worry to use them as much as necessary. 1. True, let's leave it as is. CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 19 ] |
(10) All array parameters must be type-hinted. tomtom RESOLVED in r35147 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 19 ] |
(11) I've removed some unnecessary spaces in r35137. tomtom Seems OK, CLOSED |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 19 ] |
(12) Please use === null instead of is_null(). It's mentioned in our guidelines. tomtom RESOLVED in r35147 jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2013 Apr 22 ] |
TESTED. Please review (8) before merging. |
Comment by Toms (Inactive) [ 2013 May 07 ] |
Fixed in 2.0.7 r35430 |
Comment by Toms (Inactive) [ 2013 May 07 ] |
Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-6407-tr for trunk |
Comment by Pavels Jelisejevs (Inactive) [ 2013 May 08 ] |
Trunk fixed TESTED. |
Comment by Toms (Inactive) [ 2013 May 09 ] |
Fixed as well in 2.1.0 r35530 |
Comment by richlv [ 2013 Jun 18 ] |
this resulted in a regression - |