[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Kirill Shcherbatov]

> 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.

> 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;
        }

> 4. This error is duplicated already above, it is not?
Damn, you are right.
-               error = sqlite3DbStrDup(db, error);

> 5. Please, use tt_sprintf. Do not declare big arrays on the
> stack when possible.
-               char error[DIAG_ERRMSG_MAX];
-               snprintf(error, DIAG_ERRMSG_MAX,
-                        tnt_errcode_desc(ER_TRIGGER_EXISTS), zName);
+               const char *error_msg =
+                       tt_sprintf(tnt_errcode_desc(ER_TRIGGER_EXISTS), zName);

> 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.