Skip to content

Commit 3fe0214

Browse files
kenjiskenjis
authored
Merge pull request #5202 from kenjis/fix-insertBatch-memory-leak
Reduce memory usage of insertBatch(), updateBatch()
2 parents f6cd180 + 9d329a7 commit 3fe0214

File tree

5 files changed

+69
-66
lines changed

5 files changed

+69
-66
lines changed

‎system/Database/BaseBuilder.php

Lines changed: 56 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1590,7 +1590,7 @@ public function getWhere($where = null, ?int $limit = null, ?int $offset = 0, bo
15901590
*
15911591
* @throws DatabaseException
15921592
*
1593-
* @return false|int Number of rows inserted or FALSE on failure
1593+
* @return false|int|string[] Number of rows inserted or FALSE on failure, SQL array when testMode
15941594
*/
15951595
public function insertBatch(?array $set = null, ?bool $escape = null, int $batchSize = 100)
15961596
{
@@ -1602,38 +1602,49 @@ public function insertBatch(?array $set = null, ?bool $escape = null, int $batch
16021602

16031603
return false; // @codeCoverageIgnore
16041604
}
1605-
} else {
1606-
if (empty($set)) {
1607-
if (CI_DEBUG) {
1608-
throw new DatabaseException('insertBatch() called with no data');
1609-
}
1610-
1611-
return false; // @codeCoverageIgnore
1605+
} elseif (empty($set)) {
1606+
if (CI_DEBUG) {
1607+
throw new DatabaseException('insertBatch() called with no data');
16121608
}
16131609

1614-
$this->setInsertBatch($set, '', $escape);
1610+
return false; // @codeCoverageIgnore
16151611
}
16161612

1613+
$hasQBSet = $set === null;
1614+
16171615
$table = $this->QBFrom[0];
16181616

16191617
$affectedRows = 0;
1618+
$savedSQL = [];
16201619

1621-
for ($i = 0, $total = count($this->QBSet); $i < $total; $i += $batchSize) {
1622-
$sql = $this->_insertBatch($this->db->protectIdentifiers($table, true, null, false), $this->QBKeys, array_slice($this->QBSet, $i, $batchSize));
1620+
if ($hasQBSet) {
1621+
$set = $this->QBSet;
1622+
}
1623+
1624+
for ($i = 0, $total = count($set); $i < $total; $i += $batchSize) {
1625+
if ($hasQBSet) {
1626+
$QBSet = array_slice($this->QBSet, $i, $batchSize);
1627+
} else {
1628+
$this->setInsertBatch(array_slice($set, $i, $batchSize), '', $escape);
1629+
$QBSet = $this->QBSet;
1630+
}
1631+
$sql = $this->_insertBatch($this->db->protectIdentifiers($table, true, null, false), $this->QBKeys, $QBSet);
16231632

16241633
if ($this->testMode) {
1625-
$affectedRows++;
1634+
$savedSQL[] = $sql;
16261635
} else {
1627-
$this->db->query($sql, $this->binds, false);
1636+
$this->db->query($sql, null, false);
16281637
$affectedRows += $this->db->affectedRows();
16291638
}
1630-
}
16311639

1632-
if (! $this->testMode) {
1633-
$this->resetWrite();
1640+
if (! $hasQBSet) {
1641+
$this->resetWrite();
1642+
}
16341643
}
16351644

1636-
return $affectedRows;
1645+
$this->resetWrite();
1646+
1647+
return $this->testMode ? $savedSQL : $affectedRows;
16371648
}
16381649

16391650
/**
@@ -1677,8 +1688,8 @@ public function setInsertBatch($key, string $value = '', ?bool $escape = null)
16771688

16781689
$clean = [];
16791690

1680-
foreach ($row as $k => $rowValue) {
1681-
$clean[] = ':' . $this->setBind($k, $rowValue, $escape) . ':';
1691+
foreach ($row as $rowValue) {
1692+
$clean[] = $escape ? $this->db->escape($rowValue) : $rowValue;
16821693
}
16831694

16841695
$row = $clean;
@@ -1944,7 +1955,7 @@ protected function validateUpdate(): bool
19441955
*
19451956
* @throws DatabaseException
19461957
*
1947-
* @return mixed Number of rows affected, SQL string, or FALSE on failure
1958+
* @return false|int|string[] Number of rows affected or FALSE on failure, SQL array when testMode
19481959
*/
19491960
public function updateBatch(?array $set = null, ?string $index = null, int $batchSize = 100)
19501961
{
@@ -1964,28 +1975,37 @@ public function updateBatch(?array $set = null, ?string $index = null, int $batc
19641975

19651976
return false; // @codeCoverageIgnore
19661977
}
1967-
} else {
1968-
if (empty($set)) {
1969-
if (CI_DEBUG) {
1970-
throw new DatabaseException('updateBatch() called with no data');
1971-
}
1972-
1973-
return false; // @codeCoverageIgnore
1978+
} elseif (empty($set)) {
1979+
if (CI_DEBUG) {
1980+
throw new DatabaseException('updateBatch() called with no data');
19741981
}
19751982

1976-
$this->setUpdateBatch($set, $index);
1983+
return false; // @codeCoverageIgnore
19771984
}
19781985

1986+
$hasQBSet = $set === null;
1987+
19791988
$table = $this->QBFrom[0];
19801989

19811990
$affectedRows = 0;
19821991
$savedSQL = [];
19831992
$savedQBWhere = $this->QBWhere;
19841993

1985-
for ($i = 0, $total = count($this->QBSet); $i < $total; $i += $batchSize) {
1994+
if ($hasQBSet) {
1995+
$set = $this->QBSet;
1996+
}
1997+
1998+
for ($i = 0, $total = count($set); $i < $total; $i += $batchSize) {
1999+
if ($hasQBSet) {
2000+
$QBSet = array_slice($this->QBSet, $i, $batchSize);
2001+
} else {
2002+
$this->setUpdateBatch(array_slice($set, $i, $batchSize), $index);
2003+
$QBSet = $this->QBSet;
2004+
}
2005+
19862006
$sql = $this->_updateBatch(
19872007
$table,
1988-
array_slice($this->QBSet, $i, $batchSize),
2008+
$QBSet,
19892009
$this->db->protectIdentifiers($index)
19902010
);
19912011

@@ -1996,6 +2016,10 @@ public function updateBatch(?array $set = null, ?string $index = null, int $batc
19962016
$affectedRows += $this->db->affectedRows();
19972017
}
19982018

2019+
if (! $hasQBSet) {
2020+
$this->resetWrite();
2021+
}
2022+
19992023
$this->QBWhere = $savedQBWhere;
20002024
}
20012025

@@ -2065,9 +2089,8 @@ public function setUpdateBatch($key, string $index = '', ?bool $escape = null)
20652089
$indexSet = true;
20662090
}
20672091

2068-
$bind = $this->setBind($k2, $v2, $escape);
2069-
2070-
$clean[$this->db->protectIdentifiers($k2, false)] = ":{$bind}:";
2092+
$clean[$this->db->protectIdentifiers($k2, false)]
2093+
= $escape ? $this->db->escape($v2) : $v2;
20712094
}
20722095

20732096
if ($indexSet === false) {

‎tests/system/Database/Builder/InsertTest.php

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,9 @@ public function testInsertBatch()
9191
$query = $this->db->getLastQuery();
9292
$this->assertInstanceOf(Query::class, $query);
9393

94-
$raw = 'INSERT INTO "jobs" ("description", "id", "name") VALUES (:description:,:id:,:name:), (:description.1:,:id.1:,:name.1:)';
94+
$raw = <<<'SQL'
95+
INSERT INTO "jobs" ("description", "id", "name") VALUES ('There''s something in your teeth',2,'Commedian'), ('I am yellow',3,'Cab Driver')
96+
SQL;
9597
$this->assertSame($raw, str_replace("\n", ' ', $query->getOriginalQuery()));
9698

9799
$expected = "INSERT INTO \"jobs\" (\"description\", \"id\", \"name\") VALUES ('There''s something in your teeth',2,'Commedian'), ('I am yellow',3,'Cab Driver')";
@@ -121,9 +123,6 @@ public function testInsertBatchWithoutEscape()
121123
$query = $this->db->getLastQuery();
122124
$this->assertInstanceOf(Query::class, $query);
123125

124-
$raw = 'INSERT INTO "jobs" ("description", "id", "name") VALUES (:description:,:id:,:name:), (:description.1:,:id.1:,:name.1:)';
125-
$this->assertSame($raw, str_replace("\n", ' ', $query->getOriginalQuery()));
126-
127126
$expected = 'INSERT INTO "jobs" ("description", "id", "name") VALUES (1 + 2,2,1 + 1), (2 + 2,3,2 + 1)';
128127
$this->assertSame($expected, str_replace("\n", ' ', $query->getQuery()));
129128
}
@@ -148,9 +147,6 @@ public function testInsertBatchWithFieldsEndingInNumbers()
148147
$query = $this->db->getLastQuery();
149148
$this->assertInstanceOf(Query::class, $query);
150149

151-
$raw = 'INSERT INTO "ip_table" ("ip", "ip2") VALUES (:ip:,:ip2:), (:ip.1:,:ip2.1:), (:ip.2:,:ip2.2:), (:ip.3:,:ip2.3:)';
152-
$this->assertSame($raw, str_replace("\n", ' ', $query->getOriginalQuery()));
153-
154150
$expected = "INSERT INTO \"ip_table\" (\"ip\", \"ip2\") VALUES ('1.1.1.0','1.1.1.2'), ('2.2.2.0','2.2.2.2'), ('3.3.3.0','3.3.3.2'), ('4.4.4.0','4.4.4.2')";
155151
$this->assertSame($expected, str_replace("\n", ' ', $query->getQuery()));
156152
}

‎tests/system/Database/Builder/UpdateTest.php

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -208,19 +208,6 @@ public function testUpdateBatch()
208208

209209
$space = ' ';
210210

211-
$expected = <<<EOF
212-
UPDATE "jobs" SET "name" = CASE{$space}
213-
WHEN "id" = :id: THEN :name:
214-
WHEN "id" = :id.1: THEN :name.1:
215-
ELSE "name" END, "description" = CASE{$space}
216-
WHEN "id" = :id: THEN :description:
217-
WHEN "id" = :id.1: THEN :description.1:
218-
ELSE "description" END
219-
WHERE "id" IN(:id:,:id.1:)
220-
EOF;
221-
222-
$this->assertSame($expected, $query->getOriginalQuery());
223-
224211
$expected = <<<EOF
225212
UPDATE "jobs" SET "name" = CASE{$space}
226213
WHEN "id" = 2 THEN 'Comedian'
@@ -261,19 +248,6 @@ public function testSetUpdateBatchWithoutEscape()
261248

262249
$space = ' ';
263250

264-
$expected = <<<EOF
265-
UPDATE "jobs" SET "name" = CASE{$space}
266-
WHEN "id" = :id: THEN :name:
267-
WHEN "id" = :id.1: THEN :name.1:
268-
ELSE "name" END, "description" = CASE{$space}
269-
WHEN "id" = :id: THEN :description:
270-
WHEN "id" = :id.1: THEN :description.1:
271-
ELSE "description" END
272-
WHERE "id" IN(:id:,:id.1:)
273-
EOF;
274-
275-
$this->assertSame($expected, $query->getOriginalQuery());
276-
277251
$expected = <<<EOF
278252
UPDATE "jobs" SET "name" = CASE{$space}
279253
WHEN "id" = 2 THEN SUBSTRING(name, 1)

‎user_guide_src/source/changelogs/v4.1.5.rst

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ BREAKING
1414

1515
- Fixed `a bug <https://github.com/codeigniter4/CodeIgniter4/issues/2913>`_ on CSRF protection. Now CSRF protection works on PUT/PATCH/DELETE requests when CSRF filter is applied. If you use such requests, you need to send CSRF token.
1616
- In the previous version, if you didn't provide your own headers, ``CURLRequest`` would send the request-headers from the browser, due to a bug. As of this version, it does not send them.
17+
- Fixed ``BaseBuilder::insertBatch()`` return value for ``testMode``. Now it returns SQL string array instead of a number of affected rows. This change was made because of maintaining compatibility between returning types for batch methods. Now the returned data type for ``BaseBuilder::insertBatch()`` is the same as the `updateBatch()` method.
18+
- Major optimizations have been made to the way data is processed in ``BaseBuilder::insertBatch()`` and ``BaseBuilder::updateBatch()`` methods. This resulted in reduced memory usage and faster query processing. As a trade-off, the result generated by the ``$query->getOriginalQuery()`` method was changed. It no longer returns the query with the binded parameters, but the actual query that was run.
1719

1820
Enhancements
1921
============

‎user_guide_src/source/installation/upgrade_415.rst

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,14 @@ The bug was fixed. If your requests depend on the headers, your requests might f
6161
In this case, add the necessary headers manually.
6262
See `CURLRequest Class <../libraries/curlrequest.html#headers>`_ for how to add.
6363

64+
Query Builder changes
65+
---------------------
66+
67+
For optimization and a bug fix, the following behaviors, mostly used in testing, have been changed.
68+
69+
- When you use ``insertBatch()`` and ``updateBatch()``, the return value of ``$query->getOriginalQuery()`` has changed. It no longer returns the query with the binded parameters, but the actual query that was run.
70+
- If ``testMode`` is ``true``, ``insertBatch()`` will return an SQL string array instead of the number of affected rows. This change was made so that the returned data type is the same as the ``updateBatch()`` method.
71+
6472
Breaking Enhancements
6573
=====================
6674

0 commit comments

Comments
 (0)