[ZBX-9989] Redundant SQLs to history/trends tables when building graphs Created: 2015 Oct 24 Updated: 2017 Sep 16 Resolved: 2016 Sep 28 |
|
Status: | Closed |
Project: | ZABBIX BUGS AND ISSUES |
Component/s: | Frontend (F) |
Affects Version/s: | 1.8.22, 2.0.15, 2.2.10, 2.4.6, 3.0.0alpha3 |
Fix Version/s: | 3.2.0alpha1 |
Type: | Incident report | Priority: | Minor |
Reporter: | Oleksii Zagorskyi | Assignee: | Unassigned |
Resolution: | Fixed | Votes: | 0 |
Labels: | graphs, performance, sql | ||
Remaining Estimate: | Not Specified | ||
Time Spent: | Not Specified | ||
Original Estimate: | Not Specified |
Issue Links: |
|
Description |
When getting values from history|trends, zabbix frontend does absolutely unneeded extra query to one of history|trends table for every item on a graph. if (!$trendsEnabled || (($item['history'] * SEC_PER_DAY) > (time() - ($this->from_time + $this->period / 2)) && ($this->period / $this->sizeX) <= (ZBX_MAX_TREND_DIFF / ZBX_GRAPH_MAX_SKIP_CELL))) { $this->dataFrom = 'history'; array_push($sql_arr, 'SELECT itemid,'.$calc_field.' AS i,'. 'COUNT(*) AS count,AVG(value) AS avg,MIN(value) as min,'. 'MAX(value) AS max,MAX(clock) AS clock'. ' FROM history '. ' WHERE itemid='.zbx_dbstr($this->items[$i]['itemid']). ' AND clock>='.zbx_dbstr($from_time). ' AND clock<='.zbx_dbstr($to_time). ' GROUP BY itemid,'.$calc_field , 'SELECT itemid,'.$calc_field.' AS i,'. 'COUNT(*) AS count,AVG(value) AS avg,MIN(value) AS min,'. 'MAX(value) AS max,MAX(clock) AS clock'. ' FROM history_uint '. ' WHERE itemid='.zbx_dbstr($this->items[$i]['itemid']). ' AND clock>='.zbx_dbstr($from_time). ' AND clock<='.zbx_dbstr($to_time). ' GROUP BY itemid,'.$calc_field ); } else { $this->dataFrom = 'trends'; array_push($sql_arr, 'SELECT itemid,'.$calc_field.' AS i,'. 'SUM(num) AS count,AVG(value_avg) AS avg,MIN(value_min) AS min,'. 'MAX(value_max) AS max,MAX(clock) AS clock'. ' FROM trends'. ' WHERE itemid='.zbx_dbstr($this->items[$i]['itemid']). ' AND clock>='.zbx_dbstr($from_time). ' AND clock<='.zbx_dbstr($to_time). ' GROUP BY itemid,'.$calc_field , 'SELECT itemid,'.$calc_field.' AS i,'. 'SUM(num) AS count,AVG(value_avg) AS avg,MIN(value_min) AS min,'. 'MAX(value_max) AS max,MAX(clock) AS clock'. ' FROM trends_uint '. ' WHERE itemid='.zbx_dbstr($this->items[$i]['itemid']). ' AND clock>='.zbx_dbstr($from_time). ' AND clock<='.zbx_dbstr($to_time). ' GROUP BY itemid,'.$calc_field ); $this->items[$i]['delay'] = max($this->items[$i]['delay'], SEC_PER_HOUR); } I tried to investigate when this logic has been added in the past to try to understand why. Results: Additional unconditional select from "history_uint" table added 2006-03-27 in r2711 in /trunk/frontends/php/include/classes/graph.inc.php Additional unconditional select from "trends_uint" table has been added 2008-07-29 in r5845 /trunk/frontends/php/include/classes/chart.inc.php As for me, it looks like it was introduced NOT to show values on graphs even when someone has changed item information type after some history has been already collected. I think it was just a simplest implementation. The additional SQL produces extra and unneeded load on database and this is the most critical point. As for current zabbix versions, the possibly positive point (to show values on graph event after changing information type) does NOT work! Housekeeping collected data for items after changing information type discussed in ZBX-9278 |
Comments |
Comment by Oleksii Zagorskyi [ 2015 Oct 25 ] |
(1) Additionally to the issue I posted in description here is additional one, which probably worth to be fixed in the same development. Have for example a graph with 2 float and 1 integer items. SQL (0.000519): SELECT MIN(ht.c) AS min_clock FROM ( SELECT MIN(ht.clock) AS c FROM trends ht WHERE ht.itemid='23299' UNION ALL SELECT MIN(ht.clock) AS c FROM trends ht WHERE ht.itemid='23300' UNION ALL SELECT MIN(ht.clock) AS c FROM trends ht WHERE ht.itemid='23301' ) ht charts.php:162 → CView->render() → include() → CScreenChart->get() → get_min_itemclock_by_graphid() → get_min_itemclock_by_itemid() → DBselect() in /zab/www-dev/2.4/include/graphs.inc.php:269 SQL (0.000576): SELECT MIN(ht.c) AS min_clock FROM ( SELECT MIN(ht.clock) AS c FROM trends ht WHERE ht.itemid='23299' UNION ALL SELECT MIN(ht.clock) AS c FROM trends ht WHERE ht.itemid='23300' UNION ALL SELECT MIN(ht.clock) AS c FROM trends ht WHERE ht.itemid='23301' UNION ALL SELECT MIN(ht.clock) AS c FROM trends_uint ht WHERE ht.itemid='23299' UNION ALL SELECT MIN(ht.clock) AS c FROM trends_uint ht WHERE ht.itemid='23300' UNION ALL SELECT MIN(ht.clock) AS c FROM trends_uint ht WHERE ht.itemid='23301') ht charts.php:162 → CView->render() → include() → CScreenChart->get() → get_min_itemclock_by_graphid() → get_min_itemclock_by_itemid() → DBselect() in /zab/www-dev/2.4/include/graphs.inc.php:269 Questions are - Why 2 SQLs? Why do we request a table which should not contain data for current information type? Patched version for the same graph produces next result, note 1 SQL instead of 2: SQL (0.000527): SELECT MIN(ht.c) AS min_clock FROM ( SELECT MIN(ht.clock) AS c FROM trends ht WHERE ht.itemid IN ('23300','23301') UNION ALL SELECT MIN(ht.clock) AS c FROM trends_uint ht WHERE ht.itemid='23299' ) ht charts.php:162 → CView->render() → include() → CScreenChart->get() → get_min_itemclock_by_graphid() → get_min_itemclock_by_itemid() → DBselect() in /zab/www-dev/2.4/include/graphs.inc.php:267 I did the test on a production system too (partitioned postgres 9.3, 15Gb RAM, ~50Gb trends data, 9 months long (from trends) graph "Zabbix server performance" with two items uint+float) Another graph "Zabbix internal process busy" with 9 float items" the same 9 months: Here is the patch: Index: include/graphs.inc.php =================================================================== --- include/graphs.inc.php (revision 56332) +++ include/graphs.inc.php (working copy) @@ -258,19 +258,14 @@ default: $sqlFrom = 'history'; } - - foreach ($itemIds as $itemId) { - $sqlUnions[] = 'SELECT MIN(ht.clock) AS c FROM '.$sqlFrom.' ht WHERE ht.itemid='.zbx_dbstr($itemId); - } - - $dbMin = DBfetch(DBselect( - 'SELECT MIN(ht.c) AS min_clock'. - ' FROM ('.implode(' UNION ALL ', $sqlUnions).') ht' + + $sqlUnions[] = 'SELECT MIN(ht.clock) AS c FROM '.$sqlFrom.' ht WHERE '.dbConditionInt('ht.itemid', $items); + } + $dbMin = DBfetch(DBselect( + 'SELECT MIN(ht.c) AS min_clock'. + ' FROM ('.implode(' UNION ALL ', $sqlUnions).') ht' )); - - $min = $min ? min($min, $dbMin['min_clock']) : $dbMin['min_clock']; - } - + $min = $dbMin['min_clock']; // in case DB clock column is corrupted having negative numbers, return min clock from max possible history storage return ($min > 0) ? $min : $result; } sasha It must be reverted. See (5) for more details. REOPENED gunarspujats RESOLVED in r57141 sasha CLOSED |
Comment by Gunars Pujats (Inactive) [ 2015 Oct 27 ] |
(2) No translation strings changed. sasha CLOSED |
Comment by Gunars Pujats (Inactive) [ 2015 Oct 27 ] |
RESOLVED in development branch svn://svn.zabbix.com/branches/dev/ZBX-9989 |
Comment by Oleksii Zagorskyi [ 2015 Oct 27 ] |
(3) File CPieGraphDraw.php was not updated, but it contains the same queries as described in issue description. gunarspujats RESOLVED in r56402. sasha CLOSED |
Comment by Oleksii Zagorskyi [ 2015 Oct 27 ] |
(4) Interesting, why in the SQL in one place "AS" replaced to lowercase "as" ? gunarspujats RESOLVED in r56402. sasha CLOSED |
Comment by Alexander Vladishev [ 2015 Dec 09 ] |
(5) You did performance tests after changes in frontends/php/include/graphs.inc.php? These SQL statements were already optimized in zalex_ua When investigated this issue I definitely used zabbix versions after the zalex_ua as discussed with Sasha, this change may break optimization from sasha CLOSED |
Comment by Alexander Vladishev [ 2015 Dec 10 ] |
(6) Please accept my minor code improvements in r57136. gunarspujats CLOSED |
Comment by Alexander Vladishev [ 2016 Feb 11 ] |
(7) Two SQL requests can be removed from function get_min_itemclock_by_itemid() sasha RESOLVED in r58399, r58400 gunarspujats CLOSED |
Comment by Alexander Vladishev [ 2016 Feb 11 ] |
Successfully tested! But close (7) before merging. |
Comment by Gunars Pujats (Inactive) [ 2016 Feb 16 ] |
Fixed in:
|
Comment by Alexander Vladishev [ 2016 Feb 16 ] |
(8) Documentation: gunarspujats Upgrade notes updated. sasha Thanks! CLOSED |