Update visit only when needed (#15869)

* update idvisitor only when needed

* better implementation

* fix tests

3 files changed, 59 insertions, 3 deletions

diff --git a/core/Tracker/GoalManager.php b/core/Tracker/GoalManager.php
index 5353e68a8f..168f9a708b 100644
--- a/core/Tracker/GoalManager.php
+++ b/core/Tracker/GoalManager.php
@@ -877,10 +877,22 @@ class GoalManager
 
     private function getGoalFromVisitor(VisitProperties $visitProperties, Request $request, $action)
     {
+        $lastVisitTime = $visitProperties->getProperty('visit_last_action_time');
+        if (!$lastVisitTime) {
+            $lastVisitTime = $request->getCurrentTimestamp(); // fallback in case visit_last_action_time is not set
+        }
+
+        if (!empty($lastVisitTime) && is_numeric($lastVisitTime)) {
+            // visit last action time might be 2020-05-05 00:00:00
+            // we want it to prevent this being converted to a timestamp of 2020
+            // resulting in some day in 1970
+            $lastVisitTime = Date::getDatetimeFromTimestamp($lastVisitTime);
+        }
+
         $goal = array(
             'idvisit'     => $visitProperties->getProperty('idvisit'),
             'idvisitor'   => $visitProperties->getProperty('idvisitor'),
-            'server_time' => Date::getDatetimeFromTimestamp($visitProperties->getProperty('visit_last_action_time')),
+            'server_time' => $lastVisitTime,
         );
 
         $visitDimensions = VisitDimension::getAllDimensions();
diff --git a/core/Tracker/Visit.php b/core/Tracker/Visit.php
index d3de0015ca..1284a70a19 100644
--- a/core/Tracker/Visit.php
+++ b/core/Tracker/Visit.php
@@ -244,6 +244,15 @@ class Visit implements VisitInterface
             $processor->onExistingVisit($valuesToUpdate, $this->visitProperties, $this->request);
         }
 
+        // we we remove values that haven't actually changed and are still the same when comparing to the initially
+        // selected visit row. In best case this avoids the update completely. Eg when there is a bulk tracking request
+        // of many content impressions. Then it will update the visit in the first request of the bulk request, and
+        // all other visits that have same visit_last_action_time etc will be ignored and won't issue an update SQL
+        // statement at all avoiding potential lock wait time when too many requests try to update the same visit at
+        // same time
+        $visitorRecognizer = StaticContainer::get(VisitorRecognizer::class);
+        $valuesToUpdate = $visitorRecognizer->removeUnchangedValues($this->visitProperties, $valuesToUpdate);
+
         $this->updateExistingVisit($valuesToUpdate);
 
         $this->visitProperties->setProperty('visit_last_action_time', $this->request->getCurrentTimestamp());
@@ -546,8 +555,7 @@ class Visit implements VisitInterface
     {
         // Might update the idvisitor when it was forced or overwritten for this visit
         if (strlen($this->visitProperties->getProperty('idvisitor')) == Tracker::LENGTH_BINARY_ID) {
-            $binIdVisitor = $this->visitProperties->getProperty('idvisitor');
-            $valuesToUpdate['idvisitor'] = $binIdVisitor;
+            $valuesToUpdate['idvisitor'] = $this->visitProperties->getProperty('idvisitor');
         }
 
         return $valuesToUpdate;
diff --git a/core/Tracker/VisitorRecognizer.php b/core/Tracker/VisitorRecognizer.php
index f43ee5378a..51569b24e3 100644
--- a/core/Tracker/VisitorRecognizer.php
+++ b/core/Tracker/VisitorRecognizer.php
@@ -20,6 +20,12 @@ use Piwik\Tracker\Visit\VisitProperties;
 class VisitorRecognizer
 {
     /**
+     * Set when a visit was found. Stores the original values of the row that is currently stored in the DB when
+     * the visit was selected.
+     */
+    const KEY_ORIGINAL_VISIT_ROW = 'originalVisit';
+
+    /**
      * Local variable cache for the getVisitFieldsPersist() method.
      *
      * @var array
@@ -105,6 +111,7 @@ class VisitorRecognizer
         if ($visitRow
             && count($visitRow) > 0
         ) {
+            $visitProperties->setProperty(self::KEY_ORIGINAL_VISIT_ROW, $visitRow);
             $visitProperties->setProperty('idvisitor', $visitRow['idvisitor']);
             $visitProperties->setProperty('user_id', $visitRow['user_id']);
 
@@ -121,6 +128,35 @@ class VisitorRecognizer
         }
     }
 
+    public function removeUnchangedValues(VisitProperties $visitProperties, $visit)
+    {
+        $originalRow = $visitProperties->getProperty(self::KEY_ORIGINAL_VISIT_ROW);
+
+        if (empty($originalRow)) {
+            return $visit;
+        }
+
+        if (!empty($originalRow['idvisitor'])
+            && !empty($visit['idvisitor'])
+            && bin2hex($originalRow['idvisitor']) === bin2hex($visit['idvisitor'])) {
+            unset($visit['idvisitor']);
+        }
+
+        $fieldsToCompareValue = array('user_id', 'visit_last_action_time', 'visit_total_time');
+        foreach ($fieldsToCompareValue as $field) {
+            if (!empty($originalRow[$field])
+                && !empty($visit[$field])
+                && $visit[$field] == $originalRow[$field]) {
+                // we can't use === eg for visit_total_time which may be partially an integer and sometimes a string
+                // because we check for !empty things should still work as expected though
+                // (eg we wouldn't compare false with 0)
+                unset($visit[$field]);
+            }
+        }
+
+        return $visit;
+    }
+
     public function updateVisitPropertiesFromLastVisitRow(VisitProperties $visitProperties)
     {
         // These values will be used throughout the request