Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: verify if legacy hints are useful #3121

Closed
kyukhin opened this issue Feb 5, 2018 · 1 comment
Closed

sql: verify if legacy hints are useful #3121

kyukhin opened this issue Feb 5, 2018 · 1 comment
Assignees
Labels
Milestone

Comments

@kyukhin
Copy link
Contributor

kyukhin commented Feb 5, 2018

There're two different things here:

  1. Need to understand if hints are useful for Tarantool (values are BTREE_BULKLOAD and BTREE_SEEK_EQ) and adopt it if so. Remove all traces otherwise
  2. There's a status flag in cursor structurer (VALID/INVALID). Need to check if this flag can be used w/ Tarantool backend. Remove all traces otherwise
@kyukhin kyukhin added the sql label Feb 5, 2018
@kostja kostja added this to the 1.8.5 milestone Feb 7, 2018
@kshcherbatov
Copy link
Contributor

kshcherbatov commented Jun 9, 2018

The only place where pCursor->hint is set NOW is OP_ReopenIdx, OP_OpenRead, OP_OpenWrite
--> (pOp->p5 & (OPFLAG_BULKCSR|OPFLAG_SEEKEQ)) mask based on VDBE P5 VALUE:

1. OPFLAG_BULKCSR  == BTREE_BULKLOAD
2. OPFLAG_SEEKEQ    == BTREE_SEEK_EQ
  1. The BTREE_BULKLOAD hint was used in src/box/sql/btree.c in balance function
    balance_nonroot(MemPage * pParent, /* Parent page of siblings being balanced /
    int iParentIdx, /
    Index of "the page" in pParent /
    u8 * aOvflSpace, /
    page-size bytes of space for parent ovfl /
    int isRoot, /
    True if pParent is a root-page /
    int bBulk /
    True if this call is part of a bulk load */
    )
    to fill last flag bBulk.

With @Korablev77 patch 7a56118
sql: remove original SQLite backend
this btree.c become useless and was dropped.

So, BTREE_BULKLOAD, same as OPFLAG_BULKCSR could be deleted.

a) First of all, part of code is wrapped in SQLITE_ENABLE_CURSOR_HINTS that is disabled in tarantool build. I've dropped all the code in such sections and going to review the remaining one.

b) The flag is used in sqlite3CursorMovetoUnpacked (OP_SeekLE and OP_SeekGE) to setup iter_type, in (OP_SeekLT, OP_SeekLE, OP_SeekGE, OP_SeekGT), (OP_NoConflict, OP_NotFound, OP_Found), (OP_IdxDelete)

c) The VDBE P5 argument is setted OPFLAG_SEEKEQ by (1)sql_create_index, (2)sqlite3WhereBegin.
c.1: sql_create_index

sqlite3VdbeAddOp4Int(v, OP_OpenWrite, iCursor,  index_space_ptr_reg, 6);
sqlite3VdbeChangeP5(v, OPFLAG_SEEKEQ);

This means that new cursor would have BTREE_SEEK_EQ==OPFLAG_SEEKEQ and then on lookup with OP_SeekLE (in getNewIid) that allows choose correct behavior on find and iter_type.

In other words, this match flag description

* If the cursor P1 was opened using the OPFLAG_SEEKEQ flag, then this
 * opcode will always land on a record that equally equals the key, or
 * else jump immediately to P2.  When the cursor is OPFLAG_SEEKEQ, this
 * opcode must be followed by an IdxLE opcode with the same arguments.
 * The IdxLE opcode will be skipped if this opcode succeeds, but the
 * IdxLE opcode will be used on subsequent loop iterations.

and this scenario is still in use. We should keep it.

====================
Believe, I can drop SQLITE_ENABLE_CURSOR_HINTS macro and useless BTREE_BULKLOAD.

kshcherbatov added a commit that referenced this issue Jun 9, 2018
Deleted BTREE_BULKLOAD and OPFLAG_BULKCSR as there become
useless since btree.c was dropped as a part of #2419.
Removed SQLITE_ENABLE_CURSOR_HINTS sections as this code
has never been in use.
Start use OPFLAG_SEEKEQ instead of equal BTREE_SEEK_EQ as
this macro name has no sense since no btree present in SQL.

Resolves #3121.
kshcherbatov added a commit that referenced this issue Jun 9, 2018
Deleted BTREE_BULKLOAD and OPFLAG_BULKCSR as there become
useless since btree.c was dropped as a part of #2419.
Removed SQLITE_ENABLE_CURSOR_HINTS sections as this code
has never been in use.
Start use OPFLAG_SEEKEQ instead of equal BTREE_SEEK_EQ as
this macro name has no sense since no btree present in SQL.

Resolves #3121.
kshcherbatov added a commit that referenced this issue Jun 10, 2018
Deleted BTREE_BULKLOAD and OPFLAG_BULKCSR as there become
useless since btree.c was dropped as a part of #2419.
Removed SQLITE_ENABLE_CURSOR_HINTS sections as this code
has never been in use.
Start use OPFLAG_SEEKEQ instead of equal BTREE_SEEK_EQ as
this macro name has no sense since no btree present in SQL.

Resolves #3121.
kshcherbatov added a commit that referenced this issue Jun 20, 2018
Deleted BTREE_BULKLOAD and OPFLAG_BULKCSR as they become
useless since btree.c was dropped as a part of #2419.
Removed SQLITE_ENABLE_CURSOR_HINTS sections as this code
has never been in use.
Start use OPFLAG_SEEKEQ instead of equal BTREE_SEEK_EQ as
this macro name has no sense since no btree present in SQL.

Resolves #3121.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants