-
Notifications
You must be signed in to change notification settings - Fork 143
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
Add support for SELECT command #115
Conversation
Thank you ! |
src/command.c
Outdated
struct pos_array *db_str = &db_data->pos; | ||
// TODO: Implement pos_to_long then use it here, someday. | ||
if (db_str->str_len == 1 && db_str->items->str[0] == '0') { | ||
conn_add_data(cmd->client, (uint8_t*)rep_ok, 5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the overhead of str_len
is acceptable.
src/command.c
Outdated
|
||
struct pos_array *db_str = &db_data->pos; | ||
// TODO: Implement pos_to_long then use it here, someday. | ||
if (db_str->str_len == 1 && db_str->items->str[0] == '0') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comparison is not very suitable here. If the command select 000
is issued, OK
should be returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, are you suggesting pos_to_long
?
ASSERT_TYPE(data, REP_ARRAY); | ||
ASSERT_ELEMENTS(data->elements == 2, data); | ||
|
||
struct redis_data *db_data = &data->element[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second argument of SELECT
is the index of database. The name db_data
is not quite suitable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it's the index of db, I think db
tells more than index
here.
@@ -559,3 +559,23 @@ size_t pos_to_str_with_limit(struct pos_array *pos, uint8_t *str, size_t limit) | |||
} | |||
return len; | |||
} | |||
|
|||
int pos_is_zero(struct pos_array *pos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redis uses strtoll
which is available in C99 for error handling. Both of us just missed it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make use of strtoll
, a pos_array
must be converted into a string first, too expensive for an operation like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we try our best to conform to redis, we should consider this:
> select hello
(error) ERR invalid DB index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I've thought about that.
The error messages are completely different in some cases:
Redis:
> select
(error) ERR wrong number of arguments for 'select' command
Corvus:
> select
(error) ERR Proxy fail to forward command
I'm afraid it's too much work to get them exactly the same in this PR.
And if nothing goes wrong, do we really have to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think supporting "SELECT is not allowed in cluster mode"
is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's enough.
src/parser.c
Outdated
} | ||
|
||
int len = 0; | ||
for (size_t i = 0; len < length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
length
here should be pos->pos_len
. See cmd_get_map_key
for how to iterate pos_array
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But pos_to_str
uses pos->str_len
instead of pos->pos_len
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pos_array
is used to represent only one string, consisting of split sub string stored in items
struct pos_array {
struct pos *items;
int str_len; // str_len == sum(items.len)
int pos_len; // pos_len == len(items)
int max_pos_size; // max_pos_size is the actual memory size allocated
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually that's what I'm thinking about at the beginning, then I find this and missread .pos_len = 2
into .pos_len = 12
.
Thanks for the hint, what about now?
No description provided.