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

[v.shpilevoy at tarantool.org]

See below 3 comments.

> 21 марта 2018 г., в 2:48, Nikita Pettik <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.

> + * 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?
> 
> +	/*
> +	 * 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?