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.

Sign up for GitHub

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

Jump to bottom

box: support space and index names in IPROTO requests #8573

Merged
merged 2 commits into from
May 22, 2023
Merged

box: support space and index names in IPROTO requests #8573

merged 2 commits into from
May 22, 2023

Conversation

CuriousGeorgiy
Copy link
Member

@CuriousGeorgiy CuriousGeorgiy commented Apr 15, 2023 •
edited

Closes #8146

@CuriousGeorgiy CuriousGeorgiy requested a review from a team as a code owner April 15, 2023 18:54
@CuriousGeorgiy CuriousGeorgiy self-assigned this Apr 15, 2023
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-8146-{space,index}-names-in-iproto-requests branch from 297512e to ddd1901 Compare April 16, 2023 12:19
@coveralls
Copy link

coveralls commented Apr 16, 2023 •
edited

Coverage Status

Coverage: 85.778% (+0.04%) from 85.742% when pulling 81aaacc on CuriousGeorgiy:gh-8146-{space,index}-names-in-iproto-requests into 4507c59
on tarantool:master.

@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-8146-{space,index}-names-in-iproto-requests branch from ddd1901 to 206f8bd Compare April 16, 2023 12:45
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-8146-{space,index}-names-in-iproto-requests branch from 206f8bd to e3cd30d Compare April 16, 2023 13:36
andreyaksenov
andreyaksenov approved these changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@andreyaksenov andreyaksenov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor suggestions:

changelogs/unreleased/gh-8146-space-and-index-names-in-iproto-requests.md Outdated Show resolved Hide resolved
changelogs/unreleased/gh-8146-space-and-index-names-in-iproto-requests.md Outdated Show resolved Hide resolved
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-8146-{space,index}-names-in-iproto-requests branch from e3cd30d to ea8b396 Compare April 17, 2023 07:45
locker
locker reviewed Apr 18, 2023
View reviewed changes
changelogs/unreleased/gh-8146-space-and-index-names-in-iproto-requests.md Outdated Show resolved Hide resolved
src/box/iproto_constants.c Outdated Show resolved Hide resolved
src/box/iproto.cc Outdated Show resolved Hide resolved
src/box/iproto.cc Outdated Show resolved Hide resolved
src/box/iproto.cc Outdated Show resolved Hide resolved
src/box/lua/net_box.lua Outdated Show resolved Hide resolved
src/box/lua/net_box.lua Outdated Show resolved Hide resolved
src/box/lua/net_box.lua Outdated Show resolved Hide resolved
src/box/lua/net_box.lua Outdated Show resolved Hide resolved
test/box-luatest/gh_8146_space_and_index_name_in_iproto_requests_test.lua Outdated Show resolved Hide resolved
@locker locker assigned CuriousGeorgiy and unassigned locker Apr 18, 2023
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-8146-{space,index}-names-in-iproto-requests branch 2 times, most recently from d6a9575 to 8654a53 Compare April 22, 2023 09:32
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-8146-{space,index}-names-in-iproto-requests branch 4 times, most recently from bc5adea to c57387a Compare April 24, 2023 06:38
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-8146-{space,index}-names-in-iproto-requests branch from 619c1cf to a62480b Compare May 8, 2023 13:00
@CuriousGeorgiy CuriousGeorgiy mentioned this pull request May 8, 2023
Merged
@CuriousGeorgiy CuriousGeorgiy removed the request for review from Totktonada May 10, 2023 06:58
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-8146-{space,index}-names-in-iproto-requests branch from a62480b to bab0479 Compare May 15, 2023 18:38
@locker
Copy link
Member

locker commented May 17, 2023

Will review after #8660 is merged and this PR is rebased.

@locker locker assigned CuriousGeorgiy and unassigned locker May 17, 2023
@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-8146-{space,index}-names-in-iproto-requests branch from bab0479 to c0542a1 Compare May 18, 2023 10:51
@CuriousGeorgiy
Copy link
Member Author

@locker #8660 was merged, rebased onto it.

@locker locker assigned alyapunov and unassigned locker May 18, 2023
@alyapunov alyapunov added full-ci Enables all tests for a pull request and removed full-ci Enables all tests for a pull request labels May 19, 2023
@CuriousGeorgiy CuriousGeorgiy removed the request for review from Gerold103 May 19, 2023 15:58
@CuriousGeorgiy
box: add space_by_name and space_index_by_name for arbitrary strings
97bd3a2 
Change original `space_by_name` to `space_by_name0` and
`space_index_by_name` to `space_index_by_name0`, since they accept
NULL-terminated names, and add `space_by_name` and `space_index_by_name`
for arbitrary strings.

Needed for tarantool#8146

NO_CHANGELOG=refactoring
NO_DOC=refactoring
NO_TEST=refactoring
@CuriousGeorgiy
box: support space and index names in IPROTO requests
81aaacc 
Add support for accepting IPROTO requests with space or index name instead
of identifier (name is preferred over identifier to disambiguate missing
identifiers from zero identifiers): mark space identifier request
key as present upon encountering space name, and delay resolution of
identifier until request gets to transaction thread.

Add support for sending DML requests from net.box connection objects with
disabled schema fetching by manually specifying space or index name or
identifier: when schema fetching is disabled, the space and index tables of
connections return wrapper tables that store necessary context (space or
index name or identifier, determined by type, connection object and space
for indexes) for performing requests. The space and index tables cache the
wrapper table they return.

Closes tarantool#8146

@TarantoolBot document
Title: Space and index name in IPROTO requests

Refer to design document for details:
https://www.notion.so/tarantool/Schemafull-IPROTO-cc315ad6bdd641dea66ad854992d8cbf?pvs=4#f4d4b3fa2b3646f1949319866428b6c0
@CuriousGeorgiy
Copy link
Member Author

@locker @alyapunov looks like there is another problem with IPROTO_SPACE_NAME, it causes a bit shift overflow here:

tarantool/src/box/xrow.c

Lines 922 to 928 in 4507c59

uint64_t key = mp_decode_uint(&data);
const char *value = data;
mp_next(&data);
if (key >= iproto_key_MAX ||
iproto_key_type[key] != mp_typeof(*value))
goto error;
key_map &= ~iproto_key_bit(key);

I added a key < 64 check before the bit flipping.

@CuriousGeorgiy CuriousGeorgiy force-pushed the gh-8146-{space,index}-names-in-iproto-requests branch from c0542a1 to 81aaacc Compare May 19, 2023 16:08
@CuriousGeorgiy CuriousGeorgiy added the full-ci Enables all tests for a pull request label May 19, 2023
@locker locker merged commit b9550f1 into tarantool:master May 22, 2023
143 checks passed
@oleg-jukovec oleg-jukovec mentioned this pull request Oct 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Totktonada Totktonada Totktonada left review comments

@olegrok olegrok olegrok left review comments

@locker locker locker approved these changes

@alyapunov alyapunov alyapunov approved these changes

@andreyaksenov andreyaksenov andreyaksenov approved these changes

Assignees

@alyapunov alyapunov

@olegrok olegrok

Labels
full-ci Enables all tests for a pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accept names in IPROTO requests
8 participants
@CuriousGeorgiy @coveralls @locker @Totktonada @alyapunov @olegrok @andreyaksenov @Gerold103