[tarantool-patches] Re: [PATCH 2/2] sql: transactional DDL

[n.pettik]

Previous patch seems to be OK to me.

> Box recently added support of transactional DDL allowing to do
> any number of non-yielding DDL operations atomically.

So, in SQL the only yielding operation (except for registered by
user custom function) is CREATE INDEX involving non-empty space.
Is this statement correct? If so, I guess we should add a few examples
demonstrating new functionality to our SQL manual.

> This is
> really a big relief of one of the biggest pains of SQL. Before
> this patch each multirow SQL DDL statement needed to prepare its
> own rollback procedure for a case if something would go wrong.
> 
> Now with box support SQL wraps each DDL statement into a
> transaction, and doesn't need own escape-routes in a form of
> 'struct save_record' and others.
> 
> Closes #4086
> ---
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 2aefa2a3f..4884a7855 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> 
> @@ -3331,10 +3215,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
> 	} else {
> 		sqlVdbeAddOp4(v, OP_SetDiag, tarantool_error_code, 0, 0, error,
> 			      P4_DYNAMIC);
> -		if (is_clean_needed)
> -			save_record(parser, 0, 0, 0, v->nOp - 1, false);
> -		else
> -			sqlVdbeAddOp1(v, OP_Halt, -1);
> +		sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);

ABORT is set by default, isn’t it? See similar questions below.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 6a4a303b9..a71b331f8 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4373,15 +4373,16 @@ case OP_Update: {
>  */
> case OP_SInsert: {
> 	assert(pOp->p1 > 0);
> -	assert(pOp->p2 > 0);
> -	assert(pOp->p3 >= 0);
> +	assert(pOp->p2 >= 0);
> 
> -	pIn3 = &aMem[pOp->p3];
> +	pIn2 = &aMem[pOp->p2];
> 	struct space *space = space_by_id(pOp->p1);
> 	assert(space != NULL);
> 	assert(space_is_system(space));
> -	if (tarantoolsqlInsert(space, pIn3->z, pIn3->z + pIn3->n) != 0)
> -		goto jump_to_p2;
> +	if (tarantoolsqlInsert(space, pIn2->z, pIn2->z + pIn2->n) != 0) {
> +		p->errorAction = ON_CONFLICT_ACTION_ABORT;
> +		goto abort_due_to_error;

Why do we need set errorAction here? As I see, it is set
to _ABORT by default:

sqlVdbeRewind() -> sqlVdbeMakeReady() -> sql_finish_coding()


> @@ -4404,8 +4405,10 @@ case OP_SDelete: {
> 	struct space *space = space_by_id(pOp->p1);
> 	assert(space != NULL);
> 	assert(space_is_system(space));
> -	if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0)
> +	if (sql_delete_by_key(space, 0, pIn2->z, pIn2->n) != 0) {
> +		p->errorAction = ON_CONFLICT_ACTION_ABORT;

Same question concerning errorAction.

> 		goto abort_due_to_error;
> +	}
> 	if (pOp->p5 & OPFLAG_NCHANGE)
> 		p->nChange++;
> 	break;
> diff --git a/test/sql/ddl.test.lua b/test/sql/ddl.test.lua
> new file mode 100644
> index 000000000..477158796
> --- /dev/null
> +++ b/test/sql/ddl.test.lua
> @@ -0,0 +1,187 @@
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')
> +box.execute('pragma sql_default_engine=\''..engine..'\'')
> +
> +--
> +-- gh-4086: SQL transactional DDL.
> +--
> +test_run:cmd("setopt delimiter ';'")
> +box.begin()
> +box.execute('CREATE TABLE t1(id INTEGER PRIMARY KEY);')
> +box.execute('CREATE TABLE t2(id INTEGER PRIMARY KEY);')
> +box.commit();
> +test_run:cmd("setopt delimiter ''");
> +
> +box.space.T1 ~= nil
> +box.space.T1.index[0] ~= nil
> +box.space.T2 ~= nil
> +box.space.T2.index[0] ~= nil
> +
> +test_run:cmd("setopt delimiter ';'")
> +box.begin()
> +box.execute('DROP TABLE t1;')
> +assert(box.space.T1 == nil)
> +assert(box.space.T2 ~= nil)
> +box.execute('DROP TABLE t2;')
> +assert(box.space.T2 == nil)
> +box.commit();
> +test_run:cmd("setopt delimiter ''");
> +
> +--
> +-- Use all the possible SQL DDL statements.
> +--
> +test_run:cmd("setopt delimiter '$'")
> +function monster_ddl()

Still you have missed ALTER RENAME statement :)
Also, AFAIR TRUNCATE is also considered to be DDL
operation. Anyway, great tests, I like them. Personally,
I’d write them in Lua format as well. IMHO *.sql.test would
look a bit weird taking into account number of involved Lua
functions.

> +    local _, err1, err2, err3
> +    box.execute([[CREATE TABLE t1(id INTEGER PRIMARY KEY,
> +                                  a INTEGER,
> +                                  b INTEGER);]])
> +    box.execute([[CREATE TABLE t2(id INTEGER PRIMARY KEY,
> +                                  a INTEGER,
> +                                  b INTEGER UNIQUE,
> +                                  CONSTRAINT ck1 CHECK(b < 100));]])
> +
> +    box.execute('CREATE INDEX t1a ON t1(a);')
> +    box.execute('CREATE INDEX t2a ON t2(a);')
> +    box.execute('DROP INDEX t2a ON t2;')
> +
> +    box.execute('ALTER TABLE t1 ADD CONSTRAINT ck1 CHECK(b > 0);')
> +    box.execute('ALTER TABLE t1 ADD CONSTRAINT ck2 CHECK(a > 0);')
> +    box.space.T1.ck_constraint.CK1:drop()
> +
> +    box.execute([[ALTER TABLE t1 ADD CONSTRAINT fk1 FOREIGN KEY
> +                  (a) REFERENCES t2(b);]])
> +    box.execute('ALTER TABLE t1 DROP CONSTRAINT fk1;')
> +
> +-- Try random errors inside this big batch of DDL to ensure, that
> +-- they do not affect normal operation.

Nit: a bit broken comment's indentation..?

> diff --git a/test/sql/view_delayed_wal.test.lua b/test/sql/view_delayed_wal.test.lua
> index 8e73b03f3..0a10d121b 100644
> --- a/test/sql/view_delayed_wal.test.lua
> +++ b/test/sql/view_delayed_wal.test.lua
> @@ -5,8 +5,8 @@ fiber = require('fiber')
> 
> -- View reference counters are incremented before firing
> -- on_commit triggers (i.e. before being written into WAL), so
> --- it is impossible to create view on dropped (but not written
> --- into WAL) space.
> +-- it is impossible to drop a space referenced by a created, but
> +-- no committed view.

Nit: I guess ’*still* not yet committed view’ would sound better.