[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:
Duplicate

 Description   

Last 20 issues displays only 20 issues (of course it can be configured, but default is 20).
Frontend gets all PROBLEM triggers without limitation because CTrigger class should check Dependencies.
If a user has a lot of triggers in PROBLEM state (7000 triggers in PROBLEM state on test lab) this operation is much expensive.



 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
In cases $limit could be string, this variable would be irrelevant, but loose comparison would not work as intended.
This line were removed in r35086.

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.

jelisejev

1. True, let's leave it as is.
2. Sorry, I meant max(). But ok, performance won't be an issue here.

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 - ZBX-6714

Generated at Fri Mar 29 03:57:26 EET 2024 using Jira 9.12.4#9120004-sha1:625303b708afdb767e17cb2838290c41888e9ff0.