[tarantool-patches] [PATCH] sql: remove unnecessary MakeRecord opcodes

[tarantool-patches] [PATCH] sql: remove unnecessary MakeRecord opcodes

[Bulat Niatshin]

OP_MakeRecord creates a record from a given range of registers
and stores that record in a specified register. But now even
secondary indexes with non-default ON CONFLICT clause doesn't
need it, it is necessary only for primary key indexes. However
that unnecesary opcodes appear in INSERT listings. This patch
contains a fix for that.

Closes #3317
---
 src/box/sql/insert.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 4a57b23f5..701fae412 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1309,18 +1309,10 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 					sqlite3VdbeResolveLabel(v, skip_if_null);
 				}
 			}
-			if (IsPrimaryKeyIndex(pIdx) || uniqueByteCodeNeeded) {
-				sqlite3VdbeAddOp3(v, OP_MakeRecord, regNewData + 1,
-						  pTab->nCol, aRegIdx[ix]);
-				VdbeComment((v, "for %s", pIdx->zName));
-			}
-		} else {
-			/* kyukhin: for Tarantool, this should be evaluated to NOP.  */
-			if (IsPrimaryKeyIndex(pIdx) || uniqueByteCodeNeeded) {
-				sqlite3VdbeAddOp3(v, OP_MakeRecord, regIdx,
-						  nIdxCol, aRegIdx[ix]);
-				VdbeComment((v, "for %s", pIdx->zName));
-			}
+
+			sqlite3VdbeAddOp3(v, OP_MakeRecord, regNewData + 1,
+					pTab->nCol, aRegIdx[ix]);
+			VdbeComment((v, "for %s", pIdx->zName));
 		}
 
 		/* In an UPDATE operation, if this index is the PRIMARY KEY
-- 
2.14.1

[tarantool-patches] Re: [tarantool-patches] [PATCH] sql: remove unnecessary MakeRecord opcodes

[Bulat Niatshin]

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

I am sorry, I forgot to specify branch name and ticket.

Branch:
https://github.com/tarantool/tarantool/tree/bn/gh-3317-remove-makerecord  
Issue:
https://github.com/tarantool/tarantool/issues/3317


>Среда,  4 апреля 2018, 15:38 +03:00 от Bulat Niatshin <niatshin@tarantool.org>:
>
>OP_MakeRecord creates a record from a given range of registers
>and stores that record in a specified register. But now even
>secondary indexes with non-default ON CONFLICT clause doesn't
>need it, it is necessary only for primary key indexes. However
>that unnecesary opcodes appear in INSERT listings. This patch
>contains a fix for that.
>
>Closes #3317
>---
> src/box/sql/insert.c | 16 ++++------------
> 1 file changed, 4 insertions(+), 12 deletions(-)
>
>diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>index 4a57b23f5..701fae412 100644
>--- a/src/box/sql/insert.c
>+++ b/src/box/sql/insert.c
>@@ -1309,18 +1309,10 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
> 					sqlite3VdbeResolveLabel(v, skip_if_null);
> 				}
> 			}
>-			if (IsPrimaryKeyIndex(pIdx) || uniqueByteCodeNeeded) {
>-				sqlite3VdbeAddOp3(v, OP_MakeRecord, regNewData + 1,
>-						  pTab->nCol, aRegIdx[ix]);
>-				VdbeComment((v, "for %s", pIdx->zName));
>-			}
>-		} else {
>-			/* kyukhin: for Tarantool, this should be evaluated to NOP.  */
>-			if (IsPrimaryKeyIndex(pIdx) || uniqueByteCodeNeeded) {
>-				sqlite3VdbeAddOp3(v, OP_MakeRecord, regIdx,
>-						  nIdxCol, aRegIdx[ix]);
>-				VdbeComment((v, "for %s", pIdx->zName));
>-			}
>+
>+			sqlite3VdbeAddOp3(v, OP_MakeRecord, regNewData + 1,
>+					pTab->nCol, aRegIdx[ix]);
>+			VdbeComment((v, "for %s", pIdx->zName));
> 		}
> 
> 		/* In an UPDATE operation, if this index is the PRIMARY KEY
>-- 
>2.14.1
>
>




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

[tarantool-patches] Re: [PATCH] sql: remove unnecessary MakeRecord opcodes

[n.pettik]

Hello. Only minor remark concerning commit message.
Rephrase it little bit, please. Lets mention, that you are saying about
bytecode which is generated to process constraint checks.

> On 4 Apr 2018, at 15:38, Bulat Niatshin <niatshin@tarantool.org> wrote:
> 
> OP_MakeRecord creates a record from a given range of registers
> and stores that record in a specified register.

This is obvious and can be dropped.

> But now even
> secondary indexes with non-default ON CONFLICT clause doesn't
> need it,

Good place to explain why.

> it is necessary only for primary key indexes. However
> that unnecesary opcodes appear in INSERT listings. This patch
> contains a fix for that.
> 
> Closes #3317
> ---
> src/box/sql/insert.c | 16 ++++------------
> 1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 4a57b23f5..701fae412 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1309,18 +1309,10 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
> 					sqlite3VdbeResolveLabel(v, skip_if_null);
> 				}
> 			}
> -			if (IsPrimaryKeyIndex(pIdx) || uniqueByteCodeNeeded) {
> -				sqlite3VdbeAddOp3(v, OP_MakeRecord, regNewData + 1,
> -						  pTab->nCol, aRegIdx[ix]);
> -				VdbeComment((v, "for %s", pIdx->zName));
> -			}
> -		} else {
> -			/* kyukhin: for Tarantool, this should be evaluated to NOP.  */
> -			if (IsPrimaryKeyIndex(pIdx) || uniqueByteCodeNeeded) {
> -				sqlite3VdbeAddOp3(v, OP_MakeRecord, regIdx,
> -						  nIdxCol, aRegIdx[ix]);
> -				VdbeComment((v, "for %s", pIdx->zName));
> -			}
> +
> +			sqlite3VdbeAddOp3(v, OP_MakeRecord, regNewData + 1,
> +					pTab->nCol, aRegIdx[ix]);
> +			VdbeComment((v, "for %s", pIdx->zName));
> 		}
> 
> 		/* In an UPDATE operation, if this index is the PRIMARY KEY
> -- 
> 2.14.1
> 

[tarantool-patches] Re: [tarantool-patches] Re: [PATCH] sql: remove unnecessary MakeRecord opcodes

[Bulat Niatshin]

Branch:
https://github.com/tarantool/tarantool/tree/bn/gh-3317-remove-makerecord

Issue:
https://github.com/tarantool/tarantool/issues/3317

>Среда, 11 апреля 2018, 22:32 +03:00 от n.pettik <korablev@tarantool.org>:
>
>Hello. Only minor remark concerning commit message.
>Rephrase it little bit, please. Lets mention, that you are saying about
>bytecode which is generated to process constraint checks.
>
>> On 4 Apr 2018, at 15:38, Bulat Niatshin < niatshin@tarantool.org > wrote:
>> 
>> OP_MakeRecord creates a record from a given range of registers
>> and stores that record in a specified register.
>
>This is obvious and can be dropped.

Done, see new commit message below.

>
>> But now even
>> secondary indexes with non-default ON CONFLICT clause doesn't
>> need it,
>
>Good place to explain why.

Done, new commit message with a clearer explanation:

OP_MakeRecord in INSERT/UPDATE query is required for making
insertion into Tarantool and for checking constraints with
ON CONFLICT clause. There was one OP_MakeRecord for each
constraint with ON CONFLICT, and each constraint was checked by
VDBE. But now some constraints with error actions like ABORT can
be handled by Tarantool facilities (without emitting bytecode) and
therefore MakeRecord for that index would be unnecessary if it is
not a primary key index (in that case MakeRecord is also needed
for OP_IdxInsert). This patch contains a fix, which removes
unnecessary MakeRecord opcodes from INSERT/UPDATE VDBE listings.

[tarantool-patches] Re: [PATCH] sql: remove unnecessary MakeRecord opcodes

[n.pettik]

I am OK with this patch.

[tarantool-patches] Re: [PATCH] sql: remove unnecessary MakeRecord opcodes

[Kirill Yukhin]

Hello Bulat, Nikita,
On 22 апр 14:20, n.pettik wrote:
> I am OK with this patch.

I've committed your patch to 2.1 branch.

--
Regards, Kirill Yukhin