[tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: Kirill Shcherbatov <kshcherbatov@xxxxxxxxxxxxx>, tarantool-patches@xxxxxxxxxxxxx
  • Date: Tue, 10 Jul 2018 14:48:09 +0300

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;
  }

Other related posts: