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 ACD6A2D36C for ; Fri, 23 Mar 2018 12:33:27 -0400 (EDT) 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 WUouWOeJuAtv for ; Fri, 23 Mar 2018 12:33:27 -0400 (EDT) Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 20E6E2D36B for ; Fri, 23 Mar 2018 12:33:26 -0400 (EDT) From: "n.pettik" Message-Id: <8B10DD51-5F2B-4CC1-9909-1F3E14B11A95@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_05AD420D-830D-4BDD-83A2-EF5A8A915FC2" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 3/4] sql: rework code generation for DDL routine Date: Fri, 23 Mar 2018 19:33:24 +0300 In-Reply-To: <33D8E3FE-EA0B-4426-8CC0-BCCB448AD239@tarantool.org> References: <71a9b8ac842931885bb91551c2466c670f238d10.1521583434.git.korablev@tarantool.org> <33D8E3FE-EA0B-4426-8CC0-BCCB448AD239@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org --Apple-Mail=_05AD420D-830D-4BDD-83A2-EF5A8A915FC2 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 22 Mar 2018, at 16:57, v.shpilevoy@tarantool.org wrote: >=20 > See below 4 comments. >=20 >> 21 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2018 =D0=B3., =D0=B2 2:48, Nikita = Pettik > = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >>=20 >> As far as new opcodes to operate on system spaces have been = introduced, >> there is no more need to open cursors on such tables, when it comes = to >> insert/delete operations. In addition, it allows to get rid of system >> spaces lookups from SQL data dictionary. Moreover, previously DDL >> relied on nested parse to make deletions from system space. But = during >> nested parse it is impossible to convert space id to space pointer = (i.e. >> emit OP_SIDtoPtr opcode) without any overhead or "hacks". So, nested >> parse has been replaced with hardcoded sequences of opcodes, >> implementing the same logic. >>=20 >> Closes #3252 >> --- >> src/box/sql/build.c | 322 = ++++++++++++++++++++++--------------------- >> src/box/sql/sqliteInt.h | 5 + >> src/box/sql/trigger.c | 27 ++-- >> test/sql/transition.result | 11 +- >> test/sql/transition.test.lua | 8 +- >> 5 files changed, 190 insertions(+), 183 deletions(-) >>=20 >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index 229c8b4d5..02d27dec6 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -2238,9 +2182,11 @@ sqlite3CodeDropTable(Parse * pParse, Table * = pTab, int isView) >=20 > 1. sqlite3CodeDropTable now is used in build.c only, and can be = declared as static (and removed from sqliteInt.h). Done: --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2306,7 +2250,7 @@ sqlite3ClearStatTables(Parse * pParse, /* The = parsing context */ /* * Generate code to drop a table. */ -void +static void sqlite3CodeDropTable(Parse * pParse, Table * pTab, int isView) --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3007,7 +3012,6 @@ int sqlite3ViewGetColumnNames(Parse *, Table *); int sqlite3DbMaskAllZero(yDbMask); #endif void sqlite3DropTable(Parse *, SrcList *, int, int); -void sqlite3CodeDropTable(Parse *, Table *, int); >=20 >> v =3D sqlite3GetVdbe(pParse); >> assert(v !=3D 0); >> sqlite3BeginWriteOperation(pParse, 1); >> - >> - /* Drop all triggers associated with the table being dropped. = Code >> - * is generated to remove entries from _trigger space. >> + /* >> + * Drop all triggers associated with the table being >> + * dropped. Code is generated to remove entries from >> + * _trigger. OP_DropTrigger will remove it from internal >> + * SQL structures. >> */ >> pTrigger =3D pTab->pTrigger; >> /* Do not account triggers deletion - they will be accounted >> @@ -2254,58 +2200,115 @@ sqlite3CodeDropTable(Parse * pParse, Table * = pTab, int isView) >> - >> - /* Drop all _space and _index entries that refer to the >> - * table. The program loops through the _index & _space tables = and deletes >> - * every row that refers to a table. >> - * Triggers are handled separately because a trigger can be >> - * created in the temp database that refers to a table in = another >> - * database. >> + /* Delete entry by from _space_sequence. */ >> + sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, >> + idx_rec_reg); >> + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SPACE_SEQUENCE_ID, >> + idx_rec_reg); >> + VdbeComment((v, "Delete entry from _space_sequence")); >> + /* >> + * Program below implement analogue of SQL statement: >> + * "DELETE FROM BOX_SEQUENCE WHERE name =3D zName;" >> + * However, we can't use nested parse for DDL, >> + * since pointers to spaces will expire. >> + */ >=20 > 2. Why do you delete by name, if you have struct = space.sequence->def->id? To be honest, I forgot about such opportunity. Fetching id directly from = sequence allows to significantly reduce number of opcodes and simplify query = logic: + /* Delete entry by id from _sequence. */ + assert(space->sequence !=3D NULL); + int sequence_id_reg =3D ++pParse->nMem; + sqlite3VdbeAddOp2(v, OP_Integer, = space->sequence->def->id, + sequence_id_reg); + sqlite3VdbeAddOp3(v, OP_MakeRecord, sequence_id_reg, 1, + idx_rec_reg); + sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID, = idx_rec_reg); + VdbeComment((v, "Delete entry from _sequence")); + } >=20 >>=20 >> + /* >> + * Drop all _space and _index entries that refer to the >> + * table. >> */ >> - int space_id =3D SQLITE_PAGENO_TO_SPACEID(pTab->tnum); >> if (!isView) { >> - if (pTab->pIndex && pTab->pIndex->pNext) { >> - /* Remove all indexes, except for primary. >> - Tarantool won't allow remove primary when = secondary exist. */ >> - sqlite3NestedParse(pParse, >> - "DELETE FROM \"%s\" WHERE = \"id\"=3D%d AND \"iid\">0", >> - TARANTOOL_SYS_INDEX_NAME, = space_id); >> + struct space *space =3D space_by_id(space_id); >> + assert(space !=3D NULL); >> + uint32_t index_count =3D space->index_count; >> + if (index_count > 1) { >> + /* >> + * Remove all indexes, except for primary. >> + * Tarantool won't allow remove primary when >> + * secondary exist. >> + */ >> + uint32_t *iids =3D >> + (uint32_t *) = sqlite3Malloc(sizeof(uint32_t) * >> + (index_count = - 1)); >=20 > 3.1. Check for OOM; > 3.2. For such short lived allocations you can use region_alloc - it = allows to avoid multiple free() calls on each alloc, and free all temp = allocations at once after a transaction is finished. Ok, substituted with region_alloc: + size_t size =3D sizeof(uint32_t) * (index_count = - 1); + uint32_t *iids =3D + (uint32_t *) region_alloc(&fiber()->gc, = size); + if (iids =3D=3D NULL) { + diag_set(OutOfMemory, size, = "region_alloc", + "iids"); + return; + } >>=20 >> @@ -2603,16 +2606,18 @@ sqlite3RefillIndex(Parse * pParse, Index * = pIndex, int memRootPage) >> sqlite3VdbeJumpHere(v, addr1); >> if (memRootPage < 0) >> sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0); >> - struct space *space =3D = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum)); >> - assert(space !=3D NULL); >> int space_ptr_reg =3D ++pParse->nMem; >> - sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0, >> - ((int64_t) space)); >> + /* >> + * OP_Clear can use truncate optimization >> + * (i.e. insert record into _truncate) and schema >> + * may change. Thus, use dynamic conversion from >> + * space id to ptr. >> + */ >> + sqlite3VdbeAddOp2(v, OP_SIDtoPtr, = SQLITE_PAGENO_TO_SPACEID(tnum), >> + space_ptr_reg); >> sqlite3VdbeAddOp4(v, OP_OpenWrite, iIdx, tnum, space_ptr_reg, >> (char *)pKey, P4_KEYINFO); >> - sqlite3VdbeChangeP5(v, >> - OPFLAG_BULKCSR | ((memRootPage >=3D 0) ? >> - OPFLAG_P2ISREG : 0)); >> + sqlite3VdbeChangeP5(v, OPFLAG_FRESH_PTR); >=20 > 4. OPFLAG_FRESH_PTR seems to be unused. Why do you need it? I need it to indicate that pointer can=E2=80=99t become expired. Its = usage is introduced in the last patch. --Apple-Mail=_05AD420D-830D-4BDD-83A2-EF5A8A915FC2 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
On 22 Mar 2018, at 16:57, v.shpilevoy@tarantool.org wrote:

See below 4 comments.
21 =D0=BC=D0=B0=D1=80=D1=82=D0= =B0 2018 =D0=B3., =D0=B2 2:48, Nikita Pettik <korablev@tarantool.org> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0= =B0=D0=BB(=D0=B0):

As far as new opcodes to = operate on system spaces have been introduced,
there is no = more need to open cursors on such tables, when it comes to
insert/delete operations. In addition, it allows to get rid = of system
spaces lookups from SQL data dictionary. =  Moreover, previously DDL
relied on nested parse to = make deletions from system space. But during
nested parse = it is impossible to convert space id to space pointer (i.e.
emit OP_SIDtoPtr opcode) without any overhead or "hacks". =  So, nested
parse has been replaced with hardcoded = sequences of opcodes,
implementing the same logic.

Closes #3252
---
src/box/sql/build.c =          | 322 = ++++++++++++++++++++++---------------------
src/box/sql/sqliteInt.h      | =   5 +
src/box/sql/trigger.c =        |  27 ++--
test/sql/transition.result   |  11 +-
test/sql/transition.test.lua |   8 +-
5= files changed, 190 insertions(+), 183 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 229c8b4d5..02d27dec6 100644
--- = a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2238,9 +2182,11 @@ sqlite3CodeDropTable(Parse * pParse, = Table * pTab, int isView)

1. sqlite3CodeDropTable now is used in build.c = only, and can be declared as static (and removed from = sqliteInt.h).

Done:

--- = a/src/box/sql/build.c
+++ = b/src/box/sql/build.c
@@ -2306,7 +2250,7 @@ = sqlite3ClearStatTables(Parse * pParse, /* The parsing context = */
 /*
  * Generate code to drop a = table.
  */
-void
+static = void
 sqlite3CodeDropTable(Parse * pParse, Table * = pTab, int isView)

--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3007,7 +3012,6 @@ int sqlite3ViewGetColumnNames(Parse *, = Table *);
 int = sqlite3DbMaskAllZero(yDbMask);
 #endif
 void sqlite3DropTable(Parse *, SrcList *, int, = int);
-void sqlite3CodeDropTable(Parse *, Table *, = int);


v =3D sqlite3GetVdbe(pParse);
= assert(v !=3D 0);
= sqlite3BeginWriteOperation(pParse, 1);
-
- = /* Drop all triggers associated with the table being dropped. = Code
-  * is = generated to remove entries from _trigger space.
+ /*
+ =  * Drop all = triggers associated with the table being
+  * dropped. Code is = generated to remove entries from
+  * _trigger. OP_DropTrigger = will remove it from internal
+  * SQL structures.
=  */
= pTrigger =3D pTab->pTrigger;
/* Do not = account triggers deletion - they will be accounted
@@ = -2254,58 +2200,115 @@ sqlite3CodeDropTable(Parse * pParse, Table * pTab, = int isView)
-
- /* Drop all _space and _index = entries that refer to the
-  * table. The program loops = through the _index & _space tables and deletes
-  * every row that refers to = a table.
-  * Triggers are handled = separately because a trigger can be
-  * created in the temp = database that refers to a table in another
-  * database.
+ = = /* Delete entry by from _space_sequence. */
+ = sqlite3VdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1,
+ = = = =   idx_rec_reg);
+ = = sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SPACE_SEQUENCE_ID,
+ = = = =   idx_rec_reg);
+ = = VdbeComment((v, "Delete entry from _space_sequence"));
+ = = /*
+  * Program below implement = analogue of SQL statement:
+  * "DELETE FROM BOX_SEQUENCE = WHERE name =3D zName;"
+  * However, we can't use = nested parse for DDL,
+  * since pointers to spaces = will expire.
+  */

2. Why do you delete by name, if you have = struct space.sequence->def->id?

To be = honest, I forgot about such opportunity. Fetching id directly from = sequence
allows to significantly reduce number of opcodes and = simplify query logic:

+ = /* Delete entry by id from _sequence. */
+ = assert(space->sequence !=3D NULL);
+ = int sequence_id_reg =3D ++pParse->nMem;
+ = sqlite3VdbeAddOp2(v, OP_Integer, = space->sequence->def->id,
+ =  sequence_id_reg);
+ sqlite3VdbeAddOp3(v, = OP_MakeRecord, sequence_id_reg, 1,
+ =  idx_rec_reg);
+ sqlite3VdbeAddOp2(v, = OP_SDelete, BOX_SEQUENCE_ID, idx_rec_reg);
+ = VdbeComment((v, "Delete entry from _sequence"));
+ = }



+ /*
+ =  * Drop all = _space and _index entries that refer to the
+  * table.
 */
- int = space_id =3D SQLITE_PAGENO_TO_SPACEID(pTab->tnum);
if = (!isView) {
- if (pTab->pIndex && = pTab->pIndex->pNext) {
- /*  Remove all indexes, = except for primary.
-    Tarantool won't = allow remove primary when secondary exist. */
- = sqlite3NestedParse(pParse,
-    "DELETE FROM = \"%s\" WHERE \"id\"=3D%d AND \"iid\">0",
-    TARANTOOL_SYS_IND= EX_NAME, space_id);
+ struct space *space =3D = space_by_id(space_id);
+ assert(space !=3D NULL);
+ = = uint32_t index_count =3D space->index_count;
+ = = if (index_count > 1) {
+ /*
+ = = =  * Remove all = indexes, except for primary.
+  * Tarantool won't allow = remove primary when
+  * secondary exist.
+ = = =  */
+ = = = uint32_t *iids =3D
+ (uint32_t *) = sqlite3Malloc(sizeof(uint32_t) *
+    (index_count - = 1));

3.1. Check for OOM;
3.2. For such short lived allocations you can = use region_alloc - it allows to avoid multiple free() calls on each = alloc, and free all temp allocations at once after a transaction is = finished.

Ok, substituted with region_alloc:

+ size_t size =3D = sizeof(uint32_t) * (index_count - 1);
+ = uint32_t *iids =3D
+ (uint32_t = *) region_alloc(&fiber()->gc, size);
+ = if (iids =3D=3D NULL) {
+ = diag_set(OutOfMemory, size, "region_alloc",
+ = "iids");
+ = return;
+ }


@@ -2603,16 = +2606,18 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int = memRootPage)
sqlite3VdbeJumpHere(v, addr1);
= if (memRootPage < 0)
= sqlite3VdbeAddOp2(v, OP_Clear, tnum, 0);
- struct = space *space =3D space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum));
- = assert(space !=3D NULL);
int = space_ptr_reg =3D ++pParse->nMem;
- = sqlite3VdbeAddOp4Int64(v, OP_Int64, 0, space_ptr_reg, 0,
- = = =       = ; ((int64_t) space));
+ /*
+  * OP_Clear can use truncate = optimization
+  * (i.e. insert record into = _truncate) and schema
+  * may change. Thus, use = dynamic conversion from
+  * space id to ptr.
+ =  */
+ = sqlite3VdbeAddOp2(v, OP_SIDtoPtr, = SQLITE_PAGENO_TO_SPACEID(tnum),
+   space_ptr_reg);
= sqlite3VdbeAddOp4(v, OP_OpenWrite, iIdx, tnum, space_ptr_reg,
= = =   (char = *)pKey, P4_KEYINFO);
- sqlite3VdbeChangeP5(v,
- = = =     OPFLAG_BULK= CSR | ((memRootPage >=3D 0) ?
-       = ;OPFLAG_P2ISREG : 0));
+ sqlite3VdbeChangeP5(v, = OPFLAG_FRESH_PTR);

4. OPFLAG_FRESH_PTR seems to be = unused. Why do you need it?

I need it to indicate that pointer can=E2=80=99t = become expired. Its usage is introduced in the last patch.

= --Apple-Mail=_05AD420D-830D-4BDD-83A2-EF5A8A915FC2--