From dffa4576cefb814daabd882e9c3fd1141319130c Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Fri, 8 Feb 2019 15:04:25 +0300
Subject: [PATCH] sql: use 64b bitmasks instead of 32b where possible
In some cases(like foreign keys) the SQL code used
32-bit bit mask, while 64-bit bit masks will perform better
column optimizations. There was refactored code to work with 64b
bitmasks where required.
The 32b bitmasks are still used to specify constant OP_Function
arguments because this change would require changing the P1 type
of the VDBE p1 argument, which is not desirable. Moreover, the
64 function's arguments is an explicit overkill.
The ticket was created in connection with the introduction of
foreign keys and their use of 32-bit bit masks. In the rest of
the scripts in SQL already use 64 bit masks, and the "smart" bit
of the mask is not applicable.
Closes #3571
---
src/box/alter.cc | 10 ++--------
src/box/column_mask.h | 14 +++++++++++++
src/box/sql/delete.c | 7 ++-----
src/box/sql/resolve.c | 19 ++++++------------
src/box/sql/sqlInt.h | 46 ++++++++++++++++++++++++-------------------
src/box/sql/trigger.c | 14 ++++++-------
src/box/sql/update.c | 24 +++++++++-------------
7 files changed, 66 insertions(+), 68 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 415e8a5bc5..3fc31b8d6f 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -29,6 +29,7 @@
* SUCH DAMAGE.
*/
#include "alter.h"
+#include "column_mask.h"
#include "schema.h"
#include "user.h"
#include "space.h"
@@ -3747,13 +3748,6 @@ fk_constraint_remove(struct rlist *child_fk_list, const char *fk_name)
return NULL;
}
-/**
- * FIXME: as sql legacy temporary we use such mask throughout
- * SQL code. It should be replaced later with regular
- * mask from column_mask.h
- */
-#define FKEY_MASK(x) (((x)>31) ? 0xffffffff : ((uint64_t)1<<(x)))
-
/**
* Set bits of @mask which correspond to fields involved in
* given foreign key constraint.
@@ -3767,7 +3761,7 @@ static inline void
fk_constraint_set_mask(const struct fk_constraint *fk, uint64_t *mask, int type)
{
for (uint32_t i = 0; i < fk->def->field_count; ++i)
- *mask |= FKEY_MASK(fk->def->links[i].fields[type]);
+ column_mask_set_fieldno(mask, fk->def->links[i].fields[type]);
}
/**
diff --git a/src/box/column_mask.h b/src/box/column_mask.h
index d71911d465..14da841f1e 100644
--- a/src/box/column_mask.h
+++ b/src/box/column_mask.h
@@ -109,4 +109,18 @@ key_update_can_be_skipped(uint64_t key_mask, uint64_t update_mask)
return (key_mask & update_mask) == 0;
}
+/**
+ * Test a bit in the bitmask corresponding to a column fieldno.
+ * @param column_mask Mask to test.
+ * @param fieldno Fieldno number to test (index base must be 0).
+ * @retval true If bit corresponding to a column fieldno.
+ * @retval false if bit is not set or fieldno > 64.
+ */
+static inline bool
+column_mask_fieldno_is_set(uint64_t column_mask, uint32_t fieldno)
+{
+ uint64_t mask = (uint64_t) 1 << (fieldno < 63 ? fieldno : 63);
+ return (column_mask & mask) != 0;
+}
+
#endif
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index f536bb8230..8d9d0ff32e 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -464,7 +464,7 @@ sql_generate_row_delete(struct Parse *parse, struct space *space,
(fk_constraint_is_required(space, NULL) || trigger_list != NULL)) {
/* Mask of OLD.* columns in use */
/* TODO: Could use temporary registers here. */
- uint32_t mask =
+ uint64_t mask =
sql_trigger_colmask(parse, trigger_list, 0, 0,
TRIGGER_BEFORE | TRIGGER_AFTER,
space, onconf);
@@ -479,10 +479,7 @@ sql_generate_row_delete(struct Parse *parse, struct space *space,
*/
sqlVdbeAddOp2(v, OP_Copy, reg_pk, first_old_reg);
for (int i = 0; i < (int)space->def->field_count; i++) {
- testcase(mask != 0xffffffff && iCol == 31);
- testcase(mask != 0xffffffff && iCol == 32);
- if (mask == 0xffffffff
- || (i <= 31 && (mask & MASKBIT32(i)) != 0)) {
+ if (column_mask_fieldno_is_set(mask, i)) {
sqlExprCodeGetColumnOfTable(v, space->def,
cursor, i,
first_old_reg +
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index dd511518d9..84e2376de2 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -340,20 +340,13 @@ lookupName(Parse * pParse, /* The parsing context */
if (iCol < 0) {
pExpr->type =
FIELD_TYPE_INTEGER;
- } else if (pExpr->iTable == 0) {
- testcase(iCol == 31);
- testcase(iCol == 32);
- pParse->oldmask |=
- (iCol >=
- 32 ? 0xffffffff
- : (((u32) 1) << iCol));
} else {
- testcase(iCol == 31);
- testcase(iCol == 32);
- pParse->newmask |=
- (iCol >=
- 32 ? 0xffffffff
- : (((u32) 1) << iCol));
+ uint64_t *mask =
+ pExpr->iTable == 0 ?
+ &pParse->oldmask :
+ &pParse->newmask;
+ column_mask_set_fieldno(mask,
+ iCol);
}
pExpr->iColumn = (i16) iCol;
pExpr->space_def = space_def;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index b32c3d338f..b8a8e75c08 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -67,6 +67,7 @@
#include <stdbool.h>
+#include "box/column_mask.h"
#include "box/field_def.h"
#include "box/sql.h"
#include "box/txn.h"
@@ -2578,22 +2579,24 @@ struct SelectDest {
#endif
/*
- * At least one instance of the following structure is created for each
- * trigger that may be fired while parsing an INSERT, UPDATE or DELETE
- * statement. All such objects are stored in the linked list headed at
- * Parse.pTriggerPrg and deleted once statement compilation has been
- * completed.
- *
- * A Vdbe sub-program that implements the body and WHEN clause of trigger
- * TriggerPrg.pTrigger, assuming a default ON CONFLICT clause of
- * TriggerPrg.orconf, is stored in the TriggerPrg.pProgram variable.
- * The Parse.pTriggerPrg list never contains two entries with the same
- * values for both pTrigger and orconf.
- *
- * The TriggerPrg.aColmask[0] variable is set to a mask of old.* columns
- * accessed (or set to 0 for triggers fired as a result of INSERT
- * statements). Similarly, the TriggerPrg.aColmask[1] variable is set to
- * a mask of new.* columns used by the program.
+ * At least one instance of the following structure is created for
+ * each trigger that may be fired while parsing an INSERT, UPDATE
+ * or DELETE statement. All such objects are stored in the linked
+ * list headed at Parse.pTriggerPrg and deleted once statement
+ * compilation has been completed.
+ *
+ * A Vdbe sub-program that implements the body and WHEN clause of
+ * trigger TriggerPrg.pTrigger, assuming a default ON CONFLICT
+ * clause of TriggerPrg.orconf, is stored in the
+ * TriggerPrg.pProgram variable. The Parse.pTriggerPrg list never
+ * contains two entries with the same values for both pTrigger
+ * and orconf.
+ *
+ * The TriggerPrg.column_mask[0] variable is set to a mask of
+ * old.* columns accessed (or set to 0 for triggers fired as a
+ * result of INSERT statements). Similarly, the
+ * TriggerPrg.column_mask[1] variable is set to a mask of new.*
+ * columns used by the program.
*/
struct TriggerPrg {
/** Trigger this program was coded from. */
@@ -2601,7 +2604,8 @@ struct TriggerPrg {
TriggerPrg *pNext; /* Next entry in Parse.pTriggerPrg list */
SubProgram *pProgram; /* Program implementing pTrigger/orconf */
int orconf; /* Default ON CONFLICT policy */
- u32 aColmask[2]; /* Masks of old.*, new.* columns accessed */
+ /* Masks of old.*, new.* columns accessed. */
+ uint64_t column_mask[2];
};
enum ast_type {
@@ -2680,8 +2684,10 @@ struct Parse {
int nSelectIndent; /* How far to indent SELECTTRACE() output */
Parse *pToplevel; /* Parse structure for main program (or NULL) */
u32 nQueryLoop; /* Est number of iterations of a query (10*log2(N)) */
- u32 oldmask; /* Mask of old.* columns referenced */
- u32 newmask; /* Mask of new.* columns referenced */
+ /* Mask of old.* columns referenced. */
+ uint64_t oldmask;
+ /* Mask of new.* columns referenced. */
+ uint64_t newmask;
u8 eTriggerOp; /* TK_UPDATE, TK_INSERT or TK_DELETE */
u8 eOrconf; /* Default ON CONFLICT policy for trigger steps */
/** Region to make SQL temp allocations. */
@@ -4069,7 +4075,7 @@ TriggerStep *sqlTriggerDeleteStep(sql *, Token *, Expr *);
*
* @retval mask value.
*/
-u32
+uint64_t
sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger,
ExprList *changes_list, int new, int tr_tm,
struct space *space, int orconf);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index c984950647..519e3305a7 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -785,8 +785,8 @@ sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
sqlVdbeLinkSubProgram(pTop->pVdbe, pProgram);
pPrg->trigger = trigger;
pPrg->orconf = orconf;
- pPrg->aColmask[0] = 0xffffffff;
- pPrg->aColmask[1] = 0xffffffff;
+ pPrg->column_mask[0] = COLUMN_MASK_FULL;
+ pPrg->column_mask[1] = COLUMN_MASK_FULL;
/*
* Allocate and populate a new Parse context to use for
@@ -858,8 +858,8 @@ sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
pProgram->nMem = pSubParse->nMem;
pProgram->nCsr = pSubParse->nTab;
pProgram->token = (void *)trigger;
- pPrg->aColmask[0] = pSubParse->oldmask;
- pPrg->aColmask[1] = pSubParse->newmask;
+ pPrg->column_mask[0] = pSubParse->oldmask;
+ pPrg->column_mask[1] = pSubParse->newmask;
sqlVdbeDelete(v);
}
@@ -976,13 +976,13 @@ vdbe_code_row_trigger(struct Parse *parser, struct sql_trigger *trigger,
}
}
-u32
+uint64_t
sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger,
ExprList *changes_list, int new, int tr_tm,
struct space *space, int orconf)
{
const int op = changes_list != NULL ? TK_UPDATE : TK_DELETE;
- u32 mask = 0;
+ uint64_t mask = 0;
assert(new == 1 || new == 0);
for (struct sql_trigger *p = trigger; p != NULL; p = p->next) {
@@ -991,7 +991,7 @@ sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger,
TriggerPrg *prg =
sql_row_trigger(parser, p, space, orconf);
if (prg != NULL)
- mask |= prg->aColmask[new];
+ mask |= prg->column_mask[new];
}
}
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 0b645eb922..63a191f02a 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -100,7 +100,6 @@ sqlUpdate(Parse * pParse, /* The parser context */
/* List of triggers on pTab, if required. */
struct sql_trigger *trigger;
int tmask; /* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */
- int newmask; /* Mask of NEW.* columns accessed by BEFORE triggers */
int iEph = 0; /* Ephemeral table holding all primary key values */
int nKey = 0; /* Number of elements in regKey */
int aiCurOnePass[2]; /* The write cursors opened by WHERE_ONEPASS */
@@ -338,18 +337,15 @@ sqlUpdate(Parse * pParse, /* The parser context */
*/
if (is_pk_modified || hasFK != 0 || trigger != NULL) {
assert(space != NULL);
- u32 oldmask = hasFK ? space->fk_constraint_mask : 0;
+ uint64_t oldmask = hasFK ? space->fk_constraint_mask : 0;
oldmask |= sql_trigger_colmask(pParse, trigger, pChanges, 0,
TRIGGER_BEFORE | TRIGGER_AFTER,
space, on_error);
for (i = 0; i < (int)def->field_count; i++) {
- if (oldmask == 0xffffffff
- || (i < 32 && (oldmask & MASKBIT32(i)) != 0) ||
- sql_space_column_is_in_pk(space, i)) {
- testcase(oldmask != 0xffffffff && i == 31);
- sqlExprCodeGetColumnOfTable(v, def,
- pk_cursor, i,
- regOld + i);
+ if (column_mask_fieldno_is_set(oldmask, i) ||
+ sql_space_column_is_in_pk(space, i)) {
+ sqlExprCodeGetColumnOfTable(v, def, pk_cursor,
+ i, regOld + i);
} else {
sqlVdbeAddOp2(v, OP_Null, 0, regOld + i);
}
@@ -370,22 +366,20 @@ sqlUpdate(Parse * pParse, /* The parser context */
* may have modified them). So not loading those that are not going to
* be used eliminates some redundant opcodes.
*/
- newmask = sql_trigger_colmask(pParse, trigger, pChanges, 1,
- TRIGGER_BEFORE, space, on_error);
+ uint64_t newmask = sql_trigger_colmask(pParse, trigger, pChanges, 1,
+ TRIGGER_BEFORE, space, on_error);
for (i = 0; i < (int)def->field_count; i++) {
j = aXRef[i];
if (j >= 0) {
sqlExprCode(pParse, pChanges->a[j].pExpr,
regNew + i);
- } else if (0 == (tmask & TRIGGER_BEFORE) || i > 31
- || (newmask & MASKBIT32(i))) {
+ } else if ((tmask & TRIGGER_BEFORE) == 0 ||
+ column_mask_fieldno_is_set(newmask, i) != 0) {
/* This branch loads the value of a column that will not be changed
* into a register. This is done if there are no BEFORE triggers, or
* if there are one or more BEFORE triggers that use this value via
* a new.* reference in a trigger program.
*/
- testcase(i == 31);
- testcase(i == 32);
sqlExprCodeGetColumnToReg(pParse, def, i,
pk_cursor, regNew + i);
} else {
--
GitLab