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

net.box schema contains collation_id (number) instead of collation (string) #3941

Closed
Totktonada opened this issue Jan 15, 2019 · 6 comments
Closed
Assignees
Labels
bug Something isn't working good first issue Good for newcomers lua
Milestone

Comments

@Totktonada
Copy link
Member

How to reproduce (on clean instance, because no box.once is used):

box.cfg{listen = 3301}
box.schema.user.grant('guest', 'read,write,execute,create', 'universe')
box.schema.space.create('s')
box.space.s:create_index('pk')
box.space.s:create_index('sk', {type = 'tree', unique = false, parts = {{field = 2, type = 'string', is_nullable = false, collation = 'unicode_ci'}}})
net_box = require('net.box')
c = net_box.connect('localhost:3301')
box.space.s.index.sk.parts[1]
---
- type: string
  is_nullable: false
  collation: unicode_ci
  fieldno: 2
...
c.space.s.index.sk.parts[1]
---
- type: string
  is_nullable: false
  collation_id: 2
  fieldno: 2
...

As we see local box provides collation field, but net.box provides collation_id. There are two reasons why this looks as a problem for me:

  • It complicates box/lua/key_def.lua code (merger patchset) a bit (need to handle both formats).
  • Lack of some kind of a primary identifier for a collation. This should be its string name I think.

How can be implemented:

net.box can acquire and use info from _collation space.

@Totktonada Totktonada added the lua label Jan 15, 2019
@Totktonada
Copy link
Member Author

It also worth to handle the case when we connect to a quite old tarantool w/o _collation system space at all. It should not lead to an error, but parts should be shown w/o 'collation' field instead.

@Totktonada
Copy link
Member Author

If the key_def module created in the scope of #3276 will land before this patch it is necessary to remove collation_id parameter support in the module.

@kyukhin kyukhin added bug Something isn't working good first issue Good for newcomers labels Feb 8, 2019
@kyukhin kyukhin added this to the 2.1.1 milestone Feb 8, 2019
romanhabibov added a commit that referenced this issue Mar 5, 2019
romanhabibov added a commit that referenced this issue Mar 12, 2019
romanhabibov added a commit that referenced this issue Mar 21, 2019
Add "_vcollation" sysview to read it from net.box. This
sysview is always readable, exept when the user doesn't have
"public" role.

Needed for #3941
romanhabibov added a commit that referenced this issue Mar 21, 2019
Fetch "_vcollation" sysview to show collation name instead collation id.

Closes #3941
romanhabibov added a commit that referenced this issue Mar 21, 2019
Add "_vcollation" sysview to read it from net.box. This
sysview is always readable, exept when the user doesn't have
"public" role.

Needed for #3941
romanhabibov added a commit that referenced this issue Mar 21, 2019
Fetch "_vcollation" sysview to show collation name instead collation id.

Closes #3941
@Totktonada
Copy link
Member Author

Merger (#3276) will be pushed to 1.10, it depends on key_def module (it is part of merger's patchset). The reason of this issue is to simplify lua/key_def.c code and remove collation_id from it at all. So this issue should be fixed in 1.10 too.

@kyukhin Can you please review the milestone?

@kyukhin kyukhin modified the milestones: 2.1.2, 1.10.3 Mar 25, 2019
@kyukhin kyukhin modified the milestones: 1.10.3, 1.10.4 Apr 1, 2019
romanhabibov added a commit that referenced this issue May 16, 2019
Add "_vcollation" sysview to read it from net.box. This
sysview is always readable, exept when the user doesn't have
"public" role.

Needed for #3941
romanhabibov added a commit that referenced this issue May 16, 2019
Fetch "_vcollation" sysview to show collation name instead collation id.

Closes #3941
romanhabibov added a commit that referenced this issue May 16, 2019
Fetch "_vcollation" sysview to show collation name instead collation id.

Closes #3941
romanhabibov added a commit that referenced this issue May 28, 2019
Add "_vcollation" sysview to read it from net.box. This
sysview is always readable, exept when the user doesn't have
"public" role.

Needed for #3941
romanhabibov added a commit that referenced this issue May 28, 2019
Fetch "_vcollation" sysview to show collation name instead collation id.

Closes #3941
romanhabibov added a commit that referenced this issue May 30, 2019
Fetch "_vcollation" sysview to show collation name instead collation id.

Closes #3941
@locker
Copy link
Member

locker commented Jun 4, 2019

Merger (#3276) will be pushed to 1.10, it depends on key_def module (it is part of merger's patchset). The reason of this issue is to simplify lua/key_def.c code and remove collation_id from it at all. So this issue should be fixed in 1.10 too.

@kyukhin Can you please review the milestone?

Pushing this one to 1.10 would be tricky as it upgrades the schema to create _vcollation space. If we had to update the upgrade script both in 1.10 and 2.1 and master, then in master and 2.1 we'd have to handle the case when the space already exists (i.e. it was created by upgrade to a previous version), which is kinda ugly.

Do we really need _vcollation to backport the merger to 1.10? As a matter of fact, do we really need to backport the merger to 1.10?

@Totktonada
Copy link
Member Author

Now it seems that we should not backport merger to 1.10, the demand is gone. So this patch can be implemented for master only.

@locker
Copy link
Member

locker commented Jun 4, 2019

Now it seems that we should not backport merger to 1.10, the demand is gone. So this patch can be implemented for master only.

Okay then. Moving back to 2.2.

@locker locker modified the milestones: 1.10.4, 2.2.0 Jun 4, 2019
romanhabibov added a commit that referenced this issue Jun 5, 2019
Add "_vcollation" sysview to read it from net.box. This
sysview is always readable, except when a user doesn't have
"public" role.

Needed for #3941

@TarantoolBot document
Title: box.space._vcollation

_vcollation is a system space that represents a virtual view.
The structure of its tuples is identical to that of _collation.
Tuples of this sysview is always readable, except when the user
doesn't have "public" role.
romanhabibov added a commit that referenced this issue Jun 5, 2019
Fetch "_vcollation" sysview to show collation name instead collation id.

Closes #3941
romanhabibov added a commit that referenced this issue Jun 6, 2019
Add "_vcollation" sysview to read it from net.box. This
sysview is always readable, except when a user doesn't have
"public" role.

Needed for #3941

@TarantoolBot document
Title: box.space._vcollation

_vcollation is a system space that represents a virtual view.
The structure of its tuples is identical to that of _collation.
Tuples of this sysview is always readable, except when the user
doesn't have "public" role.
romanhabibov added a commit that referenced this issue Jun 6, 2019
Fetch "_vcollation" sysview to show collation name instead collation id.

Closes #3941
locker pushed a commit that referenced this issue Jun 6, 2019
Add "_vcollation" sysview to read it from net.box. This
sysview is always readable, except when a user doesn't have
"public" role.

Needed for #3941

@TarantoolBot document
Title: box.space._vcollation

_vcollation is a system space that represents a virtual view.
The structure of its tuples is identical to that of _collation.
Tuples of this sysview is always readable, except when the user
doesn't have "public" role.
@locker locker closed this as completed in a7c855e Jun 6, 2019
Totktonada added a commit that referenced this issue Jun 7, 2019
This is the regression from a7c855e
(net.box: fetch '_vcollation' sysview into the module). The result file
was corrupted, because the test sometimes produces incorrect result in
the test case for on_connect / on_disconnect triggers. This issue is
tracked by #4273.

Follow up #3941.
Totktonada added a commit that referenced this issue Jun 7, 2019
This is the regression from a7c855e
(net.box: fetch '_vcollation' sysview into the module). The result file
was corrupted, because the test sometimes produces incorrect result in
the test case for on_connect / on_disconnect triggers. This issue is
tracked by #4273.

Follow up #3941.
@kyukhin kyukhin added the tmp label Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers lua
Projects
None yet
Development

No branches or pull requests

4 participants