From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 E9E4546970E for ; Thu, 30 Jan 2020 01:48:22 +0300 (MSK) Date: Thu, 30 Jan 2020 01:48:21 +0300 From: Nikita Pettik Message-ID: <20200129224821.GD1049@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix INSTEAD OF DELETE trigger for VIEW List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org On 22 Jan 16:54, imeevma@tarantool.org wrote: > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 8d9ae4f..e9edbcc 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -231,7 +231,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > int eph_cursor = parse->nTab++; > int addr_eph_open = sqlVdbeCurrentAddr(v); > if (is_view) { > - pk_len = space->def->field_count; > + pk_len = space->def->field_count + 1; > parse->nMem += pk_len; > sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph, Please, add comments explaining what's going on here. Despite the seeming simplicity, this change is way far from being obvious. This time I've added comment myself: + /* + * At this stage SELECT is already materialized + * into ephemeral table, which has one additional + * tail field (except for ones specified in view + * format) which contains sequential ids. These ids + * are required since selected values may turn out to + * be non-unique. For instance: + * CREATE TABLE t (id INT PRIMARY KEY, a INT); + * INSERT INTO t VALUES (1, 1), (2, 1), (3, 1); + * CREATE VIEW v AS SELECT a FROM t; + * Meanwhile ephemeral tables feature PK covering ALL + * fields of format, so without id materialization + * processing is impossible. Then, results of + * materialization are transferred to the table being + * created below. So to fit tuples in it we must + * account that id field as well. That's why pk_len + * has one field more than view format. + */ Next time I'll reject such patches. Pushed to master, 2.3 and 2.2.