[patches] [constraints 2/2] sql: change constraints checks order

[n.pettik at corp.mail.ru]

>Signed-off-by: Bulat Niatshin <niatshin at tarantool.org>

Please add name of your branch, link to the issue and short
patch description to cover letter. Moreover, AFAIK we don't
use "signed-off" option.

>+static bool
>+checkMultipleReplaceEntries(Table * pTab)
>+{
>+	bool onReplaceUsed = false;
>+	Index * pIdx;
>+	int i;
>+
>+	for (pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext) {
>+		if (pIdx->onError == OE_Replace) {
>+	       		if (onReplaceUsed == true) {
>+				return true;
>+			}
>+			onReplaceUsed = true;
>+		}
>+	}
>+
>+	for (i = 0; i < pTab->nCol; i++) {
>+		u8 onError = pTab->aCol[i].notNull;
>+		if (onError == OE_Replace) {
>+	       		if (onReplaceUsed == true) {
>+				return true;
>+			}
>+			onReplaceUsed = true;
>+		}
>+	}
>+
>+	return false;
>+}

Please, add comment(s)/description to this function. I don't understand
why we should firstly check all indexes' actions and later -- all columns'
ones.  Moreover, now we are integrating Tarantool DD into SQL, so
rewrite this function using Tarantool facilities: get space by its id
and iterate through fields to check their actions. For this reason, use
Tarantool codestyle while implementing this routine.

>+	for (i = 0; i < pTab->nCol; i++) {
>+		u8 onError = pTab->aCol[i].notNull;
>+		if (onError == OE_Replace) {
>+	       		if (onReplaceUsed == true) {
>+				return true;
>+			}
>+			onReplaceUsed = true;
>+		}
>+	}

I guess, before new loop iteration you should reset onReplaceUsed
to default value.

>+	if (checkMultipleReplaceEntries(p)) {
>+		sqlite3ErrorMsg(pParse,
>+				"in table %s - only one ON CONFLICT REPLACE is allowed",
>+				p->zName);
>+		return;
>+	}

I suggest to make following error message:
"Table %s can feature only one ON CONFLICT REPLACE constraint.", or
"Table %s is allowed to feature only one ON CONFLICT REPLACE constraint."
Or kind of variation -- it sounds more naturally.
Moreover, code should fit into 80 chars.

>+		CREATE TABLE t1(a INT PRIMARY KEY,
>+						b INT UNIQUE ON CONFLICT REPLACE,
>+						c INT UNIQUE
>+						);
>+	]], {

Bad alignment. In other tests below it repeats.

>+	int isReplace;	/* Set to true if constraints may cause a replace */
>+	int bUseSeek;	/* True to use OPFLAG_SEEKRESULT */

Follow one code style: whether you call them u8/bool bIsFoo or int iUseFoo...

>+	/* Run the BEFORE and INSTEAD OF triggers, if there are any
>+	 */

Finish comment with a dot. Moreover, make this comment fit into one line.

>+			} else {
>+				assert(pSelect == 0);	/* Otherwise useTempTable is true */

It doesn't fit into 80 chars.

>+		/* If this is an INSERT on a view with an INSTEAD OF INSERT trigger,
>+		 * do not attempt any conversions before assembling the record.
>+		 * If this is a real table, attempt conversions as required by the
>+		 * table column affinities.
>+		 */

Either this comment.

>+	/* Update the count of rows that are inserted
>+	 */

Finish comment with a dot. Moreover, make this comment fit into one line.

>+		/* Generate code to check constraints and generate index keys
>+		   and do the insertion.
>+		 */

You forget one asterisk inside the comment.

>+#define CONSTRAINT_CHECK_REPLACE 1
>+#define CONSTRAINT_MAIN_MODE 2

Add complete description of these flags.
Also, it would be better to name it CONSTRAINT_DEFAULT_MODE or something like that.

>@@ -1066,7 +1076,8 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
>-				int *aiChng)		/* column i is unchanged if aiChng[i]<0 */
>+				int *aiChng,		/* column i is unchanged if aiChng[i]<0 */
>+				int mode)

Look, sqlite3GenerateConstraintChecks() features really great and detailed comment.
If you add auxiliary flag, which significantly changes behaviour of this function,
you should update comment in every detail. Also, it would be great if you add comments
to all if-else branches which are affected by your flag.

>+		} else if (onError == OE_Default && mode == CONSTRAINT_MAIN_MODE) {

It doesn't fit into 80 chars.

>+		assert((mode == CONSTRAINT_MAIN_MODE && (onError == OE_Rollback
>+		       || onError == OE_Abort || onError == OE_Fail
>+		       || onError == OE_Ignore)) ||
>+		       (mode == CONSTRAINT_CHECK_REPLACE && (onError == OE_Abort ||
>+		       onError == OE_Replace)));

Either this. Moreover, use one style of carrying binary condition.

>+		/* Find out what action to take in case there is a uniqueness conflict */

It doesn't fit into 80 chars.

>+		assert((mode == CONSTRAINT_MAIN_MODE && (onError == OE_Rollback
>+		       || onError == OE_Abort || onError == OE_Fail
>+		       || onError == OE_Ignore || onError == OE_Replace)) ||
>+		       (mode == CONSTRAINT_CHECK_REPLACE && (onError == OE_Abort ||
>+		       onError == OE_Replace)));

Either this. Moreover, use one style of carrying binary condition.

>+		int bReplace = 0;	/* True if REPLACE conflict resolution might happen */

It doesn't fit into 80 chars.

>+						aXRef, CONSTRAINT_CHECK_REPLACE);

Either this.