[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Jun 19 13:24:28 MSK 2018
Hello. Thanks for the fixes! See 4 comments below.
On 18/06/2018 22:22, Kirill Shcherbatov wrote:
>> 1. Sorry, after discussion with Nikita we have decided to
>> use 'vdbe_' prefix for emitters even if they take parser in
>> first argument.
> Tnx, squashed.
>
>> 2. Mismatch of parameter names: error in header, error_src in source.
> Fixed.
1. Still is not fixed. Now mismatch of the parameter name in the
declaration and the name in the comment.
>
>> 3. Please, reduce the mess. Separate these OOM handlers.
> name = sqlite3DbStrDup(db, name_src);
> - if (name == NULL)
> + if (name == NULL) {
> + size_t size = strlen(name_src) + 1;
> + diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name");
> return -1;
> + }
> error = sqlite3DbStrDup(db, error_src);
> if (error == NULL) {
> sqlite3DbFree(db, name);
> - return -1;
> - }
> - if (name == NULL || error == NULL) {
> - size_t size =
> - (name != NULL ? strlen(error_src) :
> - strlen(name_src)) + 1;
> - const char *var_name = name != NULL ? "error" : "name";
> - diag_set(OutOfMemory, size, "sqlite3DbStrDup", var_name);
> - sqlite3DbFree(db, name);
> - sqlite3DbFree(db, error);
> + size_t size = strlen(error_src) + 1;
> + diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error");
> return -1;
> }
>
>
>> 6. Earlier this function could return either DONE or ERROR. Now
>> it can return SQL_TARANTOOL_ERROR, and I am not sure that the
>> behavior should be changed in such way. Please, explain why do you
>> need SQL_TARANTOOL_ERROR here.
> This is the sane behavior that parser has. I'd like to distinguish tarantool errors and out them from diag like Parser does.
> Take a look to the changes in sqlite3_errmsg.
2. I see the changes, but they are not linked with VdbeExec return value. The only
place where you have used SQL_TARANTOOL_ERROR is an assertion in sqlite3Step.
db->errCode finally is set not to VdbeExec return value, but to Vdbe->rc, that
is SQL_TARANTOOL_ERROR already.
I have reverted your changes at the end of OP_Halt:
@@ -1033,10 +1033,7 @@ case OP_Halt: {
p->rc = SQLITE_BUSY;
} else {
assert(rc==SQLITE_OK || (p->rc&0xff)==SQLITE_CONSTRAINT);
- if (p->rc != SQL_TARANTOOL_ERROR)
- rc = p->rc != SQLITE_OK ? SQLITE_ERROR : SQLITE_DONE;
- else
- rc = SQL_TARANTOOL_ERROR;
+ rc = p->rc ? SQLITE_ERROR : SQLITE_DONE;
}
goto vdbe_return;
And nothing is changed. The tests passed ok.
Why?
3. Please, put a new patch version at the end of letter.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 0edbda1d9..4588b1c6d 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -4145,4 +4145,50 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
> sqlite3DbFree(db, pWith);
> }
> }
> +
> +int
> +vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
> + int index_id, const char *name_src,
> + int tarantool_error_code,
> + const char *error_src, bool no_error)
> +{
> + struct Vdbe *v = sqlite3GetVdbe(parser);
> + assert(v != NULL);
> +
> + struct sqlite3 *db = parser->db;
> + char *name = NULL;
> + char *error = NULL;
> + name = sqlite3DbStrDup(db, name_src);
> + if (name == NULL) {
> + size_t size = strlen(name_src) + 1;
> + diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name");
> + return -1;
> + }
> + error = sqlite3DbStrDup(db, error_src);
> + if (error == NULL) {
> + sqlite3DbFree(db, name);
> + size_t size = strlen(error_src) + 1;
> + diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error");
> + return -1;
> + }
4. Why do you need to separate char *name and *error announcements from
assignments?
More information about the Tarantool-patches
mailing list