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 24CC32D37E for ; Fri, 23 Mar 2018 12:39:12 -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 C37FHAwPRZ4a for ; Fri, 23 Mar 2018 12:39:12 -0400 (EDT) Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 AEED02D37B for ; Fri, 23 Mar 2018 12:39:11 -0400 (EDT) From: "n.pettik" Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_78897D65-3342-42B3-BCBC-937797423A5F" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead Date: Fri, 23 Mar 2018 19:39:07 +0300 In-Reply-To: <899B6F64-2E1F-47B3-9ED7-9E13DD34EB8B@tarantool.org> References: <84263f8fdb228ea8de7adda1a0d70d3112b5adbc.1521583434.git.korablev@tarantool.org> <899B6F64-2E1F-47B3-9ED7-9E13DD34EB8B@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=_78897D65-3342-42B3-BCBC-937797423A5F Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 22 Mar 2018, at 17:11, v.shpilevoy@tarantool.org wrote: >=20 > See below 3 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 >> After new DDL SQL implementation has been introduced, OP_OpenWrite, >> OP_OpenRead and OP_ReopenIdx opcodes can be refactored. >>=20 >> Firstly, if schema versions at compile time and runtime don't match, >> finish VDBE execution with appropriate message error. Exception is = the >> situation when fifth pointer is set to OPFLAG_FRESH_PTR, which means >> that space pointer has been fetched during runtime right before that. >>=20 >> Secondly, there is no need to fetch number of columns in index from >> KeyInfo: iterator yields the full tuple, so it would always be equal = to >> the number of fields in a whole space. >>=20 >> Finally, now we always can pass space pointer to these opcodes >> regardless of DML routine. In case of OP_ReopenIdx opcode space and >> index from given cursor is checked on equality to those given in >> arguments. If they match, this opcode will become no-op. >> --- >> src/box/sql/opcodes.c | 6 +- >> src/box/sql/opcodes.h | 6 +- >> src/box/sql/vdbe.c | 197 = +++++++++++++++++--------------------------------- >> 3 files changed, 71 insertions(+), 138 deletions(-) >>=20 >> -/* Opcode: OpenRead P1 P2 * P4 P5 >> - * Synopsis: root=3DP2 >> +/* Opcode: OpenRead P1 P2 P=D0=97 P4 P5 >=20 > 1. Seems like you wrote here a cyrillic letter '=D0=97' instead of = digit three. Holy sh=E2=80=A6 How did you manage to notice that? My font displays = them almost identical.. I would call it =E2=80=99next-level review=E2=80=99. Anyway thanks, = fixed. >=20 >> + * Synopsis: index id =3D P2, space ptr =3D P3 >> * >> - * Open a read-only cursor for the database table whose root page is >> - * P2 in a database file.=20 >> - * Give the new cursor an identifier of P1. The P1 >> - * values need not be contiguous but all P1 values should be small = integers. >> - * It is an error for P1 to be negative. >> + * Open a cursor for a space specified by pointer in P3 and index >> + * id in P2. Give the new cursor an identifier of P1. The P1 >> + * values need not be contiguous but all P1 values should be >> + * small integers. It is an error for P1 to be negative. >> * >> - * If P5!=3D0 then use the content of register P2 as the root page, = not >> - * the value of P2 itself. >> + * The P4 value may be a pointer to a KeyInfo structure. >> + * If it is a pointer to a KeyInfo structure, then said structure >> + * defines the content and collatining sequence of the index >> + * being opened. Otherwise, P4 is NULL. >> * >> - * There will be a read lock on the database whenever there is an >> - * open cursor. If the database was unlocked prior to this = instruction >> - * then a read lock is acquired as part of this instruction. A read >> - * lock allows other processes to read the database but prohibits >> - * any other process from modifying the database. The read lock is >> - * released when all cursors are closed. If this instruction = attempts >> - * to get a read lock but fails, the script terminates with an >> - * SQLITE_BUSY error code. >> - * >> - * The P4 value may be either an integer (P4_INT32) or a pointer to >> - * a KeyInfo structure (P4_KEYINFO). If it is a pointer to a KeyInfo >> - * structure, then said structure defines the content and collating >> - * sequence of the index being opened. Otherwise, if P4 is an = integer >> - * value, it is set to the number of columns in the table. >> - * >> - * See also: OpenWrite, ReopenIdx >> + * If schema has changed since compile time, VDBE ends execution >> + * with appropriate error message. The only exception is >> + * when P5 is set to OPFLAG_FRESH_PTR, which means that >> + * space pointer has been fetched in runtime right before >> + * this opcode. >> */ >> -/* Opcode: ReopenIdx P1 P2 * P4 P5 >> - * Synopsis: root=3DP2 >> +/* Opcode: ReopenIdx P1 P2 P3 P4 P5 >> + * Synopsis: index id =3D P2, space ptr =3D P3 >> * >> - * The ReopenIdx opcode works exactly like ReadOpen except that it = first >> - * checks to see if the cursor on P1 is already open with a root = page >> - * number of P2 and if it is this opcode becomes a no-op. In other = words, >> - * if the cursor is already open, do not reopen it. >> + * The ReopenIdx opcode works exactly like ReadOpen except that >=20 > 2. I can not find ReadOpen - possibly it is OP_OpenRead? Fixed. >>=20 >> + /* >> + * Since Tarantool iterator yields the full tuple, >> + * we need a number of fields as wide as the table itself. >> + * Otherwise, not enough slots for row parser cache are >> + * allocated in VdbeCursor object. >=20 > 3. Term 'yields' slightly confused. For the first moments it seems, = that an iterator yields a fiber, not fields. Can you please find another = word? Ok, replaced with =E2=80=98provides=E2=80=99. BTW, I didn=E2=80=99t come = up with this word combination myself, just found it somewhere in source code... --Apple-Mail=_78897D65-3342-42B3-BCBC-937797423A5F Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
On 22 Mar 2018, at 17:11, v.shpilevoy@tarantool.org wrote:

See below 3 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):

After new DDL SQL = implementation has been introduced, OP_OpenWrite,
OP_OpenRead and OP_ReopenIdx opcodes can be refactored.

Firstly, if schema versions at compile time = and runtime don't match,
finish VDBE execution with = appropriate message error. Exception is the
situation when = fifth pointer is set to OPFLAG_FRESH_PTR, which means
that = space pointer has been fetched during runtime right before that.

Secondly, there is no need to fetch number of = columns in index from
KeyInfo: iterator yields the full = tuple, so it would always be equal to
the number of fields = in a whole space.

Finally, now we always = can pass space pointer to these opcodes
regardless of DML = routine. In case of OP_ReopenIdx opcode space and
index = from given cursor is checked on equality to those given in
arguments. If they match, this opcode will become no-op.
---
src/box/sql/opcodes.c |   6 +-
src/box/sql/opcodes.h |   6 +-
src/box/sql/vdbe.c    | 197 = +++++++++++++++++---------------------------------
3 files = changed, 71 insertions(+), 138 deletions(-)

-/* Opcode: OpenRead P1 P2 * P4 P5
- * = Synopsis: root=3DP2
+/* Opcode: OpenRead P1 P2 P=D0=97 P4 = P5

1. Seems like you wrote here a = cyrillic letter '=D0=97' instead of digit three.

Holy sh=E2=80= =A6 How did you manage to notice that? My font displays them almost = identical..
I would call it =E2=80=99next-level review=E2=80=99.= Anyway thanks, fixed.


+ * Synopsis: index id =3D = P2, space ptr =3D P3
*
- * Open a read-only = cursor for the database table whose root page is
- * P2 in = a database file. 
- * Give the new cursor an identifier of P1.  The P1
- * values need not be contiguous but all P1 values should be = small integers.
- * It is an error for P1 to be = negative.
+ * Open a cursor for a space specified by = pointer in P3 and index
+ * id in P2. Give the new cursor = an identifier of P1. The P1
+ * values need not be = contiguous but all P1 values should be
+ * small integers. = It is an error for P1 to be negative.
*
- * = If P5!=3D0 then use the content of register P2 as the root page, not
- * the value of P2 itself.
+ * The P4 value = may be a pointer to a KeyInfo structure.
+ * If it is a = pointer to a KeyInfo structure, then said structure
+ * = defines the content and collatining sequence of the index
+ = * being opened. Otherwise, P4 is NULL.
*
- * = There will be a read lock on the database whenever there is an
- * open cursor.  If the database was unlocked prior to = this instruction
- * then a read lock is acquired as part = of this instruction.  A read
- * lock allows other = processes to read the database but prohibits
- * any other = process from modifying the database.  The read lock is
- * released when all cursors are closed.  If this = instruction attempts
- * to get a read lock but fails, the = script terminates with an
- * SQLITE_BUSY error code.
- *
- * The P4 value may be either an integer = (P4_INT32) or a pointer to
- * a KeyInfo structure = (P4_KEYINFO). If it is a pointer to a KeyInfo
- * = structure, then said structure defines the content and collating
- * sequence of the index being opened. Otherwise, if P4 is = an integer
- * value, it is set to the number of columns = in the table.
- *
- * See also: OpenWrite, = ReopenIdx
+ * If schema has changed since compile time, = VDBE ends execution
+ * with appropriate error message. = The only exception is
+ * when P5 is set to = OPFLAG_FRESH_PTR, which means that
+ * space pointer has = been fetched in runtime right before
+ * this opcode.
*/
-/* Opcode: ReopenIdx P1 P2 * P4 P5
- * Synopsis: root=3DP2
+/* Opcode: ReopenIdx = P1 P2 P3 P4 P5
+ * Synopsis: index id =3D P2, space ptr =3D = P3
*
- * The ReopenIdx opcode works exactly = like ReadOpen except that it first
- * checks to see if = the cursor on P1 is already open with a root page
- * = number of P2 and if it is this opcode becomes a no-op.  In other = words,
- * if the cursor is already open, do not reopen = it.
+ * The ReopenIdx opcode works exactly like ReadOpen = except that

2. I can not find ReadOpen - = possibly it is OP_OpenRead?

Fixed.


+ /*
+  * Since Tarantool iterator = yields the full tuple,
+  * we need a number of = fields as wide as the table itself.
+  * Otherwise, not enough = slots for row parser cache are
+  * allocated in VdbeCursor = object.

3. Term 'yields' slightly = confused. For the first moments it seems, that an iterator yields a = fiber, not fields. Can you please find another = word?

Ok, replaced = with =E2=80=98provides=E2=80=99. BTW, I didn=E2=80=99t come up with this = word combination myself,
just found it somewhere in source = code...

= --Apple-Mail=_78897D65-3342-42B3-BCBC-937797423A5F--