From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id ED13824BAD for ; Thu, 7 Jun 2018 08:15:08 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0QZTwprFbyPp for ; Thu, 7 Jun 2018 08:15:08 -0400 (EDT) Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id CC43A2097D for ; Thu, 7 Jun 2018 08:15:07 -0400 (EDT) Date: Thu, 7 Jun 2018 15:03:24 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH 1/3] sql: fetch primary index for affinity only Message-ID: <20180607120324.h2laztbxfnhnpe2s@tarantool.org> References: <5bc1cc0b-529b-62b7-6ed3-36f657db1cd2@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5bc1cc0b-529b-62b7-6ed3-36f657db1cd2@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org Hello Vlad, My answers are inlined. Updated patch is in the bottom. On 01 июн 21:00, Vladislav Shpilevoy wrote: > Hello. Thanks for the patch! See 2 comments below. > > On 01/06/2018 18:16, Kirill Yukhin wrote: > > This small patch removes usages of primary index throughout > > code sql_table_delete_from, limiting use to fetching of > > affinities only. We cannot use space_def here, since primary > > index might contain calculated columns. > 1. What are calculated columns? A.k.a. computed columns, those which ultimately are expressions. > > Part of #3235 > > --- > > src/box/sql/delete.c | 54 ++++++++++++++++++++++++++++++++++++---------------- > > 1 file changed, 38 insertions(+), 16 deletions(-) > > > > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > > index ddad54b..28713c8 100644 > > --- a/src/box/sql/delete.c > > +++ b/src/box/sql/delete.c > > @@ -183,6 +183,16 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > > struct NameContext nc; > > memset(&nc, 0, sizeof(nc)); > > nc.pParse = parse; > > + if (tab_list->a[0].pTab == NULL) { > > + struct Table *t = malloc(sizeof(struct Table)); > > + if (t == NULL) { > > + sqlite3OomFault(parse->db); > > + goto delete_from_cleanup; > > + } > > + assert(space != NULL); > > + t->def = space->def; > > + tab_list->a[0].pTab = t; > > 2. Why can not you declare struct Table on the stack at the top of this > function, and here use its address? Can this pTab be used out of > sql_table_delete_from()? And I do not see where do you delete this > table. Relaced w/ stack variable. -- Regards, Kirill Yukhin commit 6ef9616b71c6e253336c012442b90c4d2ebb2c55 Author: Kirill Yukhin Date: Wed May 23 15:31:37 2018 +0300 sql: fetch primary index for affinity only This small patch removes usages of primary index throughout code sql_table_delete_from, limiting use to fetching of affinities only. We cannot use space_def here, since primary index might contain calculated columns. Part of #3235 diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index ddad54b..a4a8da6 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -76,6 +76,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, struct Expr *where) { struct sqlite3 *db = parse->db; + struct Table loc_table; if (parse->nErr || db->mallocFailed) goto delete_from_cleanup; @@ -183,6 +184,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, struct NameContext nc; memset(&nc, 0, sizeof(nc)); nc.pParse = parse; + if (tab_list->a[0].pTab == NULL) { + assert(space != NULL); + memset(&loc_table, 0, sizeof(struct Table)); + loc_table.def = space->def; + tab_list->a[0].pTab = &loc_table; + } nc.pSrcList = tab_list; if (sqlite3ResolveExprNames(&nc, where)) goto delete_from_cleanup; @@ -196,7 +203,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, * is held in ephemeral table, there is no PK for * it, so columns should be loaded manually. */ - struct Index *pk = NULL; + struct key_def *pk_def = NULL; int reg_pk = parse->nMem + 1; int pk_len; int eph_cursor = parse->nTab++; @@ -207,13 +214,17 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, eph_cursor, pk_len); } else { - pk = sqlite3PrimaryKeyIndex(table); - assert(pk != NULL); - pk_len = index_column_count(pk); + assert(space->index_count > 0); + pk_def = key_def_dup(space->index[0]->def->key_def); + if(pk_def == NULL) { + sqlite3OomFault(parse->db); + goto delete_from_cleanup; + } + pk_len = pk_def->part_count; parse->nMem += pk_len; - sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, eph_cursor, - pk_len); - sql_vdbe_set_p4_key_def(parse, pk); + sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, eph_cursor, + pk_len, 0, + (char *)pk_def, P4_KEYDEF); } /* Construct a query to find the primary key for @@ -252,11 +263,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, /* Extract the primary key for the current row */ if (!is_view) { for (int i = 0; i < pk_len; i++) { - assert(pk->aiColumn[i] >= 0); sqlite3ExprCodeGetColumnOfTable(v, table->def, tab_cursor, - pk-> - aiColumn[i], + pk_def->parts[i].fieldno, reg_pk + i); } } else { @@ -287,10 +296,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, * key. */ key_len = 0; - const char *zAff = is_view ? NULL : - sqlite3IndexAffinityStr(parse->db, pk); + const char *aff = NULL; + struct Index *pk = sqlite3PrimaryKeyIndex(table); + if (pk != NULL) { + aff = is_view ? NULL : + sqlite3IndexAffinityStr(parse->db, pk); + } sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len, - reg_key, zAff, pk_len); + reg_key, aff, pk_len); /* Set flag to save memory allocating one * by malloc. */ @@ -325,8 +338,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, (void *)space, P4_SPACEPTR); sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor, table->tnum, space_ptr_reg); - sql_vdbe_set_p4_key_def(parse, pk); - VdbeComment((v, "%s", pk->zName)); + struct key_def *def = key_def_dup(pk_def); + if (def == NULL) { + sqlite3OomFault(parse->db); + goto delete_from_cleanup; + } + sqlite3VdbeAppendP4(v, def, P4_KEYDEF); + + VdbeComment((v, "%s", space->index[0]->def->name)); if (one_pass == ONEPASS_MULTI) sqlite3VdbeJumpHere(v, iAddrOnce); @@ -339,7 +358,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, if (one_pass != ONEPASS_OFF) { /* OP_Found will use an unpacked key. */ assert(key_len == pk_len); - assert(pk != NULL || table->pSelect != NULL); + assert(pk_def != NULL || table->pSelect != NULL); sqlite3VdbeAddOp4Int(v, OP_NotFound, tab_cursor, addr_bypass, reg_key, key_len);