[tarantool-patches] Re: [PATCH 4/5] sql: display error on FK creation and drop failure

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx, Nikita Pettik <korablev@xxxxxxxxxxxxx>
  • Date: Wed, 18 Jul 2018 00:04:56 +0300

Thanks for the patch! See 4 comments below.

On 13/07/2018 05:04, Nikita Pettik wrote:

Before insertion to _fk_constraint we must be sure that there in no
entry with given <name, child id>. Otherwise, insertion will fail and
'duplicate key' will be shown. Such error message doesn't seem to be
informative enough, so lets manually iterate through whole space looking
for appropriate record.

1. As I know, vdbe_emit_halt_with_presence_test do not iterate through
the whole space. It uses an index to search for the record fast.

The same is for dropping constraint, but here vice versa: we test
that _fk_contraint contains entry with given name and child id.

2. Typo: _fk_contraint -> _fk_constraint.


It is worth mentioning that during CREATE TABLE processing schema id
changes and check in OP_OpenRead opcode fails (which in turn shows that
pointer to space may expire). On the other hand, _fk_constraint space
itself remains immutable, so as a temporary workaround lets use flag
indicating pointer to system space passed to OP_OpenRead. It makes
possible to use pointer to space, even if schema has changed.

Closes #3271
---
  src/box/errcode.h            |  2 ++
  src/box/sql/build.c          | 43 +++++++++++++++++++++++++++++++------------
  src/box/sql/sqliteInt.h      | 10 +++++++---
  src/box/sql/trigger.c        | 24 ++++++++++++++++--------
  src/box/sql/vdbe.c           |  3 ++-
  test/box/misc.result         |  2 ++
  test/sql-tap/alter2.test.lua | 25 ++++++++++++++++++++++++-
  7 files changed, 84 insertions(+), 25 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index c2d3cd035..20ace09e4 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1784,6 +1784,20 @@ vdbe_fkey_code_creation(struct Parse *parse_context, 
const struct fkey_def *fk)
                sqlite3VdbeAddOp2(vdbe, OP_Integer, fk->parent_id,
                                  constr_tuple_reg + 2);
        }
+       /*
+        * Lets check that constraint with this name hasn't
+        * been created before.
+        */
+       const char *error_msg =
+               tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), name_copy);
+       if (vdbe_emit_halt_with_presence_test(parse_context,
+                                             BOX_FK_CONSTRAINT_ID, 0,
+                                             constr_tuple_reg, 2,
+                                             ER_CONSTRAINT_EXISTS, error_msg,
+                                             false, OP_NoConflict) != 0) {
+               free((void *) name_copy);

3. Name_copy is allocated on db.

+               return;
+       }
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index b9723e2e7..0f227e637 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3172,7 +3172,8 @@ case OP_OpenWrite:
         * during runtime.
         */
        if (box_schema_version() != p->schema_ver &&
-           (pOp->p5 & OPFLAG_FRESH_PTR) == 0) {
+           (pOp->p5 & OPFLAG_FRESH_PTR) == 0 &&
+           (pOp->p5 & OPFLAG_SYSTEMSP) == 0) {

4. Why not p5 & (FRESH_PTR | SYSTEMSP) ?

                p->expired = 1;
                rc = SQLITE_ERROR;
                sqlite3VdbeError(p, "schema version has changed: " \

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2018