From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CCB02469719 for ; Wed, 11 Mar 2020 18:28:00 +0300 (MSK) Date: Wed, 11 Mar 2020 15:27:59 +0000 From: Nikita Pettik Message-ID: <20200311152759.GA29797@tarantool.org> References: <23e20a4665756721778df4e46d8efb3d273fde02.1579247383.git.imeevma@gmail.com> <20200120175725.GE82780@tarantool.org> <20200225101729.GA28823@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200225101729.GA28823@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: use rowid as PK of ephemeral space on INSERT List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org On 25 Feb 13:17, Mergen Imeev wrote: > Hi! Thank you for review. My answers and new patch beliw. > Date: Mon, 24 Feb 2020 16:44:07 +0300 > Subject: [PATCH] sql: do not change order of inserted values on INSERT > > Prior to this patch, if ephemeral space was used during INSERT, Does fix affect only insert, but not replace? Or both? > the inserted values were sorted by the first column. Are inserted values are always sorted during insertion op? > This can lead > to an error when using the autoincrement feature. Why? Could you please provide details? Not personally for me, but in commit message. > To avoid this, > the patch makes it possible to make the rowid of the value > inserted into the ephemeral space be the first part of > ephemeral space index. So, before patch rowid could be only last column of table? And you achieve it extending sql_key_info with additional field? All these tip-questions should help you to make commit message more informative. > Closes #4256 Please attach a changelog to the patch. > > diff --git a/src/box/sql.c b/src/box/sql.c > index 1256df8..47cf235 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -338,7 +338,10 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info) > return NULL; > } > for (uint32_t i = 0; i < field_count; ++i) { > - struct key_part_def *part = &ephemer_key_parts[i]; > + uint32_t j = i; > + if (key_info != NULL && key_info->is_rowid_first) > + j = (j + 1) % field_count; What is going on here?? > + struct key_part_def *part = &ephemer_key_parts[j]; > part->fieldno = i; > part->nullable_action = ON_CONFLICT_ACTION_NONE; > part->is_nullable = true; > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 43a0de5..6ee728a 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -455,8 +455,17 @@ sqlInsert(Parse * pParse, /* Parser context */ > reg_eph = ++pParse->nMem; > regRec = sqlGetTempReg(pParse); > regCopy = sqlGetTempRange(pParse, nColumn + 1); > - sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph, > - nColumn + 1); > + /* > + * This key_info is used to show that > + * rowid should be the first part of PK. > + */ Please, explain here why rowid should come as first column. See reference example in sql_table_delete_from(): 233 if (is_view) { 234 /* 235 * At this stage SELECT is already materialized 236 * into ephemeral table, which has one additional 237 * tail field (except for ones specified in view 238 * format) which contains sequential ids. These ids 239 * are required since selected values may turn out to 240 * be non-unique. For instance: 241 * CREATE TABLE t (id INT PRIMARY KEY, a INT); 242 * INSERT INTO t VALUES (1, 1), (2, 1), (3, 1); 243 * CREATE VIEW v AS SELECT a FROM t; ... > + struct sql_key_info *key_info = > + sql_key_info_new(pParse->db, nColumn + 1); > + key_info->parts[nColumn].type = FIELD_TYPE_UNSIGNED; > + key_info->is_rowid_first = true; > + sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph, > + nColumn + 1, 0, (char *)key_info, > + P4_KEYINFO); > addrL = sqlVdbeAddOp1(v, OP_Yield, dest.iSDParm); > VdbeCoverage(v); > sqlVdbeAddOp2(v, OP_NextIdEphemeral, reg_eph, > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index d1fcf47..d9da921 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -4111,6 +4111,8 @@ struct sql_key_info { > struct key_def *key_def; > /** Reference counter. */ > uint32_t refs; > + /** Rowid should be the first part of PK if true. */ > + bool is_rowid_first; Otherwise it is last or not presented at all? > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 620d74e..db782a7 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -2702,8 +2702,25 @@ case OP_Column: { > * key parts. > */ > if (pC->uc.pCursor->curFlags & BTCF_TEphemCursor) { > - field_type = pC->uc.pCursor->index->def-> > - key_def->parts[p2].type; > + struct key_def *key_def = > + pC->uc.pCursor->index->def->key_def; > + /* > + * There are three options: > + * |Rowid|First field|...|Last field| > + * Or: > + * |First field|...|Last field| > + * Or: > + * |First field|...|Last field|Rowid| > + * > + * If ephemeral space has a rowid column, But it contradicts schema above. > + * it is always the last column. Due to > + * this, the field number of the rowid > + * column cannot be 0. > + */ > + int partno = p2; > + if (key_def->parts[0].fieldno != 0) > + partno = (partno + 1) % key_def->part_count; See this for the second time, still don't understand what it is supposed to mean. > + field_type = key_def->parts[partno].type; > } else if (pC->uc.pCursor->curFlags & BTCF_TaCursor) { > field_type = pC->uc.pCursor->space->def-> > fields[p2].type; > diff --git a/test/sql/autoincrement.test.lua b/test/sql/autoincrement.test.lua > index 63a902a..99f4813 100644 > --- a/test/sql/autoincrement.test.lua > +++ b/test/sql/autoincrement.test.lua > @@ -31,3 +31,13 @@ seqs = box.space._sequence:select{} > #seqs == 0 or seqs > > box.schema.user.drop('user1') > + > +-- > +-- gh-4256: make sure that when inserting, values are inserted in > +-- the given order when ephemeral space is used. > +-- > +box.execute('CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT);') > +box.execute('CREATE TRIGGER r AFTER INSERT ON t FOR EACH ROW BEGIN SELECT 1; END') > +box.execute('INSERT INTO t VALUES (1), (NULL), (10), (NULL), (NULL), (3), (NULL);') Leave explanation why no-op trigger is required. Otherwise it looks redundant. Also, Kirill forces new convention saying that each bug fix should be placed at separate file named gh-xxxx-description. > +box.execute('SELECT * FROM t;') > +box.execute('DROP TABLE t;')