[tarantool-patches] Re: [PATCH v4 6/6] sql: VDBE tests for trigger existence

[Kirill Shcherbatov]

> Why? If you didn’t check it, you simply would get ‘duplicate key error’.
> So, you did it to display more informative error..?
> 
>> tested on each VDBE
>> execution attempt, not on Parser iteration.
> 
> And? After this sentence you should explain changes which you patch provides.
 Trigger presence in system should be tested on each VDBE
    execution attempt, not on Parser iteration as system state
    could be changed between opcode run.
    With this patch, such checks are the part of VDBE program
    looking to the _trigger space and raise error message if
    tuple with specified key already exists.


> I would call it vdbe_emit_halt_if_exists().
> Or vdbe_emit_halt_on_duplication()...
+vdbe_emit_halt_if_exists(struct Parse *parser, int space_id, int index_id,


> In previous patch, you used sqlite3DbMalloc() function without using diag_set(),
> since in case of fail it would call sqlite3OomFault(db);
> Lets handle all usages of sqlite3DbMalloc() in the same way.
Ok.

> The same is here.
Ok.

> Why do you set ON_CONFLICT_ACTION_FAIL? You can just skip this arg with 0.
Ok.

> Why this func is under SQLITE_OMIT_CTE guard?
Moved.

> Why have you duplicated this assert? It occurs 2 lines above.
Fixed.

> Rephrase last sentence - I can’t parse it..
+ * The function allocates error and name resources for VDBE itself.

> How can index id contain ’string primary key’?
> Name of object to test on existence.
> Error message to display on VDBE halt.
Ok.

> Looks like redundant diff. If you wanted to make comments fit into 66 chars,
> you would better do it for the whole comment.
Refactored while comment.


> Why did you provide this diff? AFAIK in this particular case these two calls
> are *almost* equivalent.
No, you set SQL_TARANTOOL_ERROR here that require diag error been set with my patch.