[tarantool-patches] Re: [PATCH] sql: increment rowcount of FK alteration

[Vladislav Shpilevoy]

LGTM.

On 10/04/2019 16:27, Nikita Pettik wrote:
> Before this patch SQL statement which involves FK constraints creation
> or drop didn't increment rowcount:
> 
> box.execute("ALTER TABLE t ADD CONSTRAINT fk1 FOREIGN KEY (b) REFERENCES parent (a);")
> ---
> - rowcount: 0
> ...
> 
> This patch fixes this misbehaviour: accidentally VDBE was forgotten to
> enable counting changes during ALTER TABLE ADD/DROP constraint.
> 
> Closes #4130
> ---
> Branch: https://github.com/tarantool/tarantool/issues/4130
> Issue: https://github.com/tarantool/tarantool/tree/np/gh-4130-increment-row-count-on-fk-alteration
> 
>  src/box/sql/build.c            |  8 ++++++--
>  test/sql/errinj.result         |  2 +-
>  test/sql/foreign-keys.result   | 24 +++++++++++++++++++++++-
>  test/sql/foreign-keys.test.lua |  5 +++++
>  4 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a06ba3e03..b1ed64a01 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1067,8 +1067,10 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
>  			  constr_tuple_reg + 9);
>  	sqlVdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0,
>  			  constr_tuple_reg + 9);
> -	if (parse_context->create_table_def.new_space == NULL)
> +	if (parse_context->create_table_def.new_space == NULL) {
> +		sqlVdbeCountChanges(vdbe);
>  		sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE);
> +	}
>  	save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
>  		    vdbe->nOp - 1);
>  	sqlReleaseTempRange(parse_context, constr_tuple_reg, 10);
> @@ -1902,7 +1904,9 @@ sql_drop_foreign_key(struct Parse *parse_context)
>  	 * ALTER TABLE DROP CONSTRAINT statement, since whole
>  	 * DROP TABLE always returns 1 (one) as a row count.
>  	 */
> -	sqlVdbeChangeP5(sqlGetVdbe(parse_context), OPFLAG_NCHANGE);
> +	struct Vdbe *v = sqlGetVdbe(parse_context);
> +	sqlVdbeCountChanges(v);
> +	sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
>  }
>  
>  /**
> diff --git a/test/sql/errinj.result b/test/sql/errinj.result
> index 6836d4afb..ee36a387b 100644
> --- a/test/sql/errinj.result
> +++ b/test/sql/errinj.result
> @@ -362,7 +362,7 @@ box.snapshot()
>  ...
>  box.execute("ALTER TABLE t3 ADD CONSTRAINT fk1 FOREIGN KEY (b) REFERENCES t3;")
>  ---
> -- row_count: 0
> +- row_count: 1
>  ...
>  box.execute("INSERT INTO t3 VALUES(1, 1, 3);")
>  ---
> diff --git a/test/sql/foreign-keys.result b/test/sql/foreign-keys.result
> index 2331c59f8..66eb94b77 100644
> --- a/test/sql/foreign-keys.result
> +++ b/test/sql/foreign-keys.result
> @@ -350,7 +350,17 @@ box.execute('CREATE TABLE tc (id INT PRIMARY KEY, a INT REFERENCES tp(a) MATCH F
>  ...
>  box.execute('ALTER TABLE tc ADD CONSTRAINT fk1 FOREIGN KEY (id) REFERENCES tp(id) MATCH PARTIAL ON DELETE CASCADE ON UPDATE SET NULL')
>  ---
> -- row_count: 0
> +- row_count: 1
> +...
> +-- Test that ADD/DROP CONSTRAINT return correct row_count value.
> +--
> +box.execute('SELECT row_count();')
> +---
> +- metadata:
> +  - name: row_count()
> +    type: integer
> +  rows:
> +  - [1]
>  ...
>  box.space._fk_constraint:select{}
>  ---
> @@ -358,6 +368,18 @@ box.space._fk_constraint:select{}
>    - ['FK_CONSTRAINT_1_TC', 518, 517, false, 'full', 'set_null', 'no_action', [1],
>      [1]]
>  ...
> +box.execute('ALTER TABLE tc DROP CONSTRAINT fk1;')
> +---
> +- row_count: 1
> +...
> +box.execute('SELECT row_count();')
> +---
> +- metadata:
> +  - name: row_count()
> +    type: integer
> +  rows:
> +  - [1]
> +...
>  box.execute('DROP TABLE tc')
>  ---
>  - row_count: 1
> diff --git a/test/sql/foreign-keys.test.lua b/test/sql/foreign-keys.test.lua
> index b5d1f589d..4ac5999d7 100644
> --- a/test/sql/foreign-keys.test.lua
> +++ b/test/sql/foreign-keys.test.lua
> @@ -156,7 +156,12 @@ box.space.PARENT:drop()
>  box.execute('CREATE TABLE tp (id INT PRIMARY KEY, a INT UNIQUE)')
>  box.execute('CREATE TABLE tc (id INT PRIMARY KEY, a INT REFERENCES tp(a) MATCH FULL ON DELETE SET NULL)')
>  box.execute('ALTER TABLE tc ADD CONSTRAINT fk1 FOREIGN KEY (id) REFERENCES tp(id) MATCH PARTIAL ON DELETE CASCADE ON UPDATE SET NULL')
> +-- Test that ADD/DROP CONSTRAINT return correct row_count value.
> +--
> +box.execute('SELECT row_count();')
>  box.space._fk_constraint:select{}
> +box.execute('ALTER TABLE tc DROP CONSTRAINT fk1;')
> +box.execute('SELECT row_count();')
>  box.execute('DROP TABLE tc')
>  box.execute('DROP TABLE tp')
>  
> -- 
> 2.15.1
>