[tarantool-patches] Re: [PATCH v2 11/11] sql: VDBE tests for trigger existence

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

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