[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:
Duplicate
is duplicated by ZBX-6792 Graphs should not request data from b... Closed

 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.
Take a look to /frontends/php/include/classes/graphdraw/CLineGraphDraw.php:235-281, here it is:

			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
svn log - replaced "union" command by sql arrays, for compatability of old MySQL (Eugene)
Worth to mention that "history_uint" table support has been added ~5 months earlier in r2313. svn log - added table history_uint (Alexei)

Additional unconditional select from "trends_uint" table has been added 2008-07-29 in r5845 /trunk/frontends/php/include/classes/chart.inc.php
svn log - [DEV-197] added support of trends_uint

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.
In any case, currently those 2 SQLs per single item looks very suspicious when you investigate SQLs performed by zabbix frontend.

The additional SQL produces extra and unneeded load on database and this is the most critical point.
Removing those unneeded SQLs can really speed up graphs building.

As for current zabbix versions, the possibly positive point (to show values on graph event after changing information type) does NOT work!
Well, it DOES work, but only for very short period (up to 1 hour) after changing information type - until the item collected history and inserted single trends value.
(why insertion to trends is a key point - because most likely all numeric items have trends configured longer than history and frontend checks for min(clock) of longest period)
After single value is inserted to a trends* table - frontend will limit possible time periods (slider, pre-selected periods) accordingly to to "min(clock) from current-table-type".
So frontend users will not see the old collected data at all and not will be able to select old period to display.

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.
On my workstation, when displaying such a graph (charts.php) in debug section I see these SQL (added line breaks for better readability):

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?
Should be fixed, IMO.

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)
default: 2 SQLs together: ~0,045 seconds
patched: 1 SQL ~0,021 seconds
(page was refreshed 4 times, analysed trends data supposedly was in postgres/os caches, at least partially).

Another graph "Zabbix internal process busy" with 9 float items" the same 9 months:
default 1 SQL: ~0,070 seconds
patched 1 SQL: ~0,060 seconds

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.
REOPENED

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 ZBX-6804.

zalex_ua When investigated this issue I definitely used zabbix versions after the ZBX-6804 fix.
In the test it can be noticed that I used 2.4 branch, it should be ~2.4.6+
2nd test done on a production installation with 2.4.6 release.

zalex_ua as discussed with Sasha, this change may break optimization from ZBX-6804.
So I need to reconsider it.

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:

  • pre-3.1.0 (trunk) r58493.
Comment by Alexander Vladishev [ 2016 Feb 16 ]

(8) Documentation:

gunarspujats Upgrade notes updated.

sasha Thanks! CLOSED

Generated at Thu Apr 18 07:17:12 EEST 2024 using Jira 9.12.4#9120004-sha1:625303b708afdb767e17cb2838290c41888e9ff0.