[tarantool-patches] Re: [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead

[n.pettik]

> On 22 Mar 2018, at 17:11, v.shpilevoy at tarantool.org wrote:
> 
> See below 3 comments.
> 
>> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev at tarantool.org <mailto:korablev at tarantool.org>> написал(а):
>> 
>> 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=P2
>> +/* Opcode: OpenRead P1 P2 PЗ P4 P5
> 
> 1. Seems like you wrote here a cyrillic letter 'З' instead of digit three.

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

> 
>> + * Synopsis: index id = P2, space ptr = 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!=0 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=P2
>> +/* Opcode: ReopenIdx P1 P2 P3 P4 P5
>> + * Synopsis: index id = P2, space ptr = 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 ‘provides’. BTW, I didn’t come up with this word combination myself,
just found it somewhere in source code...

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180323/524b5a15/attachment.html>