From cbb8e240ca5ce235e695a5218ccb588bbc924d10 Mon Sep 17 00:00:00 2001
From: Christian Kuhn <lolli@schwarzbu.ch>
Date: Wed, 21 Feb 2024 12:30:36 +0100
Subject: [PATCH] [TASK] Avoid runtime caching pages in DataHandler
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The DataHandler has a major structural flaw by not handing
over known record state to sub methods. It thus tends to
fetch and process the same table/uid combination over and
over again to figure out details like permissions.

This is of course expensive: DH tends to fire lots of DB
calls. Over the years, a series of caches have been added
in order to suppress those DB calls and mitigate DB load.

Obvious cache layers:
* $this->recInsertAccessCache
* $this->isRecordInWebMount_Cache
* $this->isInWebMount_Cache
* $this->pageCache
* $this->runtimeCache 'checkRecordUpdateAccess_'
* $this->runtimeCache 'doesRecordExist_pageLookUp_'

As an additional gem that increases complexity, runtimeCache
is currently abused to transfer state between DH sub instances.
Various lower level caches kick in, too, e.g. from BackendUtility.

There is a problem with these caches, in particular those that
suppress record state DB calls: DH itself changes this state all
the time by adding, deleting and changing records. This makes
cache eviction a crucial aspect when dealing with them. This
however is hard and not properly done at various places. The true
solution is to hand over more 'known correct state at this time'
to sub methods instead, which voids more and more of these layers.
We've done this with the 'workspace discard' methods in v11 already
with great success: The DB load has been reduced significantly,
and there are no cache eviction issues in this area anymore.

The patch removes a cache in doesRecordExist_pageLookUp(),
which unblocks one aspect of a patch to fix #92604. For now,
we actively accept a (slight, but not critical) performance
penalty due to repeated DB queries, mainly hit when importing
many records. Looking at use cases, we can and should instead
refactor the call chains to hand over 'known' page record state
directly. We will establish this with upcoming v13 patches and
will suppress more queries than this cache avoided in the first
place.

Note the penalty introduced with this patch can be mostly
suppressed for imports using DH->bypassAccessCheckForRecords=true,
which is a good idea in many cases anyway at the moment, if
the import is an admin-only operation.

Resolves: #103172
Related: #79610
Related: #92604
Releases: main
Change-Id: Ie25cb4563c0bd09eb2d0060aafeae26bea7a54f5
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/83057
Tested-by: core-ci <typo3@b13.com>
Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Reviewed-by: Sascha Nowak <typo3@saschanowak.me>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de>
Reviewed-by: Stefan Bürk <stefan@buerk.tech>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Stefan Bürk <stefan@buerk.tech>
---
 .../core/Classes/DataHandling/DataHandler.php | 20 ++-----------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/typo3/sysext/core/Classes/DataHandling/DataHandler.php b/typo3/sysext/core/Classes/DataHandling/DataHandler.php
index 6ce9b4719384..62b373f8feb8 100644
--- a/typo3/sysext/core/Classes/DataHandling/DataHandler.php
+++ b/typo3/sysext/core/Classes/DataHandling/DataHandler.php
@@ -7109,24 +7109,12 @@ class DataHandler implements LoggerAwareInterface
      * @param int $id Page id
      * @param int $perms Permission integer
      * @param array $columns Columns to select
-     * @return bool|array
      * @internal
      * @see doesRecordExist()
      */
-    protected function doesRecordExist_pageLookUp($id, $perms, $columns = ['uid'])
+    protected function doesRecordExist_pageLookUp($id, $perms, $columns = ['uid']): array|false
     {
         $permission = new Permission($perms);
-        $cacheId = md5('doesRecordExist_pageLookUp_' . $id . '_' . $perms . '_' . implode(
-            '_',
-            $columns
-        ) . '_' . (string)$this->admin);
-
-        // If result is cached, return it
-        $cachedResult = $this->runtimeCache->get($cacheId);
-        if (!empty($cachedResult)) {
-            return $cachedResult;
-        }
-
         $queryBuilder = $this->connectionPool->getQueryBuilderForTable('pages');
         $this->addDeleteRestriction($queryBuilder->getRestrictions()->removeAll());
         $queryBuilder
@@ -7147,11 +7135,7 @@ class DataHandler implements LoggerAwareInterface
                 $queryBuilder->createNamedParameter(0, Connection::PARAM_INT)
             ));
         }
-
-        $row = $queryBuilder->executeQuery()->fetchAssociative();
-        $this->runtimeCache->set($cacheId, $row);
-
-        return $row;
+        return $queryBuilder->executeQuery()->fetchAssociative();
     }
 
     /**
-- 
GitLab