Support IPROTO_FEATURE_SPACE_AND_INDEX_NAMES
oleg-jukovec opened this issue · 8 comments
We could use space/index names directly without a resolving to space/index id by schema for Tarantool 3.0.
// Using space [index] names instead of identifiers support:
// IPROTO_SPACE_NAME and IPROTO_INDEX_NAME fields in IPROTO_SELECT,
// IPROTO_UPDATE and IPROTO_DELETE request body;
// IPROTO_SPACE_NAME field in IPROTO_INSERT, IPROTO_REPLACE,
// IPROTO_UPDATE and IPROTO_UPSERT request body.
IPROTO_FEATURE_SPACE_AND_INDEX_NAMES = 5See:
So, here is the steps I think I need to do to resolve this issue:
- Add
IPROTO_FEATURE_SPACE_AND_INDEX_NAMESfeature to the list of features in theprotocol.go(and update protocol-related tests). - Add new func to the
SchemaResolvertype that will return true if theIPROTO_FEATURE_SPACE_AND_INDEX_NAMESis supported in the current Tarantool version. Or we can add a new bool field to theSchemastructure (and fill it in theloadSchemafunction). - After that create separate functions-fillers (as example, instead of just
fillInsertcreatefillInsertBySpaceIDandfillInsertBySpaceName), that will use different iproto keys (IPROTO_SPACE_ID,IPROTO_SPACE_NAMEorIPROTO_INDEX_NAMEfor select, update and delete requests). But in this case, for example, fordeleteRequestwe will need 4 functions (because we can passspace_idorspace_nameas well asindex_idorindex_name).
Or we can leave justfillInsert, but let it acceptspaceInfo interface{}as a second argument and correct iproto key as fourth. As example, this function
Lines 53 to 67 in a664c6b
will look like this:
func fillInsert(enc *msgpack.Encoder, spaceInfo interface{}, tuple interface{}, iprotoKey iproto.Key) error {
if err := enc.EncodeMapLen(2); err != nil {
return err
}
if err := enc.EncodeUint(uint64(iprotoKey)); err != nil {
return err
}
if err := enc.Encode(spaceInfo); err != nil {
return err
}
if err := enc.EncodeUint(uint64(iproto.IPROTO_TUPLE)); err != nil {
return err
}
return enc.Encode(tuple)
}
Or, if we want to use enc.EncodeUint or enc.EncodeString instead of enc.Encode, we could create an if statement, that will check iprotoKey:
if (iprotoKey == iproto.IPROTO_SPACE_NAME) { use EncodeString }
else { use EncodeUint }
- After that, in the
Bodycall, we check, ifreq.space(orreq.index) has typestring, we will call corresponding functions, or pass corresponding keys to thefill...function (without call to theResolveSpaceIndex). But right nowResolveSpaceIndexresolves both space and index. So make things easier we could split this function into two (resolver for space and index). It will help with the cases, when user providesspace_idandindex_name(orspace_nameandindex_id), so we need to resolve only one (but not two) argument.
We also need to check if Tarantool supportsIPROTO_FEATURE_SPACE_AND_INDEX_NAMES(given the info from the step 2). If it is not supported, and we havespace_nameorindex_namewe will try and resolve them as we do right now:
Lines 377 to 468 in a664c6b
- Update current tests, if needed.
- Write new tests, update docs.
Thank you.
Add new func to the SchemaResolver type that will return true if the IPROTO_FEATURE_SPACE_AND_INDEX_NAMES is supported in the current Tarantool version.
But right now ResolveSpaceIndex resolves both space and index. So make things easier we could split this function into two (resolver for space and index).
Nice, let's see the final interface.
Or we can leave just fillInsert, but let it accept spaceInfo interface{} as a second argument and correct iproto key as fourth. As example, this function
I suggest first to try to run a benchmark with the change to see if there will be additional allocations because of this.
I checked the change for Insert function using benchmem and memprofile, and there was no difference in the number of allocations. So I believe there is no additional allocations because of this change.
I checked the change for
Insertfunction usingbenchmemandmemprofile, and there was no difference in the number of allocations. So I believe there is no additional allocations because of this change.
Does compiler with -mm (or something like this) report about escapes to heap?
If not, then make sure that nothing change in that benchmark with the change for select request:
#283
And let's do that way. It looks easier from the code side.
Does compiler with
-mm(or something like this) report aboutescapes to heap?
After running go build -gcflags='-m=3' . |& grep escape I got this output: spaceNo does not escape (spaceNo was passed to the fillInsert function as an interface{} argument). I will also check a benchmark for select request soon.
Does compiler with
-mm(or something like this) report aboutescapes to heap?After running
go build -gcflags='-m=3' . |& grep escapeI got this output:spaceNo does not escape(spaceNo was passed to thefillInsertfunction as an interface{} argument). I will also check a benchmark for select request soon.
Thank you. Ok, let's do in that way than.
Benchmark for select request shows the same 15 allocations (after changes in fillSelect and fillSearch). With the spaceNo does not escape and indexNo does not escape messages from the compiler.