Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 9 additions & 2 deletions src/Database/Adapter/SQL.php
Original file line number Diff line number Diff line change
Expand Up @@ -862,6 +862,7 @@ public function getSequences(string $collection, array $documents): array
$documentIds = [];
$keys = [];
$binds = [];
$tenants = [];

foreach ($documents as $i => $document) {
if (empty($document->getSequence())) {
Expand All @@ -873,7 +874,13 @@ public function getSequences(string $collection, array $documents): array
$keys[] = $key;

if ($this->sharedTables) {
$binds[':_tenant_'.$i] = $document->getTenant();
$tenant = $document->getTenant();

// One placeholder per distinct tenant
if (!\in_array($tenant, $tenants, true)) {
$binds[':_tenant_'.\count($tenants)] = $tenant;
$tenants[] = $tenant;
}
}
}
}
Expand All @@ -888,7 +895,7 @@ public function getSequences(string $collection, array $documents): array
SELECT _uid, _id
FROM {$this->getSQLTable($collection)}
WHERE {$this->quote('_uid')} IN ({$placeholders})
{$this->getTenantQuery($collection, tenantCount: \count($documentIds))}
{$this->getTenantQuery($collection, tenantCount: \count($tenants))}
";

$stmt = $this->getPDO()->prepare($sql);
Expand Down
36 changes: 33 additions & 3 deletions src/Database/Database.php
Original file line number Diff line number Diff line change
Expand Up @@ -5866,6 +5866,10 @@ public function createDocuments(

$time = DateTime::now();
$modified = 0;
$hasRelationships = !empty(\array_filter(
$collection->getAttribute('attributes', []),
fn ($attribute) => $attribute['type'] === self::VAR_RELATIONSHIP
));

foreach ($documents as $document) {
$createdAt = $document->getCreatedAt();
Expand Down Expand Up @@ -5922,7 +5926,11 @@ public function createDocuments(
? $this->adapter->skipDuplicates($insert)
: $insert();

$batch = $this->adapter->getSequences($collection->getId(), $batch);
// A SELECT per batch, read only by relationship population and by whatever the
// caller does with the documents $onNext hands it. Skip it when neither applies.
if ($onNext !== null || $hasRelationships) {
$batch = $this->adapter->getSequences($collection->getId(), $batch);
}

if (!$this->inBatchRelationshipPopulation && $this->resolveRelationships) {
$batch = $this->silent(fn () => $this->populateDocumentsRelationships($batch, $collection, $this->relationshipFetchDepth));
Expand Down Expand Up @@ -7366,6 +7374,10 @@ public function upsertDocumentsWithIncrease(
$created = 0;
$updated = 0;
$seenIds = [];
$hasRelationships = !empty(\array_filter(
$collectionAttributes,
fn ($attribute) => $attribute['type'] === self::VAR_RELATIONSHIP
));

// Batch-fetch existing documents in one query instead of N individual getDocument() calls.
// tenantPerDocument: group ids by tenant and run one find() per tenant under withTenant,
Expand Down Expand Up @@ -7611,7 +7623,21 @@ public function upsertDocumentsWithIncrease(
$chunk
)));

$batch = $this->adapter->getSequences($collection->getId(), $batch);
// Every row that already existed was read into $existingDocs above, so its
// sequence is already in hand and does not need fetching a second time.
foreach ($batch as $index => $doc) {
if (empty($doc->getSequence()) && !empty($chunk[$index]->getOld()->getSequence())) {
$doc->setAttribute('$sequence', $chunk[$index]->getOld()->getSequence());
}
}

// Nothing leaves this method except through $onNext -- the return value is a
// count -- so a caller that passes none never sees these documents, and the work
// that finishes them has no reader. Bulk writers such as the usage/stats workers
// upsert several batches a second and read nothing back.
if ($onNext !== null || $hasRelationships) {
$batch = $this->adapter->getSequences($collection->getId(), $batch);
}

foreach ($chunk as $change) {
if ($change->getOld()->isEmpty()) {
Expand All @@ -7635,7 +7661,11 @@ public function upsertDocumentsWithIncrease(
}
}

if ($hasOperators) {
// Refetching only exists to hand computed operator values back to the caller, and
// $onNext is the only way anything leaves this method -- the return value is a
// count. $hasOperators still has to reflect the batch, because the decode below
// keys off it.
if ($hasOperators && $onNext !== null) {
$batch = $this->refetchDocuments($collection, $batch);
}

Expand Down
87 changes: 87 additions & 0 deletions tests/e2e/Adapter/Scopes/DocumentTests.php
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,14 @@ public function testCreateDocuments(): void
$this->assertEquals(5, $document->getAttribute('integer'));
$this->assertIsInt($document->getAttribute('bigint'));
$this->assertEquals(9223372036854775807, $document->getAttribute('bigint'));

// The insert does not return the sequence for the rows it wrote, so it is looked
// up afterwards. $onNext is the only way these documents reach the caller.
$this->assertNotEmpty($document->getSequence());
$this->assertEquals(
$database->getDocument($collection, $document->getId())->getSequence(),
$document->getSequence()
);
}

$documents = $database->find($collection, [
Expand Down Expand Up @@ -1851,6 +1859,85 @@ public function testPreserveSequenceUpsert(): void
$database->deleteCollection($collectionName);
}

/**
* upsertDocuments() carries the sequence of every row it already read across to the
* written document, so the follow-up getSequences() lookup only covers the rows that
* were genuinely new. That leaves the batch it receives interleaved -- some documents
* carry a sequence, some do not -- and the tenant placeholders it binds must line up
* with the ones its SQL declares regardless of where the gaps fall.
*/
public function testUpsertSequencesOnMixedBatch(): void
{
/** @var Database $database */
$database = $this->getDatabase();

if (!$database->getAdapter()->getSupportForUpserts()) {
$this->expectNotToPerformAssertions();
return;
}

$collectionName = 'upsert_mixed_sequences';

$database->createCollection($collectionName, permissions: [
Permission::create(Role::any()),
Permission::read(Role::any()),
Permission::update(Role::any()),
]);

if ($database->getAdapter()->getSupportForAttributes()) {
$database->createAttribute($collectionName, 'name', Database::VAR_STRING, 128, true);
}

$permissions = [
Permission::read(Role::any()),
Permission::update(Role::any()),
];

$existingSequences = [];
foreach (['existing1', 'existing2'] as $id) {
$created = $database->createDocument($collectionName, new Document([
'$id' => $id,
'$permissions' => $permissions,
'name' => $id,
]));

$this->assertNotEmpty($created->getSequence());
$existingSequences[$id] = $created->getSequence();
}

// Existing and new rows interleaved, so the new ones sit at odd indexes in the batch.
$upserted = [];
$database->upsertDocuments(
$collectionName,
[
new Document(['$id' => 'existing1', '$permissions' => $permissions, 'name' => 'existing1 updated']),
new Document(['$id' => 'new1', '$permissions' => $permissions, 'name' => 'new1']),
new Document(['$id' => 'existing2', '$permissions' => $permissions, 'name' => 'existing2 updated']),
new Document(['$id' => 'new2', '$permissions' => $permissions, 'name' => 'new2']),
],
onNext: function (Document $document) use (&$upserted) {
$upserted[$document->getId()] = $document->getSequence();
}
);

$this->assertCount(4, $upserted);

foreach (['existing1', 'existing2', 'new1', 'new2'] as $id) {
$this->assertNotEmpty($upserted[$id], "No sequence returned for {$id}");
$this->assertEquals(
$database->getDocument($collectionName, $id)->getSequence(),
$upserted[$id],
"Wrong sequence returned for {$id}"
);
}

// An upsert must not move a row that was already there.
$this->assertEquals($existingSequences['existing1'], $upserted['existing1']);
$this->assertEquals($existingSequences['existing2'], $upserted['existing2']);

$database->deleteCollection($collectionName);
}

public function testRespectNulls(): Document
{
/** @var Database $database */
Expand Down
36 changes: 26 additions & 10 deletions tests/e2e/Adapter/Scopes/GeneralTests.php
Original file line number Diff line number Diff line change
Expand Up @@ -609,21 +609,35 @@ public function testSharedTablesTenantPerDocument(): void

$this->assertEquals(true, $doc->isEmpty());

// Upsert new documents with different tenants
// Upsert new documents with different tenants. The sequence lookup binds one
// placeholder per distinct tenant, so a cross-tenant batch has to keep each
// tenant's value at the position its placeholder was named for -- collected here
// because $onNext is the only way these documents reach the caller.
$doc4Id = ID::unique();
$doc5Id = ID::unique();
$sequences = [];
$database
->setTenant(null)
->setTenantPerDocument(true)
->upsertDocuments(__FUNCTION__, [new Document([
'$id' => $doc4Id,
'$tenant' => 4,
'name' => 'Superman4',
]), new Document([
'$id' => $doc5Id,
'$tenant' => 5,
'name' => 'Superman5',
])]);
->upsertDocuments(
__FUNCTION__,
[new Document([
'$id' => $doc4Id,
'$tenant' => 4,
'name' => 'Superman4',
]), new Document([
'$id' => $doc5Id,
'$tenant' => 5,
'name' => 'Superman5',
])],
onNext: function (Document $document) use (&$sequences) {
$sequences[$document->getId()] = $document->getSequence();
}
);

$this->assertCount(2, $sequences);
$this->assertNotEmpty($sequences[$doc4Id]);
$this->assertNotEmpty($sequences[$doc5Id]);

// Set to tenant 4 and read
$doc = $database
Expand All @@ -633,6 +647,7 @@ public function testSharedTablesTenantPerDocument(): void

$this->assertEquals('Superman4', $doc['name']);
$this->assertEquals(4, $doc->getTenant());
$this->assertEquals($doc->getSequence(), $sequences[$doc4Id]);

// Set to tenant 5 and read
$doc = $database
Expand All @@ -642,6 +657,7 @@ public function testSharedTablesTenantPerDocument(): void

$this->assertEquals('Superman5', $doc['name']);
$this->assertEquals(5, $doc->getTenant());
$this->assertEquals($doc->getSequence(), $sequences[$doc5Id]);

// Update names via upsert
$database
Expand Down
Loading