[tarantool-patches] Re: [PATCH 3/3] sql: too many autogenerated ids leads to SEGFAULT

[Imeev Mergen <imeevma@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4172 bytes --]

Hi! Thank you for review! New patch below.

On 11/19/18 8:27 PM, n.pettik wrote:
>> This probleam appeared because region was cleaned twice: once in
> Typo: problem
>
>> 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.
> 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.

Fixed commit-message.

*New patch:**
*

commit dd5b3aaa63b9fe2312a6fb30f3ba87bf8329b222
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Nov 17 13:05:55 2018 +0300

     sql: remove fiber_gc() from sqlite3VdbeHalt()

     Too many autogenerated ids leads to SEGFAULT. This problem
     appeared because region was cleaned twice: once in
     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() has been removed.

     Follow up #2618
     Follow up #3199

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index b6afe91..cc340e9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2911,12 +2911,8 @@ case OP_MakeRecord: {
       * memory shouldn't be reused until it is written into WAL.
       *
       * However, if memory for ephemeral space is allocated
-     * on region, it will be freed only in vdbeHalt() routine.
-     * It is the only way to free this region memory,
-     * since ephemeral spaces don't have nothing in common
-     * with txn routine and region memory won't be released
-     * after txn_commit() or txn_rollback() as it happens
-     * with ordinary spaces.
+     * on region, it will be freed only in sqlite3_finalize()
+     * routine.
       */
      if (bIsEphemeral) {
          rc = sqlite3VdbeMemClearAndResize(pOut, nByte);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 615a0f0..f2faad8 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2498,12 +2498,6 @@ sqlite3VdbeHalt(Vdbe * p)
          p->rc = SQLITE_NOMEM_BKPT;
      }

-    /* Release all region memory which was allocated
-     * to hold tuples to be inserted into ephemeral spaces.
-     */
-    if (!box_txn())
-        fiber_gc();
-
      assert(db->nVdbeActive > 0 || box_txn() ||
             p->anonymous_savepoint == NULL);
      return (p->rc == SQLITE_BUSY ? SQLITE_BUSY : SQLITE_OK);
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 6c50781..000b359 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -843,6 +843,22 @@ cn:execute('select * from "test"')
  s:drop()
  ---
  ...
+-- Too many autogenerated ids leads to SEGFAULT.
+cn = remote.connect(box.cfg.listen)
+---
+...
+box.sql.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT)')
+---
+...
+for i = 0, 1000 do cn:execute("INSERT INTO t1 VALUES (null)") end
+---
+...
+_ = cn:execute("INSERT INTO t1 SELECT NULL from t1")
+---
+...
+box.sql.execute('DROP TABLE t1')
+---
+...
  cn:close()
  ---
  ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index e12decd..2501393 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -272,6 +272,13 @@ s:insert({2, {a = 3}})
  cn:execute('select * from "test"')
  s:drop()

+-- Too many autogenerated ids leads to SEGFAULT.
+cn = remote.connect(box.cfg.listen)
+box.sql.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT)')
+for i = 0, 1000 do cn:execute("INSERT INTO t1 VALUES (null)") end
+_ = cn:execute("INSERT INTO t1 SELECT NULL from t1")
+box.sql.execute('DROP TABLE t1')
+
  cn:close()

  box.schema.user.revoke('guest', 'read,write,execute', 'universe')


[-- Attachment #2: Type: text/html, Size: 5699 bytes --]