<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi! Thank you for review! New patch below.<br>
</p>
<div class="moz-cite-prefix">On 11/19/18 8:27 PM, n.pettik wrote:<br>
</div>
<blockquote type="cite"
cite="mid:ADF4F323-ADF3-47A7-BF0B-08FC61898113@tarantool.org">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">This probleam appeared because region was cleaned twice: once in
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: problem
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">sqlite3VdbeHalt() and once in sqlite3VdbeDelete() which was
executed during sqlite3_finalize(). Autogenerated ids that were
saved there, were fetched after sqlite3VdbeHalt() and before
sqlite3_finalize(). In this patch region cleaning in
sqlite3VdbeHalt() were removed.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: was (or better - has been removed).
Again, IMHO I would rephrase commit subject:
sql: remove region_truncate() from sqlite3VdbeHalt()
And explain in commit message why it was removed.
Patch itself is OK.
</pre>
</blockquote>
<p>Fixed commit-message.</p>
<p><b>New patch:</b><b><br>
</b></p>
commit dd5b3aaa63b9fe2312a6fb30f3ba87bf8329b222<br>
Author: Mergen Imeev <a class="moz-txt-link-rfc2396E"
href="mailto:imeevma@gmail.com"><imeevma@gmail.com></a><br>
Date: Sat Nov 17 13:05:55 2018 +0300<br>
<br>
sql: remove fiber_gc() from sqlite3VdbeHalt()<br>
<br>
Too many autogenerated ids leads to SEGFAULT. This problem<br>
appeared because region was cleaned twice: once in<br>
sqlite3VdbeHalt() and once in sqlite3VdbeDelete() which was<br>
executed during sqlite3_finalize(). Autogenerated ids that were<br>
saved there, were fetched after sqlite3VdbeHalt() and before<br>
sqlite3_finalize(). In this patch region cleaning in<br>
sqlite3VdbeHalt() has been removed.<br>
<br>
Follow up #2618<br>
Follow up #3199<br>
<br>
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c<br>
index b6afe91..cc340e9 100644<br>
--- a/src/box/sql/vdbe.c<br>
+++ b/src/box/sql/vdbe.c<br>
@@ -2911,12 +2911,8 @@ case OP_MakeRecord: {<br>
* memory shouldn't be reused until it is written into WAL.<br>
*<br>
* However, if memory for ephemeral space is allocated<br>
- * on region, it will be freed only in vdbeHalt() routine.<br>
- * It is the only way to free this region memory,<br>
- * since ephemeral spaces don't have nothing in common<br>
- * with txn routine and region memory won't be released<br>
- * after txn_commit() or txn_rollback() as it happens<br>
- * with ordinary spaces.<br>
+ * on region, it will be freed only in sqlite3_finalize()<br>
+ * routine.<br>
*/<br>
if (bIsEphemeral) {<br>
rc = sqlite3VdbeMemClearAndResize(pOut, nByte);<br>
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c<br>
index 615a0f0..f2faad8 100644<br>
--- a/src/box/sql/vdbeaux.c<br>
+++ b/src/box/sql/vdbeaux.c<br>
@@ -2498,12 +2498,6 @@ sqlite3VdbeHalt(Vdbe * p)<br>
p->rc = SQLITE_NOMEM_BKPT;<br>
}<br>
<br>
- /* Release all region memory which was allocated<br>
- * to hold tuples to be inserted into ephemeral spaces.<br>
- */<br>
- if (!box_txn())<br>
- fiber_gc();<br>
-<br>
assert(db->nVdbeActive > 0 || box_txn() ||<br>
p->anonymous_savepoint == NULL);<br>
return (p->rc == SQLITE_BUSY ? SQLITE_BUSY : SQLITE_OK);<br>
diff --git a/test/sql/iproto.result b/test/sql/iproto.result<br>
index 6c50781..000b359 100644<br>
--- a/test/sql/iproto.result<br>
+++ b/test/sql/iproto.result<br>
@@ -843,6 +843,22 @@ cn:execute('select * from "test"')<br>
s:drop()<br>
---<br>
...<br>
+-- Too many autogenerated ids leads to SEGFAULT.<br>
+cn = remote.connect(box.cfg.listen)<br>
+---<br>
+...<br>
+box.sql.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY
AUTOINCREMENT)')<br>
+---<br>
+...<br>
+for i = 0, 1000 do cn:execute("INSERT INTO t1 VALUES (null)") end<br>
+---<br>
+...<br>
+_ = cn:execute("INSERT INTO t1 SELECT NULL from t1")<br>
+---<br>
+...<br>
+box.sql.execute('DROP TABLE t1')<br>
+---<br>
+...<br>
cn:close()<br>
---<br>
...<br>
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua<br>
index e12decd..2501393 100644<br>
--- a/test/sql/iproto.test.lua<br>
+++ b/test/sql/iproto.test.lua<br>
@@ -272,6 +272,13 @@ s:insert({2, {a = 3}})<br>
cn:execute('select * from "test"')<br>
s:drop()<br>
<br>
+-- Too many autogenerated ids leads to SEGFAULT.<br>
+cn = remote.connect(box.cfg.listen)<br>
+box.sql.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY
AUTOINCREMENT)')<br>
+for i = 0, 1000 do cn:execute("INSERT INTO t1 VALUES (null)") end<br>
+_ = cn:execute("INSERT INTO t1 SELECT NULL from t1")<br>
+box.sql.execute('DROP TABLE t1')<br>
+<br>
cn:close()<br>
<br>
box.schema.user.revoke('guest', 'read,write,execute', 'universe')<br>
<br>
</body>
</html>