[tarantool-patches] Re: [PATCH 6/6] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY

[n.pettik]

>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 1d7b6c827..0314be957 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -2083,6 +2083,19 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)
>>  	return iid_reg;
>>  }
>>  +static void
>> +pk_check_existence(struct Parse *parse, uint32_t space_id, int cursor)
> 
> 1. It is worth mentioning in a comment that the cursor is opened on _index
> space. Or rename it to _index_cursor. It do not mind if in such a case you
> omit a comment.

I’d prefer second option:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 080914576..cdeb85bd7 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2089,12 +2089,12 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor)
 }
 
 static void
-pk_check_existence(struct Parse *parse, uint32_t space_id, int cursor)
+pk_check_existence(struct Parse *parse, uint32_t space_id, int _index_cursor)
 {
        struct Vdbe *v = sqlite3GetVdbe(parse);
        int tmp_reg = ++parse->nMem;
        sqlite3VdbeAddOp2(v, OP_Integer, space_id, tmp_reg);
-       int found_addr = sqlite3VdbeAddOp4Int(v, OP_NotFound, cursor, 0,
+       int found_addr = sqlite3VdbeAddOp4Int(v, OP_NotFound, _index_cursor, 0,

> 
>> +{
>> +	struct Vdbe *v = sqlite3GetVdbe(parse);
>> +	int tmp_reg = ++parse->nMem;
> 
> 2. Why not to use a truly temp register? As I know, we have facilities for
> it. Parse.nMem are not temporary. Parse.nTempReg - is.

I am afraid that they don’t work. I greped through code a bit and
found no real usages of tmp regs. What is more:

int
sqlite3GetTempReg(Parse * pParse)
{
       if (pParse->nTempReg == 0) {
              return ++pParse->nMem;
       }
       return pParse->aTempReg[--pParse->nTempReg];
}

It always returns real register since nTempReg == 0 and
is never incremented.

I’ve opened issue:
https://github.com/tarantool/tarantool/issues/3942
https://github.com/tarantool/tarantool/issues/3943

>> +	sqlite3VdbeAddOp2(v, OP_Integer, space_id, tmp_reg);
>> +	int found_addr = sqlite3VdbeAddOp4Int(v, OP_NotFound, cursor, 0,
>> +					     tmp_reg, 1);
>> +	sqlite3VdbeAddOp4(v, OP_Halt, SQLITE_ERROR, ON_CONFLICT_ACTION_FAIL, 0,
>> +			  "multiple primary keys are not allowed", P4_STATIC);
>> +	sqlite3VdbeJumpHere(v, found_addr);
>> +}
>> +
>>  /**
>>   * Add new index to table's indexes list.
>>   * We follow convention that PK comes first in list.
>> diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
>> index 1aad555c0..925753749 100755
>> --- a/test/sql-tap/alter.test.lua
>> +++ b/test/sql-tap/alter.test.lua
>> @@ -517,6 +517,62 @@ test:do_catchsql_test(
>>          -- </alter-7.11>
>>      })
>>  +test:do_test(
>> +    "alter-8.1.0",
>> +    function()
>> +        format = {}
>> +        format[1] = { name = 'id', type = 'scalar'}
>> +        format[2] = { name = 'f2', type = 'scalar'}
>> +        s = box.schema.create_space('T', {format = format})
>> +    end,
>> +    {})
>> +
>> +test:do_catchsql_test(
>> +    "alter-8.1.1",
>> +    [[
>> +        ALTER TABLE t ADD CONSTRAINT pk PRIMARY KEY("id");
>> +    ]], {
>> +        0
>> +    })
>> +
>> +test:do_test(
>> +    "alter-8.1.2",
>> +    function()
>> +        i = box.space.T.index[0]
>> +        return i.id
> 
> 3. Why not return box.space.T.index[0].id?

…

diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua
index 925753749..318b0f68d 100755
--- a/test/sql-tap/alter.test.lua
+++ b/test/sql-tap/alter.test.lua
@@ -538,8 +538,7 @@ test:do_catchsql_test(
 test:do_test(
     "alter-8.1.2",
     function()
-        i = box.space.T.index[0]
-        return i.id
+        return box.space.T.index[0].id
     end, 0)