[tarantool-patches] Re: [PATCH v2 11/11] sql: VDBE tests for trigger existence
Kirill Shcherbatov
kshcherbatov at tarantool.org
Thu Jun 14 19:12:29 MSK 2018
> 1. If this function is method of parser then its prefix must be 'parser_'.
> And here you do not an abortion. You emit the code of abortion if needed.
> So this function can be named like this:
> 2. struct Parse
> 3. Why is the function stored in sql.c? As far I remember sql.c/.h is a
> link between sqlite and Tarantool code, but vdbe_abort_execution_on_exists
> is used in sqlite only.
Moved to build.c.
+int
+parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
+ int index_id, const char *name,
+ int tarantool_error_code, bool no_error)
> 4. struct Vdbe. Please check other places yourself.
+ struct Vdbe *v = sqlite3GetVdbe(parser);
+ struct sqlite3 *db = parser->db;
> 5. What is 'testcase'? Why do you need it here?
> 6. It is assigned to this value few lines above.
> 7. diag is never NULL, because TX thread consists of fibers, and
> each fiber has diag. Please, remove the assertion and just inline
> diag_get() on the next line.
@@ -1454,19 +1454,14 @@ sqlite3_errmsg(sqlite3 * db)
z = sqlite3ErrStr(SQLITE_NOMEM_BKPT);
} else {
testcase(db->pErr == 0);
- z = (char *)sqlite3_value_text(db->pErr);
assert(!db->mallocFailed);
if (db->errCode != SQL_TARANTOOL_ERROR) {
- testcase(db->pErr == 0);
- z = (char *) sqlite3_value_text(db->pErr);
assert(!db->mallocFailed);
+ z = (char *)sqlite3_value_text(db->pErr);
if (z == NULL)
z = sqlite3ErrStr(db->errCode);
} else {
- struct diag *diag = diag_get();
- assert(diag != NULL);
- struct error *error = diag_last_error(diag);
- z = error->errmsg;
+ z = diag_last_error(diag_get())->errmsg;
}
}
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index 276f881..4f7dcbe 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -4537,4 +4537,24 @@ extern int sqlite3InitDatabase(sqlite3 * db);
>> enum on_conflict_action
>> table_column_nullable_action(struct Table *tab, uint32_t column);
>>
>> +/**
>> + * Generate VDBE code to halt execution with correct error if
>> + * the object with specified name is already present in
>> + * specified space.
>> + *
>> + * @param parser Parsing context.
>> + * @param space_id Space to lookup identifier.
>> + * @param index_id Index identifier containing string primary key.
>> + * @param name Of object to test existency.
>
> 8. No such word 'existency'.
Songs good for me (https://en.oxforddictionaries.com/definition/existency):
1The fact or state of existing; continuance of being.
2Something that exists; a being, an entity; = "existence".
- * @param name Of object to test existency.
+ * @param name Of object to test existence.
> 9. Out of 66.
+ * @param tarantool_error_code error to raise on object exists.
> 10. pParse->nErr++. Or find a way to delete nErr. It is very annoying and
> useless attribute.
+ pParse->nErr++;
> 11. Why here you did not put empty line after 'if P1', but did in the
> next hunk?
> 12. I am afraid of this way will not work, when a code has multiple
> arguments. And when type is not ClientError. And when it has a single
> non-string argument. And when we want to know where the error was
> generated. Here diag_set always saves filename "vdbe.c" and line "1012".
> I think we should find another way to emit an error. One possible way
> is to create and error object during parsing and save it by pointer in
> p4. When the error occurs, the error object is set into the diag object.
> But maybe you will find another way.
I've done this this way:
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index c52c41d..0a731df 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -4175,8 +4175,9 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
int
parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
int index_id, const char *name,
- int tarantool_error_code, bool no_error)
+ struct error *error, bool no_error)
{
+ error_ref(error);
struct Vdbe *v = sqlite3GetVdbe(parser);
assert(v != NULL);
@@ -4197,11 +4198,12 @@ parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
P4_DYNAMIC);
sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
if (no_error) {
+ error_unref(error);
sqlite3VdbeAddOp0(v, OP_Halt);
} else {
sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
- ON_CONFLICT_ACTION_FAIL, 0, name, P4_STATIC);
- sqlite3VdbeChangeP5(v, tarantool_error_code);
+ ON_CONFLICT_ACTION_FAIL, 0, (void *)error,
+ P4_ERROROBJ);
}
sqlite3VdbeAddOp1(v, OP_Close, cursor);
return 0;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 4f755e4..d86ace5 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4546,7 +4546,7 @@ table_column_nullable_action(struct Table *tab, uint32_t column);
* @param space_id Space to lookup identifier.
* @param index_id Index identifier containing string primary key.
* @param name Of object to test existence.
- * @param tarantool_error_code error to raise on object exists.
+ * @param error object to raise on target exists.
* @param no_error Do not raise error flag.
*
* @retval -1 on memory allocation error.
@@ -4555,6 +4555,6 @@ table_column_nullable_action(struct Table *tab, uint32_t column);
int
parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
int index_id, const char *name,
- int tarantool_error_code, bool no_error);
+ struct error *error, bool no_error);
#endif /* SQLITEINT_H */
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index d7ecdf1..4cb89fe 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -173,13 +173,13 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s
goto trigger_cleanup;
}
if (!pParse->parse_only) {
+ struct error *error =
+ error_object(ClientError, ER_TRIGGER_EXISTS, zName);
if (parser_emit_execution_halt_on_exists(pParse, BOX_TRIGGER_ID,
- 0,
- zName,
- ER_TRIGGER_EXISTS,
+ 0, zName, error,
(noErr != 0)) != 0) {
pParse->rc = SQL_TARANTOOL_ERROR;
pParse->nErr++;
goto trigger_cleanup;
}
}
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index dfc7b71..c3109ab 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -959,7 +959,7 @@ case OP_HaltIfNull: { /* in3 */
* VDBE, but do not rollback the transaction.
*
* If P4 is not null then it is an error message string.
- * If P1 is not SQL_TARANTOOL_ERROR,
+ *
* P5 is a value between 0 and 4, inclusive, that modifies the P4 string.
*
* 0: (no change)
@@ -968,8 +968,6 @@ case OP_HaltIfNull: { /* in3 */
* 3: CHECK constraint failed: P4
* 4: FOREIGN KEY constraint failed: P4
*
- * If P1 is SQL_TARANTOOL_ERROR, P5 contain ClientError code.
- *
* If P5 is not zero and P4 is NULL, then everything after the ":" is
* omitted.
*
@@ -1008,8 +1006,8 @@ case OP_Halt: {
p->errorAction = (u8)pOp->p2;
p->pc = pcx;
if (p->rc) {
- if (pOp->p5 != 0 && p->rc == SQL_TARANTOOL_ERROR) {
- diag_set(ClientError, pOp->p5, pOp->p4.z);
+ if (p->rc == SQL_TARANTOOL_ERROR) {
+ diag_add_error(diag_get(), (struct error *)pOp->p4.z);
} else if (pOp->p5 != 0) {
static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
"FOREIGN KEY" };
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 77fa41a..66e6954 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -149,6 +149,8 @@ typedef struct VdbeOpList VdbeOpList;
#define P4_PTR (-18) /* P4 is a generic pointer */
#define P4_KEYDEF (-19) /* P4 is a pointer to key_def structure. */
#define P4_SPACEPTR (-20) /* P4 is a space pointer */
+/** P4 is an error object. */
+#define P4_ERROROBJ (-21)
/* Error message codes for OP_Halt */
#define P5_ConstraintNotNull 1
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 7b3c13d..8d89894 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -981,6 +981,10 @@ freeP4(sqlite3 * db, int p4type, void *p4)
}
break;
}
+ case P4_ERROROBJ: {
+ error_unref(p4);
+ break;
+ }
}
}
diff --git a/src/diag.h b/src/diag.h
index 0ccf86d..502b294 100644
--- a/src/diag.h
+++ b/src/diag.h
@@ -261,6 +261,9 @@ struct error *
BuildSocketError(const char *file, unsigned line, const char *socketname,
const char *format, ...);
+#define error_object(class, ...) \
+ Build##class(__FILE__, __LINE__, ##__VA_ARGS__);
+
#define diag_set(class, ...) do { \
/* Preserve the original errno. */ \
int save_errno = errno; \
More information about the Tarantool-patches
mailing list