[tarantool-patches] Re: [PATCH v11] sql: add index_def to struct Index

  • From: "n.pettik" <korablev@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Fri, 6 Jul 2018 03:51:41 +0300


Then:

CREATE TABLE t11 (s1 INT, a, constraint c1 UNIQUE(s1) on conflict replace, 
PRIMARY KEY(s1));

In this case creation of unique constraint c1 is omitted, but no errors or 
warnings are shown.
It is not a problem now, but when ALTER TABLE DROP CONSTRAINT is 
implemented,
it will be possible to drop c1 constraint. Eventually, user would be 
disappointed if tried to drop
this constraint but got an error.
It seems to be out of the scope of the patch. Appropriate ticket:
https://github.com/tarantool/tarantool/issues/3498
I don’t understand how that ticket is related to given issue. Again, problem 
lies in
silent absorption of unique constraint by primary index.
I am not sure what to do with this. In Postgresql these two commands work 
silently, no UNIQUE
constraint is created

Really? I tried it on PostgreSQL 9.6.2 and got this:

CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT c1 UNIQUE(id));

SELECT conname FROM pg_constraint WHERE conrelid = (SELECT oid  FROM pg_class 
WHERE relname LIKE 't1’);

    conname
1       c1

As we can see, this unique constraint has been successfully created.

and ALTER TABLE works silently and do nothing.

And after drop:

ALTER TABLE t1 DROP CONSTRAINT c1;

SELECT conname FROM pg_constraint WHERE conrelid = (SELECT oid  FROM pg_class 
WHERE relname LIKE 't1’);

I got nothing.

In MySQL both constraints
will be created with no warnings or errors. What do you propose to do?

Lets dive into ANSI: if there is nothing about that (i.e. explicit 
contradictions), then
we shouldn’t omit creation of unique constraint over PK (at least in case it is 
named constraint).

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index d66777f73..d2fed97eb 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2430,13 +2430,14 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, 
int memRootPage)
 }
   struct Index *
-sql_index_alloc(struct sqlite3 *db)
+sql_index_alloc(struct sqlite3 *db, uint32_t part_count)
 {
        /* Size of struct Index and aiRowLogEst. */
-       int index_size = ROUND8(sizeof(struct Index));
+       int index_size = sizeof(struct Index) +
+                        sizeof(LogEst) * (part_count + 1);
        struct Index *p = sqlite3DbMallocZero(db, index_size);
        if (p != NULL)
-               p->aiRowLogEst = (LogEst *) ((char *)p + ROUND8(sizeof(*p)));
+               p->aiRowLogEst = (LogEst *) ((char *)p + sizeof(*p));
        return p;
 }

@@ -2769,11 +2757,10 @@ sql_create_index(struct Parse *parse, struct Token 
*token,
        if (sqlite3CheckIdentifierName(parse, name) != SQLITE_OK)
                goto exit_create_index;
 -       index = sql_index_alloc(db);
+       index = sql_index_alloc(db, col_list->nExpr);

@@ -2769,11 +2757,10 @@ sql_create_index(struct Parse *parse, struct Token 
*token,
        if (sqlite3CheckIdentifierName(parse, name) != SQLITE_OK)
                goto exit_create_index;
 -       index = sql_index_alloc(db);
+       index = sql_index_alloc(db, col_list->nExpr);
        if (index == NULL)
                goto exit_create_index;
 -       assert(EIGHT_BYTE_ALIGNMENT(index->aiRowLogEst));

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index ae31dfae5..2082d6ca9 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3631,7 +3631,7 @@ void sqlite3SrcListDelete(sqlite3 *, SrcList *);
  * @retval not NULL Index object.
  */
 struct Index *
-sql_index_alloc(struct sqlite3 *db);
+sql_index_alloc(struct sqlite3 *db, uint32_t part_count);
Why do we need to allocate 'part_count' part of memory? It does not seem
to be used for anything.

It is used for array aiRowLogEst. Btw, you really can remove and now use
index_def->opts.stat->tuple_log_est;  for fake indexes - then you are able to
avoid allocating this memory.

diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 85143ed20..7ca02095f 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -372,13 +372,19 @@ whereScanInit(WhereScan * pScan,    /* The 
WhereScan object being initialized */
  pScan->is_column_seen = false;
  if (pIdx) {
          int j = iColumn;
-         iColumn = pIdx->aiColumn[j];
+         iColumn = pIdx->def->key_def->parts[j].fieldno;
+         /*
+          * pIdx->tnum == 0 means that pIdx is a fake
+          * integer primary key index.
+          */
+         if (pIdx->tnum == 0)
+                 iColumn = -1;
We are going to remove tnum from struct Index and struct Table.
So, if it is possible, use index->def->iid instead (or smth else).
Removed with ‘fake_autoindex'
Well, if smb called index ‘fake_autoindex’, I guess strange things would 
happen.
Could we use for instance def->space_id == 0 as a sign of ‘fake_index'?.
Yes, it seems to be a better solution. What do you think about adding a field 
to
index_def.opts - index_def.opts.is_fake_pk?

I am strictly against - it sounds like over-engineering. In parser, for example,
space_def->opts.is_temporary is used as a sing of space_def allocated on a 
region
(but initially this property is set to be true if space is temporary and 
temporary != allocated on region).
So, since index is in fact surrogate, lets just use one of its fields.


Moreover, sql_stmt is obtained from sqlite3Malloc() and assigned to 
index->opts,
which in turn is released by common free().
In index_def_new() (where we pass opts) given sql_stmt is copied using 
strdup, so
it's okay to free() it, isn't it?

Ok, then you don’t release original sql_stmt: fix it.


diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index d66777f73..0cb0b8e08 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -984,8 +984,10 @@ sqlite3AddPrimaryKey(Parse * pParse,     /* Parsing 
context */
              sqlite3ErrorMsg(pParse, "AUTOINCREMENT is only allowed on an "
                              "INTEGER PRIMARY KEY or INT PRIMARY KEY");
      } else {
+             /* Since this index implements PK, it is unique. */
              sql_create_index(pParse, 0, 0, pList, onError, 0,
-                              0, sortOrder, false, 
SQLITE_IDXTYPE_PRIMARYKEY);
+                              0, sortOrder, false, SQLITE_IDXTYPE_PRIMARYKEY,
+                              true);
              pList = 0;
      }
@@ -1311,9 +1313,10 @@ convertToWithoutRowidTable(Parse * pParse, Table * 
pTab)
                      return;
              pList->a[0].sort_order = pParse->iPkSortOrder;
              assert(pParse->pNewTable == pTab);
+             /* Since this index implements PK, it is unique. */
              sql_create_index(pParse, 0, 0, pList, pTab->keyConf, 0, 0,
                               SORT_ORDER_ASC, false,
-                              SQLITE_IDXTYPE_PRIMARYKEY);
+                              SQLITE_IDXTYPE_PRIMARYKEY, true);
              if (db->mallocFailed)
                      return;
              pPk = sqlite3PrimaryKeyIndex(pTab);
@@ -2538,10 +2541,8 @@ addIndexToTable(Index * pIndex, Table * pTab)
 * @param expr_list List of expressions, describe which columns
 *                  of 'table' are used in index and also their
 *                  collations, orders, etc.
- * @param idx_type Index type, one of the following:
- *                 SQLITE_IDXTYPE_APPDEF     0
- *                 SQLITE_IDXTYPE_UNIQUE     1
- *                 SQLITE_IDXTYPE_PRIMARYKEY 2.
+ * @param idx_type Index type: UNIQUE constraint, PK constraint,
+ *                 or created by <CREATE INDEX ...> stmt.
 * @param sql_stmt SQL statement, which creates the index.
 * @retval 0 Success.
 * @retval -1 Error.
@@ -2632,7 +2633,7 @@ sql_create_index(struct Parse *parse, struct Token 
*token,
               struct SrcList *tbl_name, struct ExprList *col_list,
               enum on_conflict_action on_error, struct Token *start,
               struct Expr *where, enum sort_order sort_order,
-              bool if_not_exist, u8 idx_type)
+              bool if_not_exist, u8 idx_type, bool is_unique)

Wait, you already have idx_type, you don’t need another one argument.
Lets simply turn defines into appropriate enum
(probably you should pick better names, but follow code style):

-/*
- * Allowed values for Index.idxType
- */
-#define SQLITE_IDXTYPE_APPDEF      0   /* Created using CREATE INDEX */
-#define SQLITE_IDXTYPE_UNIQUE      1   /* Implements a UNIQUE constraint */
-#define SQLITE_IDXTYPE_PRIMARYKEY  2   /* Is the PRIMARY KEY for the table */
+/**
+ * There are some differences between explicitly created indexes
+ * and unique table constraints (despite the fact that they are
+ * the same in implementation). For instance, to drop index user
+ * must use <DROP INDEX ...> statement, but to drop constraint -
+ * <ALTER TABLE DROP CONSTRAINT ...> syntax.
+ */
+enum sql_index_type {
+       SQL_INDEX_USER_DEFINED_UINIQUE = 0,
+       SQL_INDEX_USER_DEFINED = 1,
+       SQL_INDEX_CONSTRAINT_UNIQUE = 2,
+       SQL_INDEX_CONSTRAINT_PK = 3,
+};


Other related posts:

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