[patches] [PATCH V2] sql: change constraints checks order

[n.pettik]

Hello.

I suppose, description of the patch doesn't match with reallity:
>- Ban an opportunity for user to create table with multiple constraints
> with ON CONFLICT REPLACE option.

But I am still able to do it. For instance:
create table t1(id primary key, a, b not null on conflict replace, unique(a) on conflict replace);

Thus, add more test cases which will cover wider range of scenarios,
and fix this issue.

Also, there are a lot of codestyle violations.
Please, review your patch carefully before send it.

Format comments according to Tarantool's codestyle.
It is desirable to fit into 66 symbols while commenting.
Comments describing function start from /**
Comments inside function start from /*

>- Modify constraints checks execution order, if user specified ON
>  CONFLICT REPLACE

Put a dot at the end of sentences.

>diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>@@ -1822,6 +1822,51 @@ emitNewSysSpaceSequenceRecord(Parse *pParse, int space_id, const char reg_seq_id
>+check_multiple_replace_entries(Table * table)
>+{
>+	bool on_replace_used = false;
>+	Index * idx;
>+	int i;

Don't put extra space between asterisk and var name.
Moreover, you don't have to declare variable beforehand:
since it is used only as loop counter, you are able to
declare it right there:
for (int i = 0; ...)

>+	/* Check all NOT NULL constraints. Iterate through columns, because
>+	 * all info about NOT NULL is stored inside column structure.
>+	 *
>+	 */

Extra *.

>@@ -838,39 +784,105 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
>+		/* Create the new column data
>+		 */

Fit it into one line and put a dot.

>+			} else if (useTempTable) {
>+				sqlite3VdbeAddOp3(v, OP_Column, srcTab, j,
>+						  regCols + i + 1);
>+			} else {
>+				/* Otherwise useTempTable is true */

Do you really need this comment? I suppose it is TOO obvious...

>@@ -1026,11 +1039,16 @@ checkConstraintUnchanged(Expr * pExpr, int *aiChng)
>+ *    but unpresence still will be ensured with ABORT action. For other

Not "unpresence", but "absence", I guess.

>-	int onError;		/* Conflict resolution strategy */
>+	int onError;  		/* Conflict resolution strategy */

Redundant diff.

>+			/* Don't bother checking for NOT NULL on columns
>+			 * that do not change. */

Put the end of comment to the separate line.

>@@ -1203,7 +1282,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
>			sqlite3VdbeResolveLabel(v, allOk);
>		}
>	}
>-#endif				/* !defined(SQLITE_OMIT_CHECK) */
>+#endif	/* !defined(SQLITE_OMIT_CHECK) */

Redundant diff.

>+		if (!IsUniqueIndex(pIdx)) {
>+			sqlite3VdbeResolveLabel(v, addrUniqueOk);
>+			continue;	/* pIdx is not a UNIQUE index */
>+		}

I suppose comment is too obvious.

>+		/* If we are checking ON CONFLICT REPLACE before UPDATE/INSERT,
>+		 * then skip iteration if onError is not OE_Replace
>+		 */
>+		if (mode == CONSTRAINT_REPLACE_MODE &&
>+		    onError != ON_CONFLICT_ACTION_REPLACE) {
>+			sqlite3VdbeResolveLabel(v, addrUniqueOk);
>+			continue;
>+		}
>+
>+

Put a dot at the end of comment. Also, remove extra empty line.

>@@ -1315,15 +1432,12 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
>+		/* If pIdx we are doing INSERT/UPDATE OR IGNORE,
>+		 * INSERT/UPDATAE OR FAIL then skip uniqueness checks and let it to be
>+		 * done by Tarantool.

If pIdx ... ? I suppose you have missed smth.
Also, fit comment at least in 80 chars (but better in 66).

>@@ -586,16 +598,17 @@ sqlite3Update(Parse * pParse,		/* The parser context */
>	}
>
>	if (!isView) {
>-		int addr1 = 0;	/* Address of jump instruction */
>-		int bReplace = 0;	/* True if REPLACE conflict resolution might happen */
>+		int addr1 = 0;     /* Address of jump instruction. */
>+		u8 isReplace = 0;  /* True if REPLACE conflict resolution
>+				      might happen. */
>
>		/* Do constraint checks. */
>		assert(regOldPk > 0);
>		sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
>						iIdxCur, regNewPk,
>						regOldPk, chngPk, onError,
>-						labelContinue, &bReplace,
>-						aXRef);
>+						labelContinue, &isReplace,
>+						aXRef, CONSTRAINT_DEFAULT_MODE);
>
>		/* Do FK constraint checks. */
>		if (hasFK) {
>@@ -603,7 +616,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
>		}
>
>		/* Delete the index entries associated with the current record.  */
>-		if (bReplace || chngPk) {
>+		if (isReplace || chngPk) {
>			addr1 =
>				sqlite3VdbeAddOp4Int(v, OP_NotFound,
>						     iDataCur, 0, regKey,
>@@ -636,7 +649,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
>			sqlite3VdbeAddOp2(v, OP_Delete, iDataCur, 0);
>		}
>#endif
>-		if (bReplace || chngPk) {
>+		if (isReplace || chngPk) {
>			sqlite3VdbeJumpHere(v, addr1);
>		}

Do you really need this diff only for renaming "bReplace" to "isReplace"?
Anyway, in tarantool's codestyle they are both incorrent.

>+++ b/src/box/sql/update.c
>@@ -546,6 +546,18 @@ sqlite3Update(Parse * pParse,		/* The parser context */
>		}
>	}
>
>+	if (!isView) {
>+		u8 isReplace = 0;	/* True if REPLACE conflict resolution might happen */
>+
>+		/* Do constraint checks. */
>+		assert(regOldPk > 0);
>+		sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
>+						iIdxCur, regNewPk,
>+						regOldPk, chngPk, onError,
>+						labelContinue, &isReplace,
>+						aXRef, CONSTRAINT_REPLACE_MODE);
>+	}
>+

Don't put comment at the same line as variable's declaration.
(Moreover, it doesn't fit into even 80 chars)
Why do you pass local variable "isReplace"? You don't use after that,
and it will be destroyed right after function's call due to its scope.

Also, add comment describing the fact that we should firstly emit
bytecode for contraints in "replace mode", and after before triggers
are coded -- in "default mode". Because now it looks like you are
generating the same bytecode twice.