Count the online visitors in SQL instead of in PHP - #65
Draft
boo-code wants to merge 1 commit into
Draft
Conversation
The Online Visitors tile only ever used the NUMBER of rows the query returned: it ran executeS() and then read NumRows(). So every online visitor was carried into PHP with five columns each and thrown away, and the database was asked to sort them by date for an order nothing reads. Both branches now count in SQL. The two joins to page and page_type only produced the page name the hook discards and both join on a primary key, so removing them cannot change the count; connections_page is the one table that can match a connection more than once, which is what the GROUP BY collapsed and COUNT(DISTINCT) still does. Reading the value with getValue() also replaces a pair of Db::getInstance() lookups that asked two different instances for the query and for its row count.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
executeS()and then readNumRows(). Every online visitor was therefore carried into PHP with five columns each and discarded, and the database was asked to sort them by date for an order nothing reads. Both branches now count in SQL. The two joins topageandpage_typeonly produced the page name the hook discards and both join on a primary key, so removing them cannot change the count;connections_pageis the one table that can match a connection more than once, which is what theGROUP BYcollapsed andCOUNT(DISTINCT)still does. Reading the value withgetValue()also replaces a pair ofDb::getInstance()lookups that asked one instance for the query and another for its row count.PS_STATSDATA_CUSTOMER_PAGESVIEWSon, browse the front office from a few guest sessions and check the Online Visitors tile still shows the same number as before this change.EXPLAINon the query the hook builds should no longer reportUsing temporary; Using filesort, and the plan should have three tables instead of five.What this does and does not fix
It does not, on its own, fix the slow query reported in PrestaShop/PrestaShop#34227, and the PR should
not be read as claiming that. Measured on a seeded scratch database (400k
connections,1.2M
connections_page, 400kguest, MySQL 8.4, medians of 5):At the default
DASHACTIVITY_VISITOR_ONLINEof 30 minutes this is worth about 6%, because thedominant cost is elsewhere:
ps_connections_pagecarries no index but its primary key(id_connections, id_page, time_start), so thetime_startwindow cannot be seeked and the planfull-scans
ps_connections. That is a coredb_structure.sqlchange; the measurement for it is onPrestaShop/PrestaShop#39960, which is open, approved and already adds indexes to that table.
What this PR is worth on its own merits: the plan loses
Using temporary; Using filesortand twojoin steps, the result stops being transferred to PHP row by row, and the gain grows with the number
of visitors actually online - 1717 ms to 830 ms when the window covers the whole table.
Correctness
COUNT(DISTINCT c.id_connections)is the exact equivalent of counting theGROUP BY c.id_connectionsrows.
connections_pageis the only table that can match a connection more than once -page,page_typeandguestare all joined on their primary key - and thecp.time_end IS NULLandtime_startpredicates sit in theWHERE, so nothing about the count depended on the LEFT-ness ofthe joins that are removed.
Checked against a control rather than asserted, since a plain
COUNT(*)would have been wrong:The other branch (
PS_STATSDATA_CUSTOMER_PAGESVIEWSoff, which is the default) has noGROUP BYandno join that can fan out, so
COUNT(*)is its equivalent; it already uses the existingdate_addkey (
type=range, rows=54) and is not the slow one.Both branches were run through PrestaShop's own DB layer on 9.2.0 with dashactivity 2.1.2 and return
an integer;
Shop::addSqlRestriction(false, 'c')renders asAND c.id_shop IN (1), which is theshape the benchmarks used.
dashactivityhas no PHPUnit harness (tests/isindex.php,php,phpstan.sh), so the evidence above stands in for a unit test.Side effect worth noting
The
GROUP BY c.id_connectionsselecting a non-aggregatedpt.nameis only legal because PrestaShopblanks
sql_modeon connect (DbPDO.php:120,DbMySQLi.php:71); under the MySQL defaultONLY_FULL_GROUP_BYit is an error, not a slow query. The rewrite has no non-aggregated column anddoes not depend on that.
Related
PrestaShop/dashactivity#62(mine) touches this file ~70 lines away, no overlap.PrestaShop/dashactivity#63(nicosomb) is +357/-12 on this file but almost purely additive aroundline 80; it does not touch this query.