First functional INCR command #20
Conversation
Build for Mac in CI
1. Make it possible to use dirty writes for higher concurrency with lower consistency 2. Ensure that we don't have any value rows before proceeding 3. Avoid setting NULL value on rondb_key, it will be set anyways (updated mask)
There was a problem hiding this comment.
Once you've changed all references of 22.10.5 to 24.10.0, can you add a test to get_set.sh? you could append the followng:
incr_key="$key:incr"
set_and_get "$incr_key" 5
incr_output=$(redis-cli INCR "$incr_key")
incr_result=$(redis-cli GET "$incr_key")
if [[ "$incr_result" == 6 ]]; then
echo "PASS: Incremented key $incr_key to value $incr_result"
else
echo "FAIL: Incrementing $incr_key"
echo "Expected: 6"
echo "Received: $incr_result"
exit 1
fi
| @@ -35,6 +37,7 @@ extern NdbRecord *entire_key_record; | |||
|
|
|||
| struct key_table | |||
| { | |||
| Uint32 null_bits; | |||
There was a problem hiding this comment.
I.e., can you add a comment pls?
There was a problem hiding this comment.
Does one need one null_bits field for every nullable column?
There was a problem hiding this comment.
This is to be able to set and get NULL bits when doing reads and writes. You need bits for every nullable column, I use Uint32 simply to ensure that the columns are aligned on 32 bit boundaries.
| assign_ndb_err_to_response(response, FAILED_CREATE_TABLE_OBJECT, dict->getNdbError()); | ||
| return false; | ||
| } | ||
| memcpy(&key_row->redis_key[2], key_str, key_len); |
There was a problem hiding this comment.
Is this necessary? Since we should never change the key..
There was a problem hiding this comment.
We add two rows of NdbRecord, one is input that contains the searched for primary key, the second is the new values to write. We use the same row for both, so setting the primary key is part of the WHERE clause. Also it is used to write the values when performing an INSERT operation (the WRITE is either becoming an UPDATE or an INSERT.
There was a problem hiding this comment.
What I mean is whether the memory has to be copied or whether one could just reference the pointer
There was a problem hiding this comment.
You have defined an NdbRecord, this defines the offset from the pointer to the Primary key pointer row that you supply in writeTuple, so if you define the offset as 0 and send in key_str as the Primary key pointer I gather it could work, but for a multi-column PK you need to copy, I don't think this optimisation makes much sense, most keys are short and so not a problem to do a quick memcpy
pink/rondis/string/db_operations.cc
Outdated
| insert_value_ptr[0] = 1; // Length is 1 | ||
| insert_value_ptr[1] = 0; // Second length byte is 0 | ||
| insert_value_ptr[2] = '1'; //Inserts a string '1' | ||
| insert_value_ptr[3] = 0; |
There was a problem hiding this comment.
Or is this the 1 we will add to the value?
There was a problem hiding this comment.
This is the bytes we will write into value_start, the string '1' with 2 length bytes representing a length of 1.
There was a problem hiding this comment.
This is used in an INSERT
pink/rondis/string/db_operations.cc
Outdated
| */ | ||
| /* UPDATE code */ | ||
| code.read_full(value_start_col, REG6, REG2); // Read value_start column | ||
| code.load_const_u16(REG1, 6);//Memory offset 2 |
There was a problem hiding this comment.
You mean to write "Memory offset 6"?
There was a problem hiding this comment.
Yep, but probably better to remove the comment since I already commented what I use REG6 for
pink/rondis/string/db_operations.cc
Outdated
| * REG0 Memory offset == 4 | ||
| * REG1 Memory offset == 6 | ||
| * REG2 Size of value_start | ||
| * REG3 Size of value_start without length bytes |
There was a problem hiding this comment.
Why not just read the length bytes directly?
There was a problem hiding this comment.
When I read value_start from offset 0 I get the following:
Byte 0-3: This contains the AttributeHeader that contains attribute id and length of read, I don't use this.
Byte 4-5: This is the two length bytes in little-endian format
Byte 6-x: This is the actual string that can be up to 26500 bytes long
| * are updated through final update. | ||
| */ | ||
|
|
||
| const Uint32 mask = 0x55; |
There was a problem hiding this comment.
Why not use a base-10 integer?
There was a problem hiding this comment.
Because if I write 85 it gives no easy translation to which attribute ids that should be written.
0x55 is binary 0101 0101 thus I can conclude that we will write attribute 0, 2, 4 and 6 in the Update
section, the values for those columns will come from key_tab struct.
We will not set column 1 (== rondb_key) which means it will retain its value (it should always be NULL for INCR)
We will not set column 3 (tot_value_len) since it is set by the interpreted program
We will not set column 5 (value_start) since it is set by the interpreted program.
There was a problem hiding this comment.
Aha okay, that makes more sense now, thanks :)
|
Also, it seems to me that you only need to compile that interpreted code once, not on every request |
pink/rondis/string/db_operations.cc
Outdated
| code.sub_const_reg(REG3, REG2, 2);//Subtract 2 from length | ||
| code.str_to_int64(REG4, REG1, REG3);//Convert string to number into register 6 | ||
| code.add_const_reg(REG5, REG4, 1); //New integer value in register 6 | ||
| code.int64_to_str(REG3, REG1, REG5);//Convert to string |
There was a problem hiding this comment.
Why is this REG1 with u16(6) needed here and in code.str_to_int64?
There was a problem hiding this comment.
See above, 6 is the offset where the actual string starts
INCR modifications
No description provided.