[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

["n.pettik" <korablev@tarantool.org>]

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


> commit fe8415a79d401b741dcb565d34eb56495223f8b6
> Author: Mergen Imeev <imeevma@gmail.com <mailto:imeevma@gmail.com>>
> Date:   Fri Aug 31 15:50:17 2018 +0300
> 
>     sql: cleanup on failed creation operation
> 
>     Some creation operations create objects even on fail. This is
>     wrong and should be fixed. This patch adds destructors for such
>     objects.

Kind of poor explanation. Where those destructors are used?
Which objects are you talking about? AFAIU ‘some creation operations’
may refer only to table creation; in other cases only one object created
per SQL statement: CREATE INDEX -> one index, CREATE TRIGGER ->
one trigger, ADD CONSTRAINT -> one constraint.
For this reason, you don’t need to add to list of records indexes and fk
constraints when it comes to separate CREATE INDEX/ADD CONSTRAINT
statements. Extend your explanation and provide examples, please.

> 
>     Closes #3592
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 60b49df..2ac86ab 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> +/**
> + * Save inserted in system space record in list.
> + *
> + * @param parser SQL Parser object.
> + * @param space_id Id of table in which record is inserted.
> + * @param reg_key Register that contains first field of the key.
> + * @param reg_key_count Exact number of fields of the key.
> + * @param insertion_opcode Number of OP_SInsert opcode.
> + */
> +static inline void
> +save_record(struct Parse *parser, uint32_t space_id, int reg_key,
> +        int reg_key_count, int insertion_opcode)
> +{
> +    struct saved_record *record =
> +        region_alloc(&fiber()->gc, sizeof(*record));

Why do you use global region? You can use parser’s one.

>  void
>  sql_finish_coding(struct Parse *parse_context)
> @@ -62,6 +110,29 @@ sql_finish_coding(struct Parse *parse_context)
>      struct sqlite3 *db = parse_context->db;
>      struct Vdbe *v = sqlite3GetVdbe(parse_context);
>      sqlite3VdbeAddOp0(v, OP_Halt);
> +    /*
> +     * Destructors for all created in current statement
> +     * spaces, indexes, sequences etc. There is no need to
> +     * create destructor for last SInsert.
> +     */
> +    if (rlist_empty(&parse_context->record_list) == 0) {

Nit: if(! rlist_empty()) 

Also, lets add comments directly to VDBE program indicating labels
for each clean-up jumps (VdbeComment). It would significantly
simplify debugging of VDBE programs.

> +        struct saved_record *record =
> +            rlist_shift_entry(&parse_context->record_list,
> +                      struct saved_record, link);
> +        /* Set P2 of SInsert. */
> +        sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
> +        rlist_foreach_entry(record, &parse_context->record_list, link) {
> +            int record_reg = ++parse_context->nMem;
> +            sqlite3VdbeAddOp3(v, OP_MakeRecord, record->reg_key,
> +                      record->reg_key_count, record_reg);
> +            sqlite3VdbeAddOp2(v, OP_SDelete, record->space_id,
> +                      record_reg);
> +            /* Set P2 of SInsert. */
> +            sqlite3VdbeChangeP2(v, record->insertion_opcode,
> +                        v->nOp);
> +        }
> +        sqlite3VdbeAddOp2(v, OP_Halt, SQL_TARANTOOL_ERROR, 0);
> +    }
>      /* Code creation of FK constraints, if any. */
>      struct fkey_parse *fk_parse;
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index e98e845..a4b65eb 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -274,6 +274,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
>      memset(parser, 0, sizeof(struct Parse));
>      parser->db = db;
>      rlist_create(&parser->new_fkey);
> +    rlist_create(&parser->record_list);
>      region_create(&parser->region, &cord()->slabc);
>  }
> 
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index f56090d..c5becbd 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2764,6 +2764,11 @@ struct Parse {
>       * Foreign key constraint appeared in CREATE TABLE stmt.
>       */
>      struct rlist new_fkey;
> +    /**
> +     * List of all records that were inserted in system spaces
> +     * in current statement.
> +     */
> +    struct rlist record_list;
>      bool initiateTTrans;    /* Initiate Tarantool transaction */
>      /** True, if table to be created has AUTOINCREMENT PK. */
>      bool is_new_table_autoinc;
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 0efc4dd..fc959bd 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1010,8 +1010,12 @@ case OP_Halt: {
>      p->pc = pcx;
>      if (p->rc) {
>          if (p->rc == SQL_TARANTOOL_ERROR) {
> -            assert(pOp->p4.z != NULL);
> -            box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
> +            if (pOp->p4.z == NULL) {
> +                assert(! diag_is_empty(diag_get()));
> +            } else {
> +                box_error_set(__FILE__, __LINE__, pOp->p5,
> +                          pOp->p4.z);
> +            }
>          } else if (pOp->p5 != 0) {
>              static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
>                                     "FOREIGN KEY" };
> @@ -4318,8 +4322,8 @@ case OP_IdxInsert: {        /* in2 */
>      break;
>  }
> 
> -/* Opcode: SInsert P1 P2 * * P5
> - * Synopsis: space id = P1, key = r[P2]
> +/* Opcode: SInsert P1 P2 P3 * P5
> + * Synopsis: space id = P1, key = r[P3], on error goto P2

It would be better to swap P2 and P3: if you did so, you wouldn’t need to
explicitly set 0 as second arg since all unspecified args are nullified by default.
For instance:

sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, iRecord);

sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord);

On the other hand, I see that many opcodes use exactly p2 operand to
process jump and label jump_to_p2. So, if you don’t want to break this
convention, you may leave it as is.

> diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
> index 9663074..ba47716 100644
> --- a/test/sql/drop-table.test.lua
> +++ b/test/sql/drop-table.test.lua
> @@ -25,3 +25,24 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
> 
>  -- Debug
>  -- require("console").start()
> +
> +--
> +-- gh-3592: segmentation fault when table with error during
> +-- creation is dropped.
> +-- We should grant user enough rights to create space, but not
> +-- enough to create index.
> +--
> +box.schema.user.create('tmp')
> +box.schema.user.grant('tmp', 'create', 'universe')
> +box.schema.user.grant('tmp', 'write', 'space', '_space')
> +box.schema.user.grant('tmp', 'write', 'space', '_schema')
> +box.session.su('tmp')
> +--
> +-- Error: user do not have rights to write in box.space._index.
> +-- Space that was already created should be automatically dropped.
> +--
> +box.sql.execute('create table t1 (id int primary key, a int)')
> +-- Error: no such table.
> +box.sql.execute('drop table t1')
> +box.session.su('admin')
> +box.schema.user.drop('tmp’)

Em, and where are tests for current feature? This test checks only one
particular case. AFAIU #3592 is only result of absence of cleanup on
table creation (original issue should be https://github.com/tarantool/tarantool/issues/3499
but it is marked as ‘doc’. Now your patch changes this behaviour again).
Anyway, add test cases when CREATE TABLE fails on VDBE execution,
e.g. half of unique indexes or referencing constraints are created, but
smth goes wrong and remains of execution are eliminated correctly.


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