Merge pull request #1312 from nextcloud/fix/stable27/chunk-mariadb

[stable27] fix(db): also chunk MariaDB deletes

Author: nickvergessen

lib/AppInfo/Application.php

@@ -56,6 +56,7 @@
 use OCP\User\Events\UserDeletedEvent;
 use OCP\Util;
 use Psr\Container\ContainerInterface;
+use Psr\Log\LoggerInterface;
 
 class Application extends App implements IBootstrap {
 	public const APP_ID = 'activity';
@@ -107,7 +108,8 @@ public function register(IRegistrationContext $context): void {
 		$context->registerService(Data::class, function (ContainerInterface $c) {
 			return new Data(
 				$c->get(IManager::class),
-				$c->get('ActivityConnectionAdapter')
+				$c->get('ActivityConnectionAdapter'),
+				$c->get(LoggerInterface::class),
 			);
 		});
 

lib/Data.php

@@ -1,4 +1,6 @@
 <?php
+
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -32,6 +34,7 @@
 use OCP\Activity\IManager;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IDBConnection;
+use Psr\Log\LoggerInterface;
 
 /**
  * @brief Class for managing the data in the activities
@@ -48,14 +51,16 @@ class Data {
 
 	/** @var ?IQueryBuilder */
 	protected $insertMail;
+	private LoggerInterface $logger;
 
 	/**
 	 * @param IManager $activityManager
 	 * @param IDBConnection $connection
 	 */
-	public function __construct(IManager $activityManager, IDBConnection $connection) {
+	public function __construct(IManager $activityManager, IDBConnection $connection, LoggerInterface $logger) {
 		$this->activityManager = $activityManager;
 		$this->connection = $connection;
+		$this->logger = $logger;
 	}
 
 	/**
@@ -368,9 +373,16 @@ public function expire($expireDays = 365) {
 	 *                      'field' => 'value'  => `field` = 'value'
 	 *    'field' => array('value', 'operator') => `field` operator 'value'
 	 */
-	public function deleteActivities($conditions) {
-		$delete = $this->connection->getQueryBuilder();
-		$delete->delete('activity');
+	public function deleteActivities($conditions): void {
+		$platform = $this->connection->getDatabasePlatform();
+		if ($platform instanceof MySQLPlatform) {
+			$this->logger->debug('Choosing chunked activity delete for MySQL/MariaDB', ['app' => 'activity']);
+			$this->deleteActivitiesForMySQL($conditions);
+			return;
+		}
+		$this->logger->debug('Choosing regular activity delete', ['app' => 'activity']);
+		$deleteQuery = $this->connection->getQueryBuilder();
+		$deleteQuery->delete('activity');
 
 		foreach ($conditions as $column => $comparison) {
 			if (is_array($comparison)) {
@@ -381,22 +393,14 @@ public function deleteActivities($conditions) {
 				$value = $comparison;
 			}
 
-			$delete->andWhere($delete->expr()->comparison($column, $operation, $delete->createNamedParameter($value)));
+			$deleteQuery->andWhere($deleteQuery->expr()->comparison($column, $operation, $deleteQuery->createNamedParameter($value)));
 		}
 
-		// Add galera safe delete chunking if using mysql
-		// Stops us hitting wsrep_max_ws_rows when large row counts are deleted
-		if ($this->connection->getDatabasePlatform() instanceof MySQLPlatform) {
-			// Then use chunked delete
-			$max = 100000;
-			$delete->setMaxResults($max);
-			do {
-				$deleted = $delete->executeStatement();
-			} while ($deleted === $max);
-		} else {
-			// Dont use chunked delete - let the DB handle the large row count natively
-			$delete->executeStatement();
-		}
+
+
+
+		// Dont use chunked delete - let the DB handle the large row count natively
+		$deleteQuery->executeStatement();
 	}
 
 	public function getById(int $activityId): ?IEvent {
@@ -467,4 +471,48 @@ public function getActivitySince(string $user, int $since, bool $byOthers) {
 
 		return $query->execute()->fetch();
 	}
+
+	/**
+	 * Add galera safe delete chunking if using mysql
+	 * Stops us hitting wsrep_max_ws_rows when large row counts are deleted
+	 *
+	 * @param array $conditions
+	 * @return void
+	 */
+	private function deleteActivitiesForMySQL(array $conditions): void {
+		$query = $this->connection->getQueryBuilder();
+		$query->select('activity_id')
+			->from('activity');
+
+		foreach ($conditions as $column => $comparison) {
+			if (is_array($comparison)) {
+				$operation = $comparison[1] ?? '=';
+				$value = $comparison[0];
+			} else {
+				$operation = '=';
+				$value = $comparison;
+			}
+			$query->where($query->expr()->comparison($column, $operation, $query->createNamedParameter($value)));
+		}
+
+		$query->setMaxResults(10000);
+		$result = $query->executeQuery();
+		$count = $result->rowCount();
+		if ($count === 0) {
+			return;
+		}
+		$ids = array_map(static function (array $id) {
+			return (int)$id[0];
+		}, $result->fetchAll(\PDO::FETCH_NUM));
+		$result->closeCursor();
+
+		$deleteQuery = $this->connection->getQueryBuilder();
+		$deleteQuery->delete('activity');
+		$deleteQuery->where($deleteQuery->expr()->in('activity_id', $deleteQuery->createParameter('ids'), IQueryBuilder::PARAM_INT_ARRAY));
+		$deleteQuery->setParameter('ids', $ids, IQueryBuilder::PARAM_INT_ARRAY);
+		$queryResult = $deleteQuery->executeStatement();
+		if ($queryResult === 10000) {
+			$this->deleteActivitiesForMySQL($conditions);
+		}
+	}
 }

psalm.xml

@@ -17,6 +17,7 @@
 		<UndefinedClass>
 			<errorLevel type="suppress">
 				<referencedClass name="Doctrine\DBAL\Platforms\MySQLPlatform" />
+				<referencedClass name="Doctrine\DBAL\Platforms\AbstractMySQLPlatform" />
 				<referencedClass name="Doctrine\DBAL\Types\Types" />
 				<referencedClass name="OC" />
 				<referencedClass name="OC\Core\Command\Base" />

tests/Controller/APIv1ControllerTest.php

@@ -41,6 +41,7 @@
 use OCP\IRequest;
 use OCP\IUserSession;
 use OCP\RichObjectStrings\IValidator;
+use Psr\Log\LoggerInterface;
 
 /**
  * Class APIv1Test
@@ -106,7 +107,8 @@ protected function tearDown(): void {
 	protected function cleanUp(): void {
 		$data = new Data(
 			$this->createMock(IManager::class),
-			\OC::$server->getDatabaseConnection()
+			\OC::$server->getDatabaseConnection(),
+			$this->createMock(LoggerInterface::class)
 		);
 
 		$this->deleteUser($data, 'activity-api-user1');
@@ -220,7 +222,10 @@ public function testGet(string $user, int $start, int $count, array $expected):
 			->method('getUID')
 			->willReturn($user);
 
-		$data = new Data($activityManager, \OC::$server->getDatabaseConnection());
+		$data = new Data($activityManager,
+			\OC::$server->getDatabaseConnection(),
+			$this->createMock(LoggerInterface::class)
+		);
 
 		$controller = new APIv1Controller(
 			'activity',

tests/DataDeleteActivitiesTest.php

@@ -27,12 +27,12 @@
 use OCA\Activity\BackgroundJob\ExpireActivities;
 use OCA\Activity\Data;
 use OCP\Activity\IExtension;
+use OCP\Activity\IManager;
 use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\BackgroundJob\IJobList;
 use OCP\DB\IPreparedStatement;
 use OCP\IConfig;
-use OCP\IUserSession;
-use OCP\Activity\IManager;
-use OCP\BackgroundJob\IJobList;
+use Psr\Log\LoggerInterface;
 
 /**
  * Class DataDeleteActivitiesTest
@@ -74,7 +74,7 @@ protected function setUp(): void {
 		$this->data = new Data(
 			$this->createMock(IManager::class),
 			\OC::$server->getDatabaseConnection(),
-			$this->createMock(IUserSession::class)
+			$this->createMock(LoggerInterface::class)
 		);
 	}
 

tests/DataTest.php

@@ -31,6 +31,7 @@
 use OCP\IUserSession;
 use OCP\Util;
 use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\NullLogger;
 
 /**
  * Class DataTest
@@ -54,17 +55,22 @@ class DataTest extends TestCase {
 	/** @var IManager */
 	protected $realActivityManager;
 
+	/** @var NullLogger */
+	protected $logger;
+
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->activityLanguage = Util::getL10N('activity', 'en');
 		$activityManager = $this->createMock(IManager::class);
 		$this->dbConnection = \OC::$server->get(IDBConnection::class);
 		$this->realActivityManager = \OC::$server->get(IManager::class);
+		$this->logger = \OC::$server->get(NullLogger::class);
 
 		$this->data = new Data(
 			$activityManager,
-			$this->dbConnection
+			$this->dbConnection,
+			$this->logger
 		);
 	}