[tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Jul 10 14:48:09 MSK 2018
Hello. Thanks for the patch!
On 09/07/2018 15:42, Kirill Shcherbatov wrote:
> Made vdbe_emit_open_cursor calls consistent:
> now it uses index id everywhere.
> This required to change a way to detect that
> VDBE has openned Read cursor to specified table
> in vdbe_has_table_read to write result of insert
> in temp table if required.
> ---
Where are the issue and the branch?
> https://github.com/tarantool/tarantool/compare/kshch/kshch/vdbe_emit_open_cursor-refactor
This is not branch and by this link I see message:
"There isn’t anything to compare."
> over branch
> https://github.com/tarantool/tarantool/issues/2847
How is this linked? The issue was found under working with this
issue, but it has been actual before and remains actual after.
See 7 comments below.
And I have pushed my own patch for LoadPtr removal on the top
of your branch. Please, fix my comments below, and fix the patch
I have pushed (most likely it does not pass tests now).
>
> src/box/sql/analyze.c | 3 +-
> src/box/sql/build.c | 4 ++-
> src/box/sql/expr.c | 5 ++-
> src/box/sql/fkey.c | 3 +-
> src/box/sql/insert.c | 95 ++++++++++++++++++++++++++++++---------------------
> src/box/sql/select.c | 4 +--
> src/box/sql/where.c | 10 ++++--
> 7 files changed, 75 insertions(+), 49 deletions(-)
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 1c00842..82447dc 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1108,6 +1108,7 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
> struct space *space)
> {
> assert(space != NULL);
> + assert(index_id == SQLITE_PAGENO_TO_INDEXID(index_id));
1. What the hell? Why?
> struct Vdbe *vdbe = parse_context->pVdbe;
> int space_ptr_reg = ++parse_context->nMem;
> sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
> addr1 = sqlite3VdbeAddOp2(v, OP_SorterSort, iSorter, 0);
2. vdbe_emit_open_cursor has outdated comment in the header.
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 58e159c..77567e7 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -179,41 +179,50 @@ sqlite3TableAffinity(Vdbe * v, Table * pTab, int iReg)
> }
> }
>
> -/*
> - * Return non-zero if the table pTab in database or any of its indices
> - * have been opened at any point in the VDBE program. This is used to see if
> - * a statement of the form "INSERT INTO <pTab> SELECT ..." can
> - * run for the results of the SELECT.
> +/**
> + * Return true if the table pTab in database or any of its
3. No pTab.
> + * indices have been opened at any point in the VDBE program.
> + * This is used to see if a statement of the form
> + * "INSERT INTO <pTab> SELECT ..." can run for the results of the
> + * SELECT.
> + *
> + * @param parser Parse context.
> + * @param table Table AST object.
> + * @retval true or false
4. Unhelpful comment. Obviously bool is either true or false.
> */
> -static int
> -readsTable(Parse * p, Table * pTab)
> +static bool
> +vdbe_has_table_read(struct Parse *parser, struct Table *table)
5. You can make table be const struct Table *.
> {
> - Vdbe *v = sqlite3GetVdbe(p);
> - int i;
> - int iEnd = sqlite3VdbeCurrentAddr(v);
> -
> - for (i = 1; i < iEnd; i++) {
> - VdbeOp *pOp = sqlite3VdbeGetOp(v, i);
> - assert(pOp != 0);
> - /* Currently, there is no difference between
> - * Read and Write cursors.
> + struct Vdbe *v = sqlite3GetVdbe(parser);
> + int last_instr = sqlite3VdbeCurrentAddr(v);
> + for (int i = 1; i < last_instr; i++) {
> + struct VdbeOp *pOp = sqlite3VdbeGetOp(v, i);
6. When a function is refactored completely, we should use
Tarantool code style for variable names.
> + assert(pOp != NULL);
> + /*
> + * Currently, there is no difference between Read
> + * and Write cursors.
> */
> - if (pOp->opcode == OP_OpenRead ||
> - pOp->opcode == OP_OpenWrite) {
> - Index *pIndex;
> - int tnum = pOp->p2;
> - if (tnum == pTab->tnum) {
> - return 1;
> - }
> - for (pIndex = pTab->pIndex; pIndex;
> - pIndex = pIndex->pNext) {
> - if (tnum == pIndex->tnum) {
> - return 1;
> - }
> + if (pOp->opcode == OP_OpenRead || pOp->opcode == OP_OpenWrite) {
> + assert(i > 1);
> + struct VdbeOp *space_var_op =
> + sqlite3VdbeGetOp(v, i - 1);
> + assert(space_var_op != NULL);
> + assert(space_var_op->opcode == OP_LoadPtr);
> + struct space *space = space_var_op->p4.space;
> +
> + if (space == space_by_id(table->def->id))
7. Why do you need space_by_id()? Why not just space->id == table->def->id?
> + return true;
> +
> + int idx_id = pOp->p2;
> + for (struct Index *pIndex = table->pIndex;
> + pIndex != NULL; pIndex = pIndex->pNext) {
> + if (idx_id ==
> + SQLITE_PAGENO_TO_INDEXID(pIndex->tnum))
> + return true;
> }
> }
> }
> - return 0;
> + return false;
> }
>
>
More information about the Tarantool-patches
mailing list