Skip to content

Fix: Large table causes sql error #2242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Koc wants to merge 4 commits into
base: feature/speedup-value-formatting
Choose a base branch
from
+497 −140
Draft

Fix: Large table causes sql error #2242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da25bd7
Fix: Large table causes sql error
Koc Dec 28, 2025
b8f18e9
Fix: Large table causes sql error
Koc Dec 28, 2025
2095d85
Fix: Large table causes sql error (cache cells)
Koc Dec 29, 2025
79508e9
Fix: Large table causes sql error (cache cells)
Koc Dec 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion appinfo/info.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ Share your tables and views with users and groups within your cloud.
Have a good time and manage whatever you want.
]]></description>
<version>2.0.0-alpha.1</version>
<version>2.0.0-alpha.2</version>
<licence>agpl</licence>
<author mail="florian.steffens@nextcloud.com">Florian Steffens</author>
<namespace>Tables</namespace>
Expand Down Expand Up @@ -57,6 +57,7 @@ Have a good time and manage whatever you want.
<post-migration>
<step>OCA\Tables\Migration\NewDbStructureRepairStep</step>
<step>OCA\Tables\Migration\DbRowSleeveSequence</step>
<step>OCA\Tables\Migration\CacheSleeveCells</step>
</post-migration>
</repair-steps>
<commands>
Expand Down
197 changes: 58 additions & 139 deletions lib/Db/Row2Mapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
use DateTime;
use DateTimeImmutable;
use OCA\Tables\Constants\UsergroupType;
use OCA\Tables\Db\RowLoader\CachedRowLoader;
use OCA\Tables\Db\RowLoader\NormalizedRowLoader;
use OCA\Tables\Errors\InternalError;
use OCA\Tables\Errors\NotFoundError;
use OCA\Tables\Helper\ColumnsHelper;
Expand All @@ -18,12 +20,8 @@
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
use OCP\AppFramework\Db\TTransactional;
use OCP\DB\Exception;
use OCP\DB\IResult;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\Server;
use Psr\Container\ContainerExceptionInterface;
use Psr\Container\NotFoundExceptionInterface;
use Psr\Log\LoggerInterface;
use Throwable;

Expand All @@ -38,15 +36,26 @@ class Row2Mapper {
protected ColumnMapper $columnMapper;

private ColumnsHelper $columnsHelper;
/**
* @var array<RowLoader\RowLoader::LOADER_*, RowLoader\RowLoader>
*/
private array $rowLoaders;

public function __construct(?string $userId, IDBConnection $db, LoggerInterface $logger, UserHelper $userHelper, RowSleeveMapper $rowSleeveMapper, ColumnsHelper $columnsHelper, ColumnMapper $columnMapper) {
public function __construct(?string $userId, IDBConnection $db, LoggerInterface $logger, UserHelper $userHelper, RowSleeveMapper $rowSleeveMapper, ColumnsHelper $columnsHelper, ColumnMapper $columnMapper,
NormalizedRowLoader $normalizedRowLoader,
CachedRowLoader $cachedRowLoader,
) {
$this->rowSleeveMapper = $rowSleeveMapper;
$this->userId = $userId;
$this->db = $db;
$this->logger = $logger;
$this->userHelper = $userHelper;
$this->columnsHelper = $columnsHelper;
$this->columnMapper = $columnMapper;
$this->rowLoaders = [
RowLoader\RowLoader::LOADER_NORMALIZED => $normalizedRowLoader,
RowLoader\RowLoader::LOADER_CACHED => $cachedRowLoader,
];
}

/**
Expand All @@ -58,7 +67,7 @@ public function delete(Row2 $row): Row2 {
$this->db->beginTransaction();
try {
foreach ($this->columnsHelper->columns as $columnType) {
$this->getCellMapperFromType($columnType)->deleteAllForRow($row->getId());
$this->columnsHelper->getCellMapperFromType($columnType)->deleteAllForRow($row->getId());
}
$this->rowSleeveMapper->deleteById($row->getId());
$this->db->commit();
Expand Down Expand Up @@ -176,7 +185,7 @@ public function findAll(array $showColumnIds, int $tableId, ?int $limit = null,
$wantedRowIdsArray = $this->getWantedRowIds($userId, $tableId, $filter, $sort, $limit, $offset);

// Get rows without SQL sorting
$rows = $this->getRows($wantedRowIdsArray, $showColumnIds);
$rows = $this->getRows($wantedRowIdsArray, $showColumnIds, RowLoader\RowLoader::LOADER_CACHED);

// Sort rows in PHP to preserve the order from getWantedRowIds
return $this->sortRowsByIds($rows, $wantedRowIdsArray);
Expand All @@ -189,61 +198,16 @@ public function findAll(array $showColumnIds, int $tableId, ?int $limit = null,
/**
* @param array $rowIds
* @param array $columnIds
* @param RowLoader\RowLoader::LOADER_* $loader
* @return Row2[]
* @throws InternalError
*/
private function getRows(array $rowIds, array $columnIds): array {
$qb = $this->db->getQueryBuilder();

$qbSqlForColumnTypes = null;
foreach ($this->columnsHelper->columns as $columnType) {
$qbTmp = $this->db->getQueryBuilder();
$qbTmp->select('row_id', 'column_id', 'last_edit_at', 'last_edit_by')
->selectAlias($qb->expr()->castColumn('value', IQueryBuilder::PARAM_STR), 'value');

// This is not ideal but I cannot think of a good way to abstract this away into the mapper right now
// Ideally we dynamically construct this query depending on what additional selects the column type requires
// however the union requires us to match the exact number of selects for each column type
if ($columnType === Column::TYPE_USERGROUP) {
$qbTmp->selectAlias($qb->expr()->castColumn('value_type', IQueryBuilder::PARAM_STR), 'value_type');
} else {
$qbTmp->selectAlias($qbTmp->createFunction('NULL'), 'value_type');
}

$qbTmp
->from('tables_row_cells_' . $columnType)
->where($qb->expr()->in('column_id', $qb->createNamedParameter($columnIds, IQueryBuilder::PARAM_INT_ARRAY, ':columnIds')))
->andWhere($qb->expr()->in('row_id', $qb->createNamedParameter($rowIds, IQueryBuilder::PARAM_INT_ARRAY, ':rowsIds')));

if ($qbSqlForColumnTypes) {
$qbSqlForColumnTypes .= ' UNION ALL ' . $qbTmp->getSQL() . ' ';
} else {
$qbSqlForColumnTypes = '(' . $qbTmp->getSQL();
}
}
$qbSqlForColumnTypes .= ')';

$qb->select('row_id', 'column_id', 'created_by', 'created_at', 't1.last_edit_by', 't1.last_edit_at', 'value', 'table_id')
// Also should be more generic (see above)
->addSelect('value_type')
->from($qb->createFunction($qbSqlForColumnTypes), 't1')
->innerJoin('t1', 'tables_row_sleeves', 'rs', 'rs.id = t1.row_id');

try {
$result = $qb->executeQuery();
} catch (Exception $e) {
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage(), );
}

try {
$sleeves = $this->rowSleeveMapper->findMultiple($rowIds);
} catch (Exception $e) {
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage());
private function getRows(array $rowIds, array $columnIds, string $loader = RowLoader\RowLoader::LOADER_NORMALIZED ): array {
if (empty($rowIds)) {
return [];
}

return $this->parseEntities($result, $sleeves);
return $this->rowLoaders[$loader]->getRows($rowIds, $columnIds);
}

/**
Expand Down Expand Up @@ -619,61 +583,6 @@ private function getSqlOperator(string $operator, IQueryBuilder $qb, string $col
}
}

/**
* @param IResult $result
* @param RowSleeve[] $sleeves
* @return Row2[]
* @throws InternalError
*/
private function parseEntities(IResult $result, array $sleeves): array {
$rows = [];
foreach ($sleeves as $sleeve) {
$rows[$sleeve->getId()] = new Row2();
$rows[$sleeve->getId()]->setId($sleeve->getId());
$rows[$sleeve->getId()]->setCreatedBy($sleeve->getCreatedBy());
$rows[$sleeve->getId()]->setCreatedAt($sleeve->getCreatedAt());
$rows[$sleeve->getId()]->setLastEditBy($sleeve->getLastEditBy());
$rows[$sleeve->getId()]->setLastEditAt($sleeve->getLastEditAt());
$rows[$sleeve->getId()]->setTableId($sleeve->getTableId());
}

$rowValues = [];
$keyToColumnId = [];
$keyToRowId = [];
$cellMapperCache = [];

while ($rowData = $result->fetch()) {
if (!isset($rowData['row_id'], $rows[$rowData['row_id']])) {
break;
}

$column = $this->columnMapper->find($rowData['column_id']);
$columnType = $column->getType();
if (!isset($cellMapperCache[$columnType])) {
$cellMapperCache[$columnType] = $this->getCellMapperFromType($columnType);
}
$value = $cellMapperCache[$columnType]->formatRowData($column, $rowData);
$compositeKey = (string)$rowData['row_id'] . ',' . (string)$rowData['column_id'];
if ($cellMapperCache[$columnType]->hasMultipleValues()) {
if (array_key_exists($compositeKey, $rowValues)) {
$rowValues[$compositeKey][] = $value;
} else {
$rowValues[$compositeKey] = [$value];
}
} else {
$rowValues[$compositeKey] = $value;
}
$keyToColumnId[$compositeKey] = $rowData['column_id'];
$keyToRowId[$compositeKey] = $rowData['row_id'];
}

foreach ($rowValues as $compositeKey => $value) {
$rows[$keyToRowId[$compositeKey]]->addCell($keyToColumnId[$compositeKey], $value);
}

return array_values($rows);
}

/**
* @throws InternalError
*/
Expand All @@ -698,9 +607,12 @@ public function insert(Row2 $row): Row2 {
}

// write all cells to its db-table
$cachedCells = [];
foreach ($row->getData() as $cell) {
$this->insertCell($rowSleeve->getId(), $cell['columnId'], $cell['value'], $rowSleeve->getLastEditAt(), $rowSleeve->getLastEditBy());
$cachedCells[$cell['columnId']] = $this->insertCell($rowSleeve->getId(), $cell['columnId'], $cell['value'], $rowSleeve->getLastEditAt(), $rowSleeve->getLastEditBy());
}
$rowSleeve->setCachedCellsArray($cachedCells);
$this->rowSleeveMapper->update($rowSleeve);

return $row;
}
Expand All @@ -717,9 +629,9 @@ public function update(Row2 $row): Row2 {

// update meta data for sleeve
try {
$sleeve = $this->rowSleeveMapper->find($row->getId());
$this->updateMetaData($sleeve);
$this->rowSleeveMapper->update($sleeve);
$rowSleeve = $this->rowSleeveMapper->find($row->getId());
$this->updateMetaData($rowSleeve);
$this->rowSleeveMapper->update($rowSleeve);
} catch (DoesNotExistException|MultipleObjectsReturnedException|Exception $e) {
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage());
Expand All @@ -728,9 +640,12 @@ public function update(Row2 $row): Row2 {
$this->columnMapper->preloadColumns(array_column($changedCells, 'columnId'));

// write all changed cells to its db-table
$cachedCells = $rowSleeve->getCachedCellsArray();
foreach ($changedCells as $cell) {
$this->insertOrUpdateCell($sleeve->getId(), $cell['columnId'], $cell['value']);
$cachedCells[$cell['columnId']] = $this->insertOrUpdateCell($rowSleeve->getId(), $cell['columnId'], $cell['value']);
}
$rowSleeve->setCachedCellsArray($cachedCells);
$this->rowSleeveMapper->update($rowSleeve);

return $row;
}
Expand Down Expand Up @@ -782,9 +697,11 @@ private function updateMetaData($entity, bool $setCreate = false, ?string $lastE
/**
* Insert a cell to its specific db-table
*
* @return array<string, mixed> normalized cell data
*
* @throws InternalError
*/
private function insertCell(int $rowId, int $columnId, $value, ?string $lastEditAt = null, ?string $lastEditBy = null): void {
private function insertCell(int $rowId, int $columnId, $value, ?string $lastEditAt = null, ?string $lastEditBy = null): array {
try {
$column = $this->columnMapper->find($columnId);
} catch (DoesNotExistException $e) {
Expand All @@ -794,7 +711,7 @@ private function insertCell(int $rowId, int $columnId, $value, ?string $lastEdit

// insert new cell
$cellMapper = $this->getCellMapper($column);

$cachedCell = [];
try {
$cellClassName = 'OCA\Tables\Db\RowCell' . ucfirst($column->getType());
if ($cellMapper->hasMultipleValues()) {
Expand All @@ -806,6 +723,7 @@ private function insertCell(int $rowId, int $columnId, $value, ?string $lastEdit
$this->updateMetaData($cell, false, $lastEditAt, $lastEditBy);
$cellMapper->applyDataToEntity($column, $cell, $val);
$cellMapper->insert($cell);
$cachedCell[] = $cellMapper->toArray($cell);
}
} else {
/** @var RowCellSuper $cell */
Expand All @@ -815,65 +733,66 @@ private function insertCell(int $rowId, int $columnId, $value, ?string $lastEdit
$this->updateMetaData($cell, false, $lastEditAt, $lastEditBy);
$cellMapper->applyDataToEntity($column, $cell, $value);
$cellMapper->insert($cell);
$cachedCell = $cellMapper->toArray($cell);
}
} catch (Exception $e) {
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new InternalError('Failed to insert column: ' . $e->getMessage(), 0, $e);
}

return $cachedCell;
}

/**
* @param RowCellSuper $cell
* @param RowCellMapperSuper $mapper
* @param mixed $value the value should be parsed to the correct format within the row service
* @param Column $column
*
* @return array<string, mixed> normalized cell data
* @throws InternalError
*/
private function updateCell(RowCellSuper $cell, RowCellMapperSuper $mapper, $value, Column $column): void {
$this->getCellMapper($column)->applyDataToEntity($column, $cell, $value);
private function updateCell(RowCellSuper $cell, RowCellMapperSuper $mapper, $value, Column $column): array {
$cellMapper = $this->getCellMapper($column);
$cellMapper->applyDataToEntity($column, $cell, $value);
$this->updateMetaData($cell);
$mapper->updateWrapper($cell);

return $cellMapper->toArray($cell);
}

/**
* @return array<string, mixed> normalized cell data
* @throws InternalError
*/
private function insertOrUpdateCell(int $rowId, int $columnId, $value): void {
private function insertOrUpdateCell(int $rowId, int $columnId, $value): array {
$column = $this->columnMapper->find($columnId);
$cellMapper = $this->getCellMapper($column);
$cachedCell = [];
try {
if ($cellMapper->hasMultipleValues()) {
$this->atomic(function () use ($cellMapper, $rowId, $columnId, $value) {
$this->atomic(function () use ($cellMapper, $rowId, $columnId, $value, &$cachedCell) {
// For a usergroup field with mutiple values, each is inserted as a new cell
// we need to delete all previous cells for this row and column, otherwise we get duplicates
$cellMapper->deleteAllForColumnAndRow($columnId, $rowId);
$this->insertCell($rowId, $columnId, $value);
$cachedCell = $this->insertCell($rowId, $columnId, $value);
}, $this->db);
} else {
$cell = $cellMapper->findByRowAndColumn($rowId, $columnId);
$this->updateCell($cell, $cellMapper, $value, $column);
$cachedCell = $this->updateCell($cell, $cellMapper, $value, $column);
}
} catch (DoesNotExistException) {
$this->insertCell($rowId, $columnId, $value);
$cachedCell = $this->insertCell($rowId, $columnId, $value);
} catch (MultipleObjectsReturnedException|Exception $e) {
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage());
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage(), $e->getCode(), $e);
}
}

private function getCellMapper(Column $column): RowCellMapperSuper {
return $this->getCellMapperFromType($column->getType());
return $cachedCell;
}

private function getCellMapperFromType(string $columnType): RowCellMapperSuper {
$cellMapperClassName = 'OCA\Tables\Db\RowCell' . ucfirst($columnType) . 'Mapper';
/** @var RowCellMapperSuper $cellMapper */
try {
return Server::get($cellMapperClassName);
} catch (NotFoundExceptionInterface|ContainerExceptionInterface $e) {
$this->logger->error($e->getMessage(), ['exception' => $e]);
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage());
}
private function getCellMapper(Column $column): RowCellMapperSuper {
return $this->columnsHelper->getCellMapperFromType($column->getType());
}

/**
Expand Down
4 changes: 4 additions & 0 deletions lib/Db/RowCellMapperSuper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ public function applyDataToEntity(Column $column, RowCellSuper $cell, $data): vo
$cell->setValue($data);
}

public function toArray(RowCellSuper $cell): array {
return ['value' => $cell->getValue()];
}

public function getDbParamType() {
return IQueryBuilder::PARAM_STR;
}
Expand Down
7 changes: 7 additions & 0 deletions lib/Db/RowCellUsergroupMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ public function formatRowData(Column $column, array $row) {
];
}

public function toArray(RowCellSuper $cell): array {
return [
'value' => $cell->getValue(),
'value_type' => $cell->getValueType(),
];
}

public function hasMultipleValues(): bool {
return true;
}
Expand Down
Loading
Loading