[tarantool-patches] [PATCH v2 0/4] Introduce row_count() function

[tarantool-patches] [PATCH v2 0/4] Introduce row_count() function

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/commits/np/gh-2181-rework-change-counter-v2
Issue: https://github.com/tarantool/tarantool/issues/2181

This patch-set slightly fixes behaviour of changes() function (in
order to be closer to ROW_COUNT analogue from ANSI SQL), renames
it row_count() and removes total_changes() function.

All these changes take place according to comments in @dev
mailing list.

Changes in v2:

Actually, it is kind of completely independent approach and it has
nothing in common with first iteration (which was aimed at dealing
with behaviour of total_changes() during transaction).

Nikita Pettik (4):
  sql: don't increment row count on FK creation within CREATE TABLE
  sql: account REPLACE as two row changes
  sql: remove total_changes() function
  sql: rename changes() to row_count()

 src/box/execute.c           |   2 +-
 src/box/sql/build.c         |   3 +-
 src/box/sql/func.c          |  34 +---------
 src/box/sql/insert.c        |   2 +-
 src/box/sql/main.c          |  32 ++-------
 src/box/sql/sqliteInt.h     |  15 +++--
 src/box/sql/vdbeaux.c       |  16 ++---
 test/sql/row-count.result   | 157 ++++++++++++++++++++++++++++++++++++++++++++
 test/sql/row-count.test.lua |  54 +++++++++++++++
 9 files changed, 237 insertions(+), 78 deletions(-)
 create mode 100644 test/sql/row-count.result
 create mode 100644 test/sql/row-count.test.lua

-- 
2.15.1

[tarantool-patches] [PATCH v2 1/4] sql: don't increment row count on FK creation within CREATE TABLE

[Nikita Pettik]

We have agreement that each successful DDL operation returns 1 (one) as
a row count (via IProto protocol or changes() SQL function), despite
the number of other created objects (e.g. indexes, sequences, FK
constraints etc).

Needed for #2181
---
 src/box/sql/build.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5b3348bd2..e28856e26 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1415,7 +1415,8 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
 			  constr_tuple_reg + 9);
 	sqlite3VdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0,
 			  constr_tuple_reg + 9);
-	sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
+	if (parse_context->pNewTable == NULL)
+		sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
 	save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
 		    vdbe->nOp - 1);
 	sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);
-- 
2.15.1

[tarantool-patches] [PATCH v2 2/4] sql: account REPLACE as two row changes

[Nikita Pettik]

In our SQL implementation REPLACE acts as DELETE + INSERT, so we should
account it as two row changes.

Needed for #2181
---
 src/box/sql/insert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index fd05c0254..a53568810 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1076,7 +1076,7 @@ process_index:  ;
 				sql_triggers_exist(tab, TK_DELETE, NULL, NULL);
 			sql_generate_row_delete(parse_context, tab, trigger,
 						cursor, idx_key_reg, part_count,
-						false,
+						true,
 						ON_CONFLICT_ACTION_REPLACE,
 						ONEPASS_SINGLE, -1);
 			sqlite3VdbeResolveLabel(v, skip_index);
-- 
2.15.1

[tarantool-patches] [PATCH v2 3/4] sql: remove total_changes() function

[Nikita Pettik]

Part of #2181
---
 src/box/sql/func.c      | 17 -----------------
 src/box/sql/main.c      | 15 ---------------
 src/box/sql/sqliteInt.h |  4 ----
 src/box/sql/vdbeaux.c   |  1 -
 4 files changed, 37 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 8c34cbb3d..5ab64821b 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -600,21 +600,6 @@ changes(sqlite3_context * context, int NotUsed, sqlite3_value ** NotUsed2)
 	sqlite3_result_int(context, sqlite3_changes(db));
 }
 
-/*
- * Implementation of the total_changes() SQL function.  The return value is
- * the same as the sqlite3_total_changes() API function.
- */
-static void
-total_changes(sqlite3_context * context, int NotUsed, sqlite3_value ** NotUsed2)
-{
-	sqlite3 *db = sqlite3_context_db_handle(context);
-	UNUSED_PARAMETER2(NotUsed, NotUsed2);
-	/* IMP: R-52756-41993 This function is a wrapper around the
-	 * sqlite3_total_changes() C/C++ interface.
-	 */
-	sqlite3_result_int(context, sqlite3_total_changes(db));
-}
-
 /*
  * A structure defining how to do GLOB-style comparisons.
  */
@@ -1869,8 +1854,6 @@ sqlite3RegisterBuiltinFunctions(void)
 		FUNCTION(version, 0, 0, 0, sql_func_version, AFFINITY_TEXT),
 		FUNCTION(quote, 1, 0, 0, quoteFunc, AFFINITY_TEXT),
 		VFUNCTION(changes, 0, 0, 0, changes, AFFINITY_INTEGER),
-		VFUNCTION(total_changes, 0, 0, 0, total_changes,
-			  AFFINITY_INTEGER),
 		FUNCTION(replace, 3, 0, 0, replaceFunc, AFFINITY_TEXT),
 		FUNCTION(zeroblob, 1, 0, 0, zeroblobFunc, AFFINITY_BLOB),
 		FUNCTION(substr, 2, 0, 0, substrFunc, AFFINITY_TEXT),
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index e1d61621f..76c152c68 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -498,21 +498,6 @@ sqlite3_changes(sqlite3 * db)
 	return db->nChange;
 }
 
-/*
- * Return the number of changes since the database handle was opened.
- */
-int
-sqlite3_total_changes(sqlite3 * db)
-{
-#ifdef SQLITE_ENABLE_API_ARMOR
-	if (!sqlite3SafetyCheckOk(db)) {
-		(void)SQLITE_MISUSE_BKPT;
-		return 0;
-	}
-#endif
-	return db->nTotalChange;
-}
-
 /*
  * Close all open savepoints.
  * This procedure is trivial as savepoints are allocated on the "region" and
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 1999ff568..73ca35c11 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -657,9 +657,6 @@ sqlite3_randomness(int N, void *P);
 int
 sqlite3_changes(sqlite3 *);
 
-int
-sqlite3_total_changes(sqlite3 *);
-
 void *
 sqlite3_user_data(sqlite3_context *);
 
@@ -1516,7 +1513,6 @@ struct sqlite3 {
 	u8 mTrace;		/* zero or more SQLITE_TRACE flags */
 	u32 magic;		/* Magic number for detect library misuse */
 	int nChange;		/* Value returned by sqlite3_changes() */
-	int nTotalChange;	/* Value returned by sqlite3_total_changes() */
 	int aLimit[SQLITE_N_LIMIT];	/* Limits */
 	int nMaxSorterMmap;	/* Maximum size of regions mapped by sorter */
 	struct sqlite3InitInfo {	/* Information used during initialization */
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index e0bf92acf..c0945d36d 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -3453,7 +3453,6 @@ void
 sqlite3VdbeSetChanges(sqlite3 * db, int nChange)
 {
 	db->nChange = nChange;
-	db->nTotalChange += nChange;
 }
 
 /*
-- 
2.15.1

[tarantool-patches] [PATCH v2 4/4] sql: rename changes() to row_count()

[Nikita Pettik]

changes() is legacy function which came from SQLite. However, its name
is not perfect since it's "number of rows affected", not"number of rows
changed". Moreover, to make it real row count, we are nullifying counter
representing it in case operation is not accounted for DML or DDL.
For example, before this patch behavior was like:

INSERT INTO t VALUES (1), (2), (3);
START TRANSACTION;
SELECT changes();
---
- - [3]
...

As one can see, row counter remained unchanged from last INSERT
operation. However, now START TRANSACTION set it to 0 and as a
consequence row_count() also returns 0.

Closes #2181
---
 src/box/execute.c           |   2 +-
 src/box/sql/func.c          |  17 +----
 src/box/sql/main.c          |  17 ++---
 src/box/sql/sqliteInt.h     |  11 +++-
 src/box/sql/vdbeaux.c       |  15 +++--
 test/sql/row-count.result   | 157 ++++++++++++++++++++++++++++++++++++++++++++
 test/sql/row-count.test.lua |  54 +++++++++++++++
 7 files changed, 234 insertions(+), 39 deletions(-)
 create mode 100644 test/sql/row-count.result
 create mode 100644 test/sql/row-count.test.lua

diff --git a/src/box/execute.c b/src/box/execute.c
index d75487498..e45044488 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -649,7 +649,7 @@ err:
 		if (iproto_reply_map_key(out, map_size, IPROTO_SQL_INFO) != 0)
 			goto err;
 		uint64_t id_count = 0;
-		int changes = sqlite3_changes(db);
+		int changes = db->nChange;
 		int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
 			   mp_sizeof_uint(changes);
 		if (!stailq_empty(autoinc_id_list)) {
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 5ab64821b..f0bf81ac3 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -585,21 +585,6 @@ randomBlob(sqlite3_context * context, int argc, sqlite3_value ** argv)
 	}
 }
 
-/*
- * Implementation of the changes() SQL function.
- *
- * IMP: R-62073-11209 The changes() SQL function is a wrapper
- * around the sqlite3_changes() C/C++ function and hence follows the same
- * rules for counting changes.
- */
-static void
-changes(sqlite3_context * context, int NotUsed, sqlite3_value ** NotUsed2)
-{
-	sqlite3 *db = sqlite3_context_db_handle(context);
-	UNUSED_PARAMETER2(NotUsed, NotUsed2);
-	sqlite3_result_int(context, sqlite3_changes(db));
-}
-
 /*
  * A structure defining how to do GLOB-style comparisons.
  */
@@ -1853,7 +1838,7 @@ sqlite3RegisterBuiltinFunctions(void)
 		FUNCTION(nullif, 2, 0, 1, nullifFunc, 0),
 		FUNCTION(version, 0, 0, 0, sql_func_version, AFFINITY_TEXT),
 		FUNCTION(quote, 1, 0, 0, quoteFunc, AFFINITY_TEXT),
-		VFUNCTION(changes, 0, 0, 0, changes, AFFINITY_INTEGER),
+		VFUNCTION(row_count, 0, 0, 0, sql_row_count, AFFINITY_INTEGER),
 		FUNCTION(replace, 3, 0, 0, replaceFunc, AFFINITY_TEXT),
 		FUNCTION(zeroblob, 1, 0, 0, zeroblobFunc, AFFINITY_BLOB),
 		FUNCTION(substr, 2, 0, 0, substrFunc, AFFINITY_TEXT),
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 76c152c68..8574d6464 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -483,19 +483,12 @@ setupLookaside(sqlite3 * db, void *pBuf, int sz, int cnt)
 	return SQLITE_OK;
 }
 
-/*
- * Return the number of changes in the most recent call to sqlite3_exec().
- */
-int
-sqlite3_changes(sqlite3 * db)
+void
+sql_row_count(struct sqlite3_context *context, MAYBE_UNUSED int unused1,
+	      MAYBE_UNUSED sqlite3_value **unused2)
 {
-#ifdef SQLITE_ENABLE_API_ARMOR
-	if (!sqlite3SafetyCheckOk(db)) {
-		(void)SQLITE_MISUSE_BKPT;
-		return 0;
-	}
-#endif
-	return db->nChange;
+	sqlite3 *db = sqlite3_context_db_handle(context);
+	sqlite3_result_int(context, db->nChange);
 }
 
 /*
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 73ca35c11..6247e12d7 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -654,8 +654,12 @@ sqlite3_user_data(sqlite3_context *);
 void
 sqlite3_randomness(int N, void *P);
 
-int
-sqlite3_changes(sqlite3 *);
+/**
+ * Return the number of affected rows in the last SQL statement.
+ */
+void
+sql_row_count(struct sqlite3_context *context, MAYBE_UNUSED int unused1,
+	      MAYBE_UNUSED sqlite3_value **unused2);
 
 void *
 sqlite3_user_data(sqlite3_context *);
@@ -1512,7 +1516,8 @@ struct sqlite3 {
 	u8 suppressErr;		/* Do not issue error messages if true */
 	u8 mTrace;		/* zero or more SQLITE_TRACE flags */
 	u32 magic;		/* Magic number for detect library misuse */
-	int nChange;		/* Value returned by sqlite3_changes() */
+	/** Value returned by sql_row_count(). */
+	int nChange;
 	int aLimit[SQLITE_N_LIMIT];	/* Limits */
 	int nMaxSorterMmap;	/* Maximum size of regions mapped by sorter */
 	struct sqlite3InitInfo {	/* Information used during initialization */
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index c0945d36d..615a0f064 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2472,16 +2472,17 @@ sqlite3VdbeHalt(Vdbe * p)
 			}
 		}
 
-		/* If this was an INSERT, UPDATE or DELETE and no statement transaction
-		 * has been rolled back, update the database connection change-counter.
+		/*
+		 * If this was an INSERT, UPDATE or DELETE and
+		 * statement transaction has been rolled back,
+		 * update the database connection change-counter.
+		 * Other statements should return 0 (zero).
 		 */
 		if (p->changeCntOn) {
-			if (eStatementOp != SAVEPOINT_ROLLBACK) {
-				sqlite3VdbeSetChanges(db, p->nChange);
-			} else {
-				sqlite3VdbeSetChanges(db, 0);
-			}
+			sqlite3VdbeSetChanges(db, p->nChange);
 			p->nChange = 0;
+		} else {
+			db->nChange = 0;
 		}
 	}
 
diff --git a/test/sql/row-count.result b/test/sql/row-count.result
new file mode 100644
index 000000000..7577d2795
--- /dev/null
+++ b/test/sql/row-count.result
@@ -0,0 +1,157 @@
+test_run = require('test_run').new()
+---
+...
+engine = test_run:get_cfg('engine')
+---
+...
+box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
+---
+...
+-- Test cases concerning row count calculations.
+--
+box.sql.execute("CREATE TABLE t1 (s1 CHAR(10) PRIMARY KEY);")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [1]
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [0]
+...
+box.sql.execute("CREATE TABLE t2 (s1 CHAR(10) PRIMARY KEY, s2 CHAR(10) REFERENCES t1 ON DELETE CASCADE);")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [1]
+...
+box.sql.execute("CREATE TABLE t3 (i1 INT PRIMARY KEY, i2 INT);")
+---
+...
+box.sql.execute("INSERT INTO t3 VALUES (0, 0);")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [1]
+...
+box.sql.execute("CREATE TRIGGER x AFTER DELETE ON t1 FOR EACH ROW BEGIN UPDATE t3 SET i1 = i1 + ROW_COUNT(); END;")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [1]
+...
+box.sql.execute("INSERT INTO t1 VALUES ('a');")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [1]
+...
+box.sql.execute("INSERT INTO t2 VALUES ('a','a');")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [1]
+...
+box.sql.execute("INSERT INTO t1 VALUES ('b'), ('c'), ('d');")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [3]
+...
+-- REPLACE is accounted for two operations: DELETE + INSERT.
+box.sql.execute("REPLACE INTO t2 VALUES('a', 'c');")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [2]
+...
+box.sql.execute("DELETE FROM t1;")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [4]
+...
+box.sql.execute("INSERT INTO t3 VALUES (1, 1), (2, 2), (3, 3);")
+---
+...
+box.sql.execute("TRUNCATE TABLE t3;")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [0]
+...
+box.sql.execute("INSERT INTO t3 VALUES (1, 1), (2, 2), (3, 3);")
+---
+...
+box.sql.execute("UPDATE t3 SET i2 = 666;")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [3]
+...
+-- All statements which are not accounted as DML should
+-- return 0 (zero) as a row count.
+--
+box.sql.execute("START TRANSACTION;")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [0]
+...
+box.sql.execute("COMMIT;")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [0]
+...
+box.sql.execute("COMMIT;")
+---
+- error: cannot commit - no transaction is active
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [0]
+...
+box.sql.execute("ANALYZE;")
+---
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [0]
+...
+box.sql.execute("EXPLAIN QUERY PLAN INSERT INTO t1 VALUES ('b'), ('c'), ('d');")
+---
+- - [0, 0, 0, 'SCAN TABLE T2']
+...
+box.sql.execute("SELECT ROW_COUNT();")
+---
+- - [0]
+...
+box.sql.execute('PRAGMA recursive_triggers')
+---
+- - [1]
+...
+-- Clean-up.
+--
+box.sql.execute("DROP TABLE t2;")
+---
+...
+box.sql.execute("DROP TABLE t3;")
+---
+...
+box.sql.execute("DROP TABLE t1;")
+---
+...
diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua
new file mode 100644
index 000000000..38d3520c2
--- /dev/null
+++ b/test/sql/row-count.test.lua
@@ -0,0 +1,54 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
+
+-- Test cases concerning row count calculations.
+--
+box.sql.execute("CREATE TABLE t1 (s1 CHAR(10) PRIMARY KEY);")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("CREATE TABLE t2 (s1 CHAR(10) PRIMARY KEY, s2 CHAR(10) REFERENCES t1 ON DELETE CASCADE);")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("CREATE TABLE t3 (i1 INT PRIMARY KEY, i2 INT);")
+box.sql.execute("INSERT INTO t3 VALUES (0, 0);")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("CREATE TRIGGER x AFTER DELETE ON t1 FOR EACH ROW BEGIN UPDATE t3 SET i1 = i1 + ROW_COUNT(); END;")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("INSERT INTO t1 VALUES ('a');")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("INSERT INTO t2 VALUES ('a','a');")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("INSERT INTO t1 VALUES ('b'), ('c'), ('d');")
+box.sql.execute("SELECT ROW_COUNT();")
+-- REPLACE is accounted for two operations: DELETE + INSERT.
+box.sql.execute("REPLACE INTO t2 VALUES('a', 'c');")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("DELETE FROM t1;")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("INSERT INTO t3 VALUES (1, 1), (2, 2), (3, 3);")
+box.sql.execute("TRUNCATE TABLE t3;")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("INSERT INTO t3 VALUES (1, 1), (2, 2), (3, 3);")
+box.sql.execute("UPDATE t3 SET i2 = 666;")
+box.sql.execute("SELECT ROW_COUNT();")
+
+-- All statements which are not accounted as DML should
+-- return 0 (zero) as a row count.
+--
+box.sql.execute("START TRANSACTION;")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("COMMIT;")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("COMMIT;")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("ANALYZE;")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute("EXPLAIN QUERY PLAN INSERT INTO t1 VALUES ('b'), ('c'), ('d');")
+box.sql.execute("SELECT ROW_COUNT();")
+box.sql.execute('PRAGMA recursive_triggers')
+
+-- Clean-up.
+--
+box.sql.execute("DROP TABLE t2;")
+box.sql.execute("DROP TABLE t3;")
+box.sql.execute("DROP TABLE t1;")
\ No newline at end of file
-- 
2.15.1

[tarantool-patches] Re: [PATCH v2 2/4] sql: account REPLACE as two row changes

[Vladislav Shpilevoy]

Thanks for the patch!

On 13/11/2018 19:11, Nikita Pettik wrote:
> In our SQL implementation REPLACE acts as DELETE + INSERT, so we should
> account it as two row changes.
> 
> Needed for #2181
> ---
>   src/box/sql/insert.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)> 
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index fd05c0254..a53568810 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1076,7 +1076,7 @@ process_index:  ;
>   				sql_triggers_exist(tab, TK_DELETE, NULL, NULL);
>   			sql_generate_row_delete(parse_context, tab, trigger,
>   						cursor, idx_key_reg, part_count,
> -						false,
> +						true,
>   						ON_CONFLICT_ACTION_REPLACE,
>   						ONEPASS_SINGLE, -1);
>   			sqlite3VdbeResolveLabel(v, skip_index);
> 

I added a test on the branch and here:

==========================================================

commit faff2944f609b443ec7c32b03ed8101e381f2828
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Wed Nov 14 15:18:02 2018 +0300

     Review fix

diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index e75f79110..d077ee861 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -742,11 +742,11 @@ box.sql.execute('drop table test3')
  --
  -- Ensure that FK inside CREATE TABLE does not affect rowcount.
  --
-cn:execute('create table test (id integer primary key)')
+cn:execute('create table test (id integer primary key, a integer)')
  ---
  - rowcount: 1
  ...
-cn:execute('create table test2 (id integer primary key, ref integer references test)')
+cn:execute('create table test2 (id integer primary key, ref integer references test(id))')
  ---
  - rowcount: 1
  ...
@@ -754,6 +754,18 @@ cn:execute('drop table test2')
  ---
  - rowcount: 1
  ...
+--
+-- Ensure that REPLACE is accounted twice in rowcount. As delete +
+-- insert.
+--
+cn:execute('insert into test values(1, 1)')
+---
+- rowcount: 1
+...
+cn:execute('insert or replace into test values(1, 2)')
+---
+- rowcount: 2
+...
  cn:execute('drop table test')
  ---
  - rowcount: 1
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index fb5685a9e..1470edad7 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -233,9 +233,16 @@ box.sql.execute('drop table test3')
  --
  -- Ensure that FK inside CREATE TABLE does not affect rowcount.
  --
-cn:execute('create table test (id integer primary key)')
-cn:execute('create table test2 (id integer primary key, ref integer references test)')
+cn:execute('create table test (id integer primary key, a integer)')
+cn:execute('create table test2 (id integer primary key, ref integer references test(id))')
  cn:execute('drop table test2')
+
+--
+-- Ensure that REPLACE is accounted twice in rowcount. As delete +
+-- insert.
+--
+cn:execute('insert into test values(1, 1)')
+cn:execute('insert or replace into test values(1, 2)')
  cn:execute('drop table test')
  
  cn:close()

[tarantool-patches] Re: [PATCH v2 1/4] sql: don't increment row count on FK creation within CREATE TABLE

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 1 comment below, my fixes at the end of the email
and on the branch.

On 13/11/2018 19:11, Nikita Pettik wrote:
> We have agreement that each successful DDL operation returns 1 (one) as
> a row count (via IProto protocol or changes() SQL function), despite
> the number of other created objects (e.g. indexes, sequences, FK
> constraints etc).
> 
> Needed for #2181
> ---
>   src/box/sql/build.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 5b3348bd2..e28856e26 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1415,7 +1415,8 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
>   			  constr_tuple_reg + 9);
>   	sqlite3VdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0,
>   			  constr_tuple_reg + 9);
> -	sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
> +	if (parse_context->pNewTable == NULL)
> +		sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
>   	save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
>   		    vdbe->nOp - 1);
>   	sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);
> 

I added a test to this, and noticed, that 'drop table' returns
rowcount 2 when it has a foreign key. Please, fix. Now my test
fails on it.

==============================================================

commit bfdc94f2ab1cf29f7ae1948a3fa002855a6cf430
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Wed Nov 14 15:10:02 2018 +0300

     Review fixes [tests fail]

diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index f390a73a9..e75f79110 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -727,9 +727,6 @@ cn:execute('insert into TEST3 values (null), (null), (null), (null)')
    - -29
    rowcount: 4
  ...
-cn:close()
----
-...
  box.sql.execute('drop table test')
  ---
  ...
@@ -742,6 +739,28 @@ sq:drop()
  box.sql.execute('drop table test3')
  ---
  ...
+--
+-- Ensure that FK inside CREATE TABLE does not affect rowcount.
+--
+cn:execute('create table test (id integer primary key)')
+---
+- rowcount: 1
+...
+cn:execute('create table test2 (id integer primary key, ref integer references test)')
+---
+- rowcount: 1
+...
+cn:execute('drop table test2')
+---
+- rowcount: 1
+...
+cn:execute('drop table test')
+---
+- rowcount: 1
+...
+cn:close()
+---
+...
  box.schema.user.revoke('guest', 'read,write,execute', 'universe')
  ---
  ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index a18755b00..fb5685a9e 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -225,12 +225,21 @@ box.sql.execute('create table test3 (id int primary key autoincrement)')
  box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
  cn:execute('insert into TEST3 values (null), (null), (null), (null)')
  
-cn:close()
  box.sql.execute('drop table test')
  s:drop()
  sq:drop()
  box.sql.execute('drop table test3')
  
+--
+-- Ensure that FK inside CREATE TABLE does not affect rowcount.
+--
+cn:execute('create table test (id integer primary key)')
+cn:execute('create table test2 (id integer primary key, ref integer references test)')
+cn:execute('drop table test2')
+cn:execute('drop table test')
+
+cn:close()
+
  box.schema.user.revoke('guest', 'read,write,execute', 'universe')
  box.schema.user.revoke('guest', 'create', 'space')
  space = nil

[tarantool-patches] Re: [PATCH v2 4/4] sql: rename changes() to row_count()

[Vladislav Shpilevoy]

Thanks for the patch!

> diff --git a/test/sql/row-count.result b/test/sql/row-count.result
> new file mode 100644
> index 000000000..7577d2795
> --- /dev/null
> +++ b/test/sql/row-count.result
> @@ -0,0 +1,157 @@
> +test_run = require('test_run').new()
> +---
> +...
> +engine = test_run:get_cfg('engine')
> +---
> +...
> +box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
> +---
> +...
> +-- Test cases concerning row count calculations.
> +--
> +box.sql.execute("CREATE TABLE t1 (s1 CHAR(10) PRIMARY KEY);")
> +---
> +...
> +box.sql.execute("SELECT ROW_COUNT();")
> +---
> +- - [1]
> +...
> +box.sql.execute("SELECT ROW_COUNT();")
> +---
> +- - [0]

1. As I understand, ROW_COUNT() should return > 0 only in a
not empty transaction. Here you got rowcount > 0 after DDL
transaction is committed. Also, twice in a row called
row_count() returning different values looks weird.

'rowcount' should be returned as a metavalue from a DDL/DML
when box.sql.execute is removed.

> diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua
> new file mode 100644
> index 000000000..38d3520c2
> --- /dev/null
> +++ b/test/sql/row-count.test.lua
> @@ -0,0 +1,54 @@
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')
> +box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
> +
> +-- Test cases concerning row count calculations.
> +--
> +box.sql.execute("CREATE TABLE t1 (s1 CHAR(10) PRIMARY KEY);")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("CREATE TABLE t2 (s1 CHAR(10) PRIMARY KEY, s2 CHAR(10) REFERENCES t1 ON DELETE CASCADE);")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("CREATE TABLE t3 (i1 INT PRIMARY KEY, i2 INT);")
> +box.sql.execute("INSERT INTO t3 VALUES (0, 0);")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("CREATE TRIGGER x AFTER DELETE ON t1 FOR EACH ROW BEGIN UPDATE t3 SET i1 = i1 + ROW_COUNT(); END;")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("INSERT INTO t1 VALUES ('a');")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("INSERT INTO t2 VALUES ('a','a');")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("INSERT INTO t1 VALUES ('b'), ('c'), ('d');")
> +box.sql.execute("SELECT ROW_COUNT();")
> +-- REPLACE is accounted for two operations: DELETE + INSERT.
> +box.sql.execute("REPLACE INTO t2 VALUES('a', 'c');")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("DELETE FROM t1;")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("INSERT INTO t3 VALUES (1, 1), (2, 2), (3, 3);")
> +box.sql.execute("TRUNCATE TABLE t3;")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("INSERT INTO t3 VALUES (1, 1), (2, 2), (3, 3);")
> +box.sql.execute("UPDATE t3 SET i2 = 666;")
> +box.sql.execute("SELECT ROW_COUNT();")
> +
> +-- All statements which are not accounted as DML should
> +-- return 0 (zero) as a row count.
> +--
> +box.sql.execute("START TRANSACTION;")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("COMMIT;")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("COMMIT;")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("ANALYZE;")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute("EXPLAIN QUERY PLAN INSERT INTO t1 VALUES ('b'), ('c'), ('d');")
> +box.sql.execute("SELECT ROW_COUNT();")
> +box.sql.execute('PRAGMA recursive_triggers')
> +
> +-- Clean-up.
> +--
> +box.sql.execute("DROP TABLE t2;")
> +box.sql.execute("DROP TABLE t3;")
> +box.sql.execute("DROP TABLE t1;")
> \ No newline at end of file

2. "No newline at end of file."

[tarantool-patches] Re: [PATCH v2 1/4] sql: don't increment row count on FK creation within CREATE TABLE

[n.pettik]

> Hi! Thanks for the patch!
> 
> See 1 comment below, my fixes at the end of the email
> and on the branch.
> 
> On 13/11/2018 19:11, Nikita Pettik wrote:
>> We have agreement that each successful DDL operation returns 1 (one) as
>> a row count (via IProto protocol or changes() SQL function), despite
>> the number of other created objects (e.g. indexes, sequences, FK
>> constraints etc).
>> Needed for #2181
>> ---
>>  src/box/sql/build.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 5b3348bd2..e28856e26 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -1415,7 +1415,8 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
>>  			  constr_tuple_reg + 9);
>>  	sqlite3VdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0,
>>  			  constr_tuple_reg + 9);
>> -	sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
>> +	if (parse_context->pNewTable == NULL)
>> +		sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
>>  	save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
>>  		    vdbe->nOp - 1);
>>  	sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);
> 
> I added a test to this, and noticed, that 'drop table' returns
> rowcount 2 when it has a foreign key. Please, fix. Now my test
> fails on it.

Thanks for provided test. Fixed:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index e28856e26..9c9a6d9e1 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1810,7 +1810,6 @@ vdbe_emit_fkey_drop(struct Parse *parse_context, char *constraint_name,
        }
        sqlite3VdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2);
        sqlite3VdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + 2);
-       sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
        VdbeComment((vdbe, "Delete FK constraint %s", constraint_name));
        sqlite3ReleaseTempRange(parse_context, key_reg, 3);
 }
@@ -2269,6 +2268,13 @@ sql_drop_foreign_key(struct Parse *parse_context, struct SrcList *table,
        if (constraint_name != NULL)
                vdbe_emit_fkey_drop(parse_context, constraint_name,
                                    child->def->id);
+       /*
+        * We account changes to row count only if drop of
+        * foreign keys take place in a separate
+        * ALTER TABLE DROP CONSTRAINT statement, since whole
+        * DROP TABLE always returns 1 (one) as a row count.
+        */
+       sqlite3VdbeChangeP5(sqlite3GetVdbe(parse_context), OPFLAG_NCHANGE);

[tarantool-patches] Re: [PATCH v2 4/4] sql: rename changes() to row_count()

[n.pettik]

>> diff --git a/test/sql/row-count.result b/test/sql/row-count.result
>> new file mode 100644
>> index 000000000..7577d2795
>> --- /dev/null
>> +++ b/test/sql/row-count.result
>> @@ -0,0 +1,157 @@
>> +test_run = require('test_run').new()
>> +---
>> +...
>> +engine = test_run:get_cfg('engine')
>> +---
>> +...
>> +box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
>> +---
>> +...
>> +-- Test cases concerning row count calculations.
>> +--
>> +box.sql.execute("CREATE TABLE t1 (s1 CHAR(10) PRIMARY KEY);")
>> +---
>> +...
>> +box.sql.execute("SELECT ROW_COUNT();")
>> +---
>> +- - [1]
>> +...
>> +box.sql.execute("SELECT ROW_COUNT();")
>> +---
>> +- - [0]
> 
> 1. As I understand, ROW_COUNT() should return > 0 only in a
> not empty transaction. Here you got rowcount > 0 after DDL
> transaction is committed. Also, twice in a row called
> row_count() returning different values looks weird.

From first sight - yes, but it makes sense:
first call returns number of row count of preceding statement,
which is 1 since table was created.
second call again return number of row count of preceding statement,
but now it is 0 since last statement was SELECT and it always
return 0.

> 
> 'rowcount' should be returned as a metavalue from a DDL/DML
> when box.sql.execute is removed.

I added this test on purpose so you can notice this behaviour.
I’ve checked MariaDB and MySQL as Peter suggested and
it works in this way (but row count is -1, not 0):

https://mariadb.com/kb/en/library/information-functions-row_count/

	• For statements which return a result set (such as SELECT, SHOW, DESC or HELP),
          returns -1, even when the result set is empty. This is also true for administrativ
          statements, such as OPTIMIZE.

And ROW_COUNT() returns the number of updated row not only
within active transaction. Again from docs:

"ROW_COUNT() returns the number of rows updated, inserted or deleted by the preceding statement. “

Only “preceding statement” is mentioned.

The same behaviour we can observe in MS Server:

https://docs.microsoft.com/ru-ru/sql/t-sql/functions/rowcount-transact-sql?view=sql-server-2017

“Every insert/update/select/set/delete statement resets the @@rowcount
 to the rows affected by the executed statement.”

And the same it works in DB2:

https://www.ibm.com/support/knowledgecenter/en/SSEPEK_11.0.0/odbc/src/tpc/db2z_fnrowcount.html

If SQLRowCount() is executed after the SQLExecDirect() or SQLExecute()
of an SQL statement other than INSERT, UPDATE, DELETE, or MERGE,
it results in return code 0 and pcrow is set to -1.

So I guess current implementation now is quite close to others.

>> diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua
>> new file mode 100644
>> index 000000000..38d3520c2
>> --- /dev/null
>> +++ b/test/sql/row-count.test.lua
>> +-- Clean-up.
>> +--
>> +box.sql.execute("DROP TABLE t2;")
>> +box.sql.execute("DROP TABLE t3;")
>> +box.sql.execute("DROP TABLE t1;")
>> \ No newline at end of file
> 
> 2. "No newline at end of file.”

Fixed.

[tarantool-patches] Re: [PATCH v2 2/4] sql: account REPLACE as two row changes

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]


> Thanks for the patch!
> 
> On 13/11/2018 19:11, Nikita Pettik wrote:
>> In our SQL implementation REPLACE acts as DELETE + INSERT, so we should
>> account it as two row changes.
>> Needed for #2181
>> ---
>>  src/box/sql/insert.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index fd05c0254..a53568810 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> @@ -1076,7 +1076,7 @@ process_index:  ;
>>  				sql_triggers_exist(tab, TK_DELETE, NULL, NULL);
>>  			sql_generate_row_delete(parse_context, tab, trigger,
>>  						cursor, idx_key_reg, part_count,
>> -						false,
>> +						true,
>>  						ON_CONFLICT_ACTION_REPLACE,
>>  						ONEPASS_SINGLE, -1);
>>  			sqlite3VdbeResolveLabel(v, skip_index);
> 
> I added a test on the branch and here:

Applied as obvious. Thx.


[-- Attachment #2: Type: text/html, Size: 6781 bytes --]

[tarantool-patches] Re: [PATCH v2 4/4] sql: rename changes() to row_count()

[Vladislav Shpilevoy]

>> 1. As I understand, ROW_COUNT() should return > 0 only in a
>> not empty transaction. Here you got rowcount > 0 after DDL
>> transaction is committed. Also, twice in a row called
>> row_count() returning different values looks weird.
> 
>  From first sight - yes, but it makes sense:
> first call returns number of row count of preceding statement,
> which is 1 since table was created.
> second call again return number of row count of preceding statement,
> but now it is 0 since last statement was SELECT and it always
> return 0.
> 
>>
>> 'rowcount' should be returned as a metavalue from a DDL/DML
>> when box.sql.execute is removed.
> 
> I added this test on purpose so you can notice this behaviour.
> I’ve checked MariaDB and MySQL as Peter suggested and
> it works in this way (but row count is -1, not 0):
> 
> https://mariadb.com/kb/en/library/information-functions-row_count/
> 
> 	• For statements which return a result set (such as SELECT, SHOW, DESC or HELP),
>            returns -1, even when the result set is empty. This is also true for administrativ
>            statements, such as OPTIMIZE.
> 
> And ROW_COUNT() returns the number of updated row not only
> within active transaction. Again from docs:
> 
> "ROW_COUNT() returns the number of rows updated, inserted or deleted by the preceding statement. “
> 
> Only “preceding statement” is mentioned.
> 
> The same behaviour we can observe in MS Server:
> 
> https://docs.microsoft.com/ru-ru/sql/t-sql/functions/rowcount-transact-sql?view=sql-server-2017
> 
> “Every insert/update/select/set/delete statement resets the @@rowcount
>   to the rows affected by the executed statement.”
> 
> And the same it works in DB2:
> 
> https://www.ibm.com/support/knowledgecenter/en/SSEPEK_11.0.0/odbc/src/tpc/db2z_fnrowcount.html
> 
> If SQLRowCount() is executed after the SQLExecDirect() or SQLExecute()
> of an SQL statement other than INSERT, UPDATE, DELETE, or MERGE,
> it results in return code 0 and pcrow is set to -1.
> 
> So I guess current implementation now is quite close to others.

Thanks for the investigation! Then ok, decent.

The patchset LGTM.

> 
>>> diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua
>>> new file mode 100644
>>> index 000000000..38d3520c2
>>> --- /dev/null
>>> +++ b/test/sql/row-count.test.lua
>>> +-- Clean-up.
>>> +--
>>> +box.sql.execute("DROP TABLE t2;")
>>> +box.sql.execute("DROP TABLE t3;")
>>> +box.sql.execute("DROP TABLE t1;")
>>> \ No newline at end of file
>>
>> 2. "No newline at end of file.”
> 
> Fixed.
> 

[tarantool-patches] Re: [PATCH v2 0/4] Introduce row_count() function

[Kirill Yukhin]

Hello,
On 13 Nov 19:11, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/commits/np/gh-2181-rework-change-counter-v2
> Issue: https://github.com/tarantool/tarantool/issues/2181
> 
> This patch-set slightly fixes behaviour of changes() function (in
> order to be closer to ROW_COUNT analogue from ANSI SQL), renames
> it row_count() and removes total_changes() function.
> 
> All these changes take place according to comments in @dev
> mailing list.
I've checked the patch set into 2.1 branch.

--
Regards, Kirill Yukhin