[tarantool-patches] Re: [PATCH 10/10] sql: move autoincrement field number to server

["n.pettik" <korablev@tarantool.org>]

> On 13 Aug 2018, at 23:24, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Are you sure it is the only way to pass autoinc fieldno? And that it can
> not be dropped and calculated from other fields without significant problems?
> 
> Now this flag looks very ugly inside _space tuples.
> 
> I think, autoinc is rather primary index option than the space, and that it
> can be detected via checking
> 
>    pk->part_count == 1 and space->sequence != NULL
> 
> then pk->parts[0].fieldno is autoinc field. It is not?

Okay, it seems to be possible. Check this out:

    sql: remove autoincrement field number from Table
    
    During INSERT SQL statement execution we may need to know field which
    stores INTEGER AUTOINCREMENT PRIMARY KEY. Since we are going to get rid
    of struct Table, lets remove this member from struct Table. Instead, it
    can be calculated using struct space: if PK consists from one part and
    sequence not NULL - table features autoincrement.
    
    Part of #3561

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index b38a934a1..dc00b5d8c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -401,7 +401,6 @@ sql_table_new(Parse *parser, char *name)
        strcpy(table->def->engine_name,
               sql_storage_engine_strs[current_session()->sql_default_engine]);
 
-       table->iAutoIncPKey = -1;
        table->pSchema = db->pSchema;
        table->nTabRef = 1;
        return table;
@@ -868,8 +867,7 @@ sqlite3AddPrimaryKey(Parse * pParse,        /* Parsing context */
            pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER &&
            sortOrder != SORT_ORDER_DESC) {
                assert(autoInc == 0 || autoInc == 1);
-               if (autoInc)
-                       pTab->iAutoIncPKey = iCol;
+               pParse->is_new_table_autoinc = autoInc;
                struct sqlite3 *db = pParse->db;
                struct ExprList *list;
                struct Token token;
@@ -1698,7 +1696,7 @@ sqlite3EndTable(Parse * pParse,   /* Parse context */
         * Check to see if we need to create an _sequence table
         * for keeping track of autoincrement keys.
         */
-       if (p->iAutoIncPKey >= 0) {
+       if (pParse->is_new_table_autoinc) {
                assert(reg_space_id != 0);
                /* Do an insertion into _sequence. */
                int reg_seq_id = ++pParse->nMem;
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index d9e230aee..7780bf749 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -100,6 +100,22 @@ sql_emit_table_affinity(struct Vdbe *v, struct space_def *def, int reg)
                          P4_DYNAMIC);
 }
 
+/**
+ * In SQL table can be created with AUTOINCREMENT.
+ * In Tarantool it can be detected as primary key which consists
+ * from one field with not NULL space's sequence.
+ */
+static uint32_t
+sql_space_autoinc_fieldno(struct space *space)
+{
+       assert(space != NULL);
+       struct index *pk = space_index(space, 0);
+       if (pk == NULL || pk->def->key_def->part_count != 1 ||
+           space->sequence == NULL)
+               return UINT32_MAX;
+       return pk->def->key_def->parts[0].fieldno;
+}
+
 /**
  * This routine is used to see if a statement of the form
  * "INSERT INTO <table> SELECT ..." can run for the results of the
@@ -556,7 +572,9 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
                    sqlite3VdbeAddOp1(v, OP_Yield, dest.iSDParm);
                VdbeCoverage(v);
        }
-
+       struct space *space = space_by_id(pTab->def->id);
+       assert(space != NULL);
+       uint32_t autoinc_fieldno = sql_space_autoinc_fieldno(space);
        /* Run the BEFORE and INSTEAD OF triggers, if there are any
         */
        endOfLoop = sqlite3VdbeMakeLabel(v);
@@ -574,7 +592,7 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
                        }
                        if ((!useTempTable && !pList)
                            || (pColumn && j >= pColumn->nId)) {
-                               if (i == pTab->iAutoIncPKey) {
+                               if (i == (int) autoinc_fieldno) {
                                        sqlite3VdbeAddOp2(v, OP_Integer, -1,
                                                          regCols + i + 1);
                                } else {
@@ -637,7 +655,7 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
                        }
                        if (j < 0 || nColumn == 0
                            || (pColumn && j >= pColumn->nId)) {
-                               if (i == pTab->iAutoIncPKey) {
+                               if (i == (int) autoinc_fieldno) {
                                        sqlite3VdbeAddOp2(v,
                                                          OP_NextAutoincValue,
                                                          pTab->def->id,
@@ -652,7 +670,7 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
                                                          dflt,
                                                          iRegStore);
                        } else if (useTempTable) {
-                               if (i == pTab->iAutoIncPKey) {
+                               if (i == (int) autoinc_fieldno) {
                                        int regTmp = ++pParse->nMem;
                                        /* Emit code which doesn't override
                                         * autoinc-ed value with select result
@@ -671,7 +689,7 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
                                }
                        } else if (pSelect) {
                                if (regFromSelect != regData) {
-                                       if (i == pTab->iAutoIncPKey) {
+                                       if (i == (int) autoinc_fieldno) {
                                                /* Emit code which doesn't override
                                                 * autoinc-ed value with select result
                                                 * in case that result is NULL
@@ -693,7 +711,7 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
                                }
                        } else {
 
-                               if (i == pTab->iAutoIncPKey) {
+                               if (i == (int) autoinc_fieldno) {
                                        if (pList->a[j].pExpr->op == TK_NULL) {
                                                sqlite3VdbeAddOp2(v, OP_Null, 0, iRegStore);
                                                continue;
@@ -730,8 +748,6 @@ sqlite3Insert(Parse * pParse,       /* Parser context */
                                            on_error, endOfLoop, 0);
                fkey_emit_check(pParse, pTab, 0, regIns, 0);
                int pk_cursor = pParse->nTab++;
-               struct space *space = space_by_id(pTab->def->id);
-               assert(space != NULL);
                vdbe_emit_open_cursor(pParse, pk_cursor, 0, space);
                vdbe_emit_insertion_completion(v, pk_cursor, regIns + 1,
                                               pTab->def->field_count,
@@ -846,6 +862,9 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
        /* Insertion into VIEW is prohibited. */
        assert(!def->opts.is_view);
        bool is_update = upd_cols != NULL;
+       struct space *space = space_by_id(tab->def->id);
+       assert(space != NULL);
+       uint32_t autoinc_fieldno = sql_space_autoinc_fieldno(space);
        /* Test all NOT NULL constraints. */
        for (uint32_t i = 0; i < def->field_count; i++) {
                /*
@@ -855,7 +874,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
                if (is_update && upd_cols[i] < 0)
                        continue;
                /* This column is allowed to be NULL. */
-               if (def->fields[i].is_nullable || tab->iAutoIncPKey == (int) i)
+               if (def->fields[i].is_nullable || autoinc_fieldno == i)
                        continue;
                enum on_conflict_action on_conflict_nullable =
                        on_conflict != ON_CONFLICT_ACTION_DEFAULT ?
@@ -948,7 +967,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
                int reg_pk = new_tuple_reg + fieldno;
                if (tab->def->fields[fieldno].affinity == AFFINITY_INTEGER) {
                        int skip_if_null = sqlite3VdbeMakeLabel(v);
-                       if (tab->iAutoIncPKey >= 0) {
+                       if (autoinc_fieldno != UINT32_MAX) {
                                sqlite3VdbeAddOp2(v, OP_IsNull, reg_pk,
                                                  skip_if_null);
                        }
@@ -1000,7 +1019,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab,
 process_index: ;
                int cursor = parse_context->nTab++;
                vdbe_emit_open_cursor(parse_context, cursor, idx->def->iid,
-                                     space_by_id(tab->def->id));
+                                     space);
                /*
                 * If there is no conflict in current index, just
                 * jump to the start of next iteration. Label is
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 9a45acedd..d2ef85846 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1850,8 +1850,6 @@ struct Savepoint {
 struct Table {
        Index *pIndex;          /* List of SQL indexes on this table. */
        u32 nTabRef;            /* Number of pointers to this Table */
-       i16 iAutoIncPKey;       /* If PK is marked INTEGER PRIMARY KEY AUTOINCREMENT, store
-                                  column number here, -1 otherwise Tarantool specifics */
        /**
         * Estimated number of entries in table.
         * Used only when table represents temporary objects,
@@ -2838,6 +2836,8 @@ struct Parse {
         */
        struct rlist new_fkey;
        bool initiateTTrans;    /* Initiate Tarantool transaction */
+       /** True, if table to be created has AUTOINCREMENT PK. */
+       bool is_new_table_autoinc;
        /** If set - do not emit byte code at all, just parse.  */
        bool parse_only;
        /** Type of parsed_ast member. */