From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A416B2C757 for ; Thu, 22 Nov 2018 13:09:41 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id TwmDv8ezfrAw for ; Thu, 22 Nov 2018 13:09:41 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1FF05284A5 for ; Thu, 22 Nov 2018 13:09:41 -0500 (EST) From: Imeev Mergen Subject: [tarantool-patches] Re: [PATCH 3/3] sql: too many autogenerated ids leads to SEGFAULT References: <6128bdce00cc4c6d47c122897c195e30e0416613.1542635129.git.v.shpilevoy@tarantool.org> Message-ID: <05a63d11-105a-6dd7-add7-8aeeca48d8ed@tarantool.org> Date: Thu, 22 Nov 2018 21:09:39 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------9BD9723D360E9BA64CD3F4B6" Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.org Cc: Vladislav Shpilevoy This is a multi-part message in MIME format. --------------9BD9723D360E9BA64CD3F4B6 Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit 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 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') --------------9BD9723D360E9BA64CD3F4B6 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit

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')

--------------9BD9723D360E9BA64CD3F4B6--