Skip to content
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

!!! TASK: Split dimensionspacepoints into separate table to reduce data duplication #4790

Merged
merged 12 commits into from
Mar 18, 2024
Merged

!!! TASK: Split dimensionspacepoints into separate table to reduce data duplication #4790

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3cf14ed
TASK: Normalize DimensionSpacePoints to separate table
kitsunet Nov 30, 2023
cdff3d3
Fix behat test
kitsunet Mar 12, 2024
074f319
Fix ProjectionIntegrityViolationDetector
kitsunet Mar 13, 2024
266ecca
Make stan happier
kitsunet Mar 13, 2024
d468dd3
Some code cleanup
kitsunet Mar 13, 2024
0c18558
Make stan happy
kitsunet Mar 13, 2024
71504a5
DimensionSpacePoints should be created in transaction
kitsunet Mar 14, 2024
42235cc
Rename to DimensionSpacePointsRepository
kitsunet Mar 15, 2024
5f477cc
Merge branch '9.0' into task/split-dimensionspacepoints
kitsunet Mar 15, 2024
4f9273d
Merge remote-tracking branch 'origin/9.0' into task/split-dimensionsp…
kitsunet Mar 15, 2024
94cbe4f
Merge remote-tracking branch 'origin/9.0' into task/split-dimensionsp…
kitsunet Mar 18, 2024
882b572
Improved usage of DimensionSpacePointsRepository
kitsunet Mar 18, 2024
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
1 change: 0 additions & 1 deletion ...lAdapter/Tests/Behavior/Features/Bootstrap/ProjectionIntegrityViolationDetectionTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,6 @@ private function transformDatasetToHierarchyRelationRecord(array $dataset): arra

return [
'contentstreamid' => $dataset['contentStreamId'],
'dimensionspacepoint' => $dimensionSpacePoint->toJson(),
'dimensionspacepointhash' => $dimensionSpacePoint->hash,
'parentnodeanchor' => $parentHierarchyRelation !== null ? $parentHierarchyRelation['childnodeanchor'] : 9999999,
'childnodeanchor' => $childHierarchyRelation !== null ? $childHierarchyRelation['childnodeanchor'] : 8888888,
Expand Down
17 changes: 10 additions & 7 deletions Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Neos\ContentGraph\DoctrineDbalAdapter\Domain\Projection\NodeRecord;
use Neos\ContentGraph\DoctrineDbalAdapter\Domain\Projection\NodeRelationAnchorPoint;
use Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\ContentGraph;
use Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\DimensionSpacePointsRepository;
use Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\NodeFactory;
use Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\ProjectionContentGraph;
use Neos\ContentRepository\Core\DimensionSpace\DimensionSpacePoint;
Expand Down Expand Up @@ -83,6 +84,8 @@ final class DoctrineDbalContentGraphProjection implements ProjectionInterface, W

private DbalCheckpointStorage $checkpointStorage;

private DimensionSpacePointsRepository $dimensionSpacePointsRepository;

public function __construct(
private readonly DbalClientInterface $dbalClient,
private readonly NodeFactory $nodeFactory,
Expand All @@ -96,6 +99,8 @@ public function __construct(
$this->tableNamePrefix . '_checkpoint',
self::class
);

$this->dimensionSpacePointsRepository = new DimensionSpacePointsRepository($this->dbalClient->getConnection(), $this->tableNamePrefix);
}

protected function getProjectionContentGraph(): ProjectionContentGraph
Expand Down Expand Up @@ -174,6 +179,7 @@ private function truncateDatabaseTables(): void
$connection->executeQuery('TRUNCATE table ' . $this->tableNamePrefix . '_node');
$connection->executeQuery('TRUNCATE table ' . $this->tableNamePrefix . '_hierarchyrelation');
$connection->executeQuery('TRUNCATE table ' . $this->tableNamePrefix . '_referencerelation');
$connection->executeQuery('TRUNCATE table ' . $this->tableNamePrefix . '_dimensionspacepoints');
}

public function canHandle(EventInterface $event): bool
Expand Down Expand Up @@ -599,7 +605,6 @@ private function whenContentStreamWasForked(ContentStreamWasForked $event): void
childnodeanchor,
`name`,
position,
dimensionspacepoint,
dimensionspacepointhash,
subtreetags,
contentstreamid
Expand All @@ -609,7 +614,6 @@ private function whenContentStreamWasForked(ContentStreamWasForked $event): void
h.childnodeanchor,
h.name,
h.position,
h.dimensionspacepoint,
h.dimensionspacepointhash,
h.subtreetags,
"' . $event->newContentStreamId->value . '" AS contentstreamid
Expand Down Expand Up @@ -947,6 +951,8 @@ private function copyReferenceRelations(
private function whenDimensionSpacePointWasMoved(DimensionSpacePointWasMoved $event): void
{
$this->transactional(function () use ($event) {
$this->dimensionSpacePointsRepository->insertDimensionSpacePoint($event->target);

// the ordering is important - we first update the OriginDimensionSpacePoints, as we need the
// hierarchy relations for this query. Then, we update the Hierarchy Relations.

Expand Down Expand Up @@ -985,7 +991,6 @@ function (NodeRecord $nodeRecord) use ($event) {
'
UPDATE ' . $this->tableNamePrefix . '_hierarchyrelation h
SET
h.dimensionspacepoint = :newDimensionSpacePoint,
h.dimensionspacepointhash = :newDimensionSpacePointHash
WHERE
h.dimensionspacepointhash = :originalDimensionSpacePointHash
Expand All @@ -994,7 +999,6 @@ function (NodeRecord $nodeRecord) use ($event) {
[
'originalDimensionSpacePointHash' => $event->source->hash,
'newDimensionSpacePointHash' => $event->target->hash,
'newDimensionSpacePoint' => $event->target->toJson(),
'contentStreamId' => $event->contentStreamId->value
]
);
Expand All @@ -1004,6 +1008,8 @@ function (NodeRecord $nodeRecord) use ($event) {
private function whenDimensionShineThroughWasAdded(DimensionShineThroughWasAdded $event): void
{
$this->transactional(function () use ($event) {
$this->dimensionSpacePointsRepository->insertDimensionSpacePoint($event->target);

// 1) hierarchy relations
$this->getDatabaseConnection()->executeStatement(
'
Expand All @@ -1013,7 +1019,6 @@ private function whenDimensionShineThroughWasAdded(DimensionShineThroughWasAdded
`name`,
position,
subtreetags,
dimensionspacepoint,
dimensionspacepointhash,
contentstreamid
)
Expand All @@ -1023,7 +1028,6 @@ private function whenDimensionShineThroughWasAdded(DimensionShineThroughWasAdded
h.name,
h.position,
h.subtreetags,
:newDimensionSpacePoint AS dimensionspacepoint,
:newDimensionSpacePointHash AS dimensionspacepointhash,
h.contentstreamid
FROM
Expand All @@ -1034,7 +1038,6 @@ private function whenDimensionShineThroughWasAdded(DimensionShineThroughWasAdded
'contentStreamId' => $event->contentStreamId->value,
'sourceDimensionSpacePointHash' => $event->source->hash,
'newDimensionSpacePointHash' => $event->target->hash,
'newDimensionSpacePoint' => $event->target->toJson(),
]
);
});
Expand Down
6 changes: 5 additions & 1 deletion Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjectionFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace Neos\ContentGraph\DoctrineDbalAdapter;

use Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\DimensionSpacePointsRepository;
use Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\NodeFactory;
use Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\ProjectionContentGraph;
use Neos\ContentRepository\Core\Factory\ProjectionFactoryDependencies;
Expand Down Expand Up @@ -40,13 +41,16 @@ public function build(
$projectionFactoryDependencies->contentRepositoryId
);



return new ContentGraphProjection(
new DoctrineDbalContentGraphProjection(
$this->dbalClient,
new NodeFactory(
$projectionFactoryDependencies->contentRepositoryId,
$projectionFactoryDependencies->nodeTypeManager,
$projectionFactoryDependencies->propertyConverter
$projectionFactoryDependencies->propertyConverter,
new DimensionSpacePointsRepository($this->dbalClient->getConnection(), $tableNamePrefix)
),
kitsunet marked this conversation as resolved.
Show resolved Hide resolved
$projectionFactoryDependencies->contentRepositoryId,
$projectionFactoryDependencies->nodeTypeManager,
Expand Down
21 changes: 14 additions & 7 deletions Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphSchemaBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public function buildSchema(AbstractSchemaManager $schemaManager): Schema
$this->createNodeTable(),
$this->createHierarchyRelationTable(),
$this->createReferenceRelationTable(),
$this->createDimensionSpacePointsTable()
]);
}

Expand All @@ -36,7 +37,6 @@ private function createNodeTable(): Table
$table = new Table($this->tableNamePrefix . '_node', [
DbalSchemaFactory::columnForNodeAnchorPoint('relationanchorpoint')->setAutoincrement(true),
DbalSchemaFactory::columnForNodeAggregateId('nodeaggregateid')->setNotnull(false),
DbalSchemaFactory::columnForDimensionSpacePoint('origindimensionspacepoint')->setNotnull(false),
DbalSchemaFactory::columnForDimensionSpacePointHash('origindimensionspacepointhash')->setNotnull(false),
DbalSchemaFactory::columnForNodeTypeName('nodetypename'),
(new Column('properties', Type::getType(Types::TEXT)))->setNotnull(true)->setCustomSchemaOption('collation', self::DEFAULT_TEXT_COLLATION),
Expand All @@ -56,24 +56,31 @@ private function createNodeTable(): Table
private function createHierarchyRelationTable(): Table
{
$table = new Table($this->tableNamePrefix . '_hierarchyrelation', [
(new Column('name', Type::getType(Types::STRING)))->setLength(255)->setNotnull(false)->setCustomSchemaOption('collation', self::DEFAULT_TEXT_COLLATION),
(new Column('name', Type::getType(Types::STRING)))->setLength(255)->setNotnull(false)->setCustomSchemaOption('charset', 'ascii')->setCustomSchemaOption('collation', 'ascii_general_ci'),
dlubitz marked this conversation as resolved.
Show resolved Hide resolved
(new Column('position', Type::getType(Types::INTEGER)))->setNotnull(true),
DbalSchemaFactory::columnForContentStreamId('contentstreamid')->setNotnull(true),
DbalSchemaFactory::columnForDimensionSpacePoint('dimensionspacepoint')->setNotnull(true),
DbalSchemaFactory::columnForDimensionSpacePointHash('dimensionspacepointhash')->setNotnull(true),
DbalSchemaFactory::columnForNodeAnchorPoint('parentnodeanchor'),
DbalSchemaFactory::columnForNodeAnchorPoint('childnodeanchor'),
(new Column('subtreetags', Type::getType(Types::JSON)))->setDefault('{}'),
]);

return $table
->addIndex(['childnodeanchor'])
->addIndex(['contentstreamid'])
->addIndex(['parentnodeanchor'])
->addIndex(['contentstreamid', 'childnodeanchor', 'dimensionspacepointhash'])
->setPrimaryKey(['childnodeanchor', 'contentstreamid', 'dimensionspacepointhash', 'parentnodeanchor'])
->addIndex(['contentstreamid', 'dimensionspacepointhash']);
Comment on lines 68 to 70
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol after debugging i found out that this change here slows down the replay of ContentStreamWasForked events dramatically

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm, this fixes replay for me:
image

I would suggest that we revert that part for now as a "hotfix". Afterwards, we could investigate why that kills the database

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #5009

}

private function createDimensionSpacePointsTable(): Table
{
$table = new Table($this->tableNamePrefix . '_dimensionspacepoints', [
DbalSchemaFactory::columnForDimensionSpacePointHash('hash')->setNotnull(true),
DbalSchemaFactory::columnForDimensionSpacePoint('dimensionspacepoint')->setNotnull(true)
]);

return $table
->setPrimaryKey(['hash']);
}

private function createReferenceRelationTable(): Table
{
$table = new Table($this->tableNamePrefix . '_referencerelation', [
Expand Down
5 changes: 4 additions & 1 deletion Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Projection/HierarchyRelation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

use Doctrine\DBAL\Connection;
use Neos\ContentRepository\Core\DimensionSpace\DimensionSpacePoint;
use Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\DimensionSpacePointsRepository;
use Neos\ContentRepository\Core\Projection\ContentGraph\NodeTags;
use Neos\ContentRepository\Core\SharedModel\Node\NodeName;
use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId;
Expand Down Expand Up @@ -44,12 +45,14 @@ public function __construct(
*/
public function addToDatabase(Connection $databaseConnection, string $tableNamePrefix): void
{
$dimensionSpacePoints = new DimensionSpacePointsRepository($databaseConnection, $tableNamePrefix);
$dimensionSpacePoints->insertDimensionSpacePoint($this->dimensionSpacePoint);

$databaseConnection->insert($tableNamePrefix . '_hierarchyrelation', [
'parentnodeanchor' => $this->parentNodeAnchor->value,
'childnodeanchor' => $this->childNodeAnchor->value,
'name' => $this->name?->value,
'contentstreamid' => $this->contentStreamId->value,
'dimensionspacepoint' => $this->dimensionSpacePoint->toJson(),
'dimensionspacepointhash' => $this->dimensionSpacePointHash,
'position' => $this->position,
'subtreetags' => json_encode($this->subtreeTags, JSON_THROW_ON_ERROR | JSON_FORCE_OBJECT),
Expand Down
7 changes: 5 additions & 2 deletions Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Projection/NodeRecord.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
namespace Neos\ContentGraph\DoctrineDbalAdapter\Domain\Projection;

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
use Doctrine\DBAL\Types\Types;
use Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\DimensionSpacePointsRepository;
use Neos\ContentRepository\Core\Feature\NodeModification\Dto\SerializedPropertyValues;
use Neos\ContentRepository\Core\NodeType\NodeTypeName;
use Neos\ContentRepository\Core\Projection\ContentGraph\Timestamps;
Expand Down Expand Up @@ -55,7 +57,6 @@ public function updateToDatabase(Connection $databaseConnection, string $tableNa
$tableNamePrefix . '_node',
[
'nodeaggregateid' => $this->nodeAggregateId->value,
'origindimensionspacepoint' => json_encode($this->originDimensionSpacePoint),
'origindimensionspacepointhash' => $this->originDimensionSpacePointHash,
'properties' => json_encode($this->properties),
'nodetypename' => $this->nodeTypeName->value,
Expand Down Expand Up @@ -138,9 +139,11 @@ public static function createNewInDatabase(
?NodeName $nodeName,
Timestamps $timestamps,
): self {
$dimensionSpacePoints = new DimensionSpacePointsRepository($databaseConnection, $tableNamePrefix);
$dimensionSpacePoints->insertDimensionSpacePointByHashAndCoordinates($originDimensionSpacePointHash, $originDimensionSpacePoint);

$databaseConnection->insert($tableNamePrefix . '_node', [
'nodeaggregateid' => $nodeAggregateId->value,
'origindimensionspacepoint' => json_encode($originDimensionSpacePoint),
'origindimensionspacepointhash' => $originDimensionSpacePointHash,
'properties' => json_encode($properties),
'nodetypename' => $nodeTypeName->value,
Expand Down
22 changes: 14 additions & 8 deletions ...tGraph.DoctrineDbalAdapter/src/Domain/Projection/ProjectionIntegrityViolationDetector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ public function hierarchyIntegrityIsProvided(): Result
}

$invalidlyHashedHierarchyRelationRecords = $this->client->getConnection()->executeQuery(
'SELECT * FROM ' . $this->tableNamePrefix . '_hierarchyrelation
WHERE dimensionspacepointhash != MD5(dimensionspacepoint)'
'SELECT * FROM ' . $this->tableNamePrefix . '_hierarchyrelation h LEFT JOIN ' . $this->tableNamePrefix . '_dimensionspacepoints dsp ON dsp.hash = h.dimensionspacepointhash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what the purpose for this check is. But after it's not fully the same as before. So we just check if the entry for a given hash is present in _dimensionspacepoints. But before we also checked if the md5-hashed value is equal to the hash.

Copy link
Member Author

@kitsunet kitsunet Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm, we can't know if the hash is the right one though, either it exists in the DSP table or it's invalid, if it exists in there IMHO we can assume it matches the dimension coordinates. We could add a separate check that reads all DSP entries and compares hash and getting the hash from the coordinates in the same row if we really want to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with keeping it as it is. Just wanted to point out this difference.

HAVING dsp.dimensionspacepoint IS NULL'
)->fetchAllAssociative();

foreach ($invalidlyHashedHierarchyRelationRecords as $record) {
Expand Down Expand Up @@ -118,6 +118,12 @@ public function siblingsAreDistinctlySorted(): Result
HAVING COUNT(position) > 1'
);

if ($ambiguouslySortedHierarchyRelationRecords->columnCount() === 0) {
return $result;
}

$dimensionSpacePoints = $this->findProjectedDimensionSpacePoints();

foreach ($ambiguouslySortedHierarchyRelationRecords as $hierarchyRelationRecord) {
$ambiguouslySortedNodeRecords = $this->client->getConnection()->executeQuery(
'SELECT nodeaggregateid
Expand All @@ -133,7 +139,7 @@ public function siblingsAreDistinctlySorted(): Result
return $record['nodeaggregateid'];
}, $ambiguouslySortedNodeRecords))
. ' are ambiguously sorted in content stream ' . $hierarchyRelationRecord['contentstreamid']
. ' and dimension space point ' . $hierarchyRelationRecord['dimensionspacepoint'],
. ' and dimension space point ' . $dimensionSpacePoints[$hierarchyRelationRecord['dimensionspacepointhash']]?->toJson(),
self::ERROR_CODE_SIBLINGS_ARE_AMBIGUOUSLY_SORTED
));
}
Expand Down Expand Up @@ -516,7 +522,7 @@ public function childNodeCoverageIsASubsetOfParentNodeCoverage(): Result
$result = new Result();
foreach ($this->findProjectedContentStreamIds() as $contentStreamId) {
$excessivelyCoveringNodeRecords = $this->client->getConnection()->executeQuery(
'SELECT n.nodeaggregateid, c.dimensionspacepoint
'SELECT n.nodeaggregateid, c.dimensionspacepointhash
FROM ' . $this->tableNamePrefix . '_hierarchyrelation c
INNER JOIN ' . $this->tableNamePrefix . '_node n
ON c.childnodeanchor = n.relationanchorpoint
Expand All @@ -535,7 +541,7 @@ public function childNodeCoverageIsASubsetOfParentNodeCoverage(): Result
$result->addError(new Error(
'Node aggregate ' . $excessivelyCoveringNodeRecord['nodeaggregateid']
. ' in content stream ' . $contentStreamId->value
. ' covers dimension space point ' . $excessivelyCoveringNodeRecord['dimensionspacepoint']
. ' covers dimension space point hash ' . $excessivelyCoveringNodeRecord['dimensionspacepointhash']
. ' but its parent does not.',
self::ERROR_CODE_CHILD_NODE_COVERAGE_IS_NO_SUBSET_OF_PARENT_NODE_COVERAGE
));
Expand All @@ -553,7 +559,7 @@ public function allNodesCoverTheirOrigin(): Result
$result = new Result();
foreach ($this->findProjectedContentStreamIds() as $contentStreamId) {
$nodeRecordsWithMissingOriginCoverage = $this->client->getConnection()->executeQuery(
'SELECT nodeaggregateid, origindimensionspacepoint
'SELECT nodeaggregateid, origindimensionspacepointhash
FROM ' . $this->tableNamePrefix . '_node n
INNER JOIN ' . $this->tableNamePrefix . '_hierarchyrelation h
ON h.childnodeanchor = n.relationanchorpoint
Expand All @@ -579,7 +585,7 @@ public function allNodesCoverTheirOrigin(): Result
$result->addError(new Error(
'Node aggregate ' . $nodeRecord['nodeaggregateid']
. ' in content stream ' . $contentStreamId->value
. ' does not cover its origin dimension space point ' . $nodeRecord['origindimensionspacepoint']
. ' does not cover its origin dimension space point hash ' . $nodeRecord['origindimensionspacepointhash']
. '.',
self::ERROR_CODE_NODE_DOES_NOT_COVER_ITS_ORIGIN
));
Expand Down Expand Up @@ -615,7 +621,7 @@ protected function findProjectedContentStreamIds(): iterable
protected function findProjectedDimensionSpacePoints(): DimensionSpacePointSet
{
$records = $this->client->getConnection()->executeQuery(
'SELECT DISTINCT dimensionspacepoint FROM ' . $this->tableNamePrefix . '_hierarchyrelation'
'SELECT dimensionspacepoint FROM ' . $this->tableNamePrefix . '_dimensionspacepoints'
)->fetchAllAssociative();
dlubitz marked this conversation as resolved.
Show resolved Hide resolved

$records = array_map(function (array $record) {
Expand Down
Loading
Loading