Fix max key size using client sizes instead of server sizes#5888
Fix max key size using client sizes instead of server sizes#5888chands10 wants to merge 1 commit intobloomberg:mainfrom
Conversation
| if (ck->expr) { | ||
| /* TODO: calculate correct on-disk size for expression index pieces. The current code does this (incorrectly) as | ||
| * well. */ | ||
| return 1; |
There was a problem hiding this comment.
This will be addressed in a separate pr. This is same as current behavior
| /* For cstrings, artificially add 1 to the size. Since we strip the '\0' at the end when storing on disk, we could | ||
| * technically store a cstring of size 513 in a key. But that would be confusing to the client. */ | ||
| if (sym->type == T_CSTR) { | ||
| ++sz; |
There was a problem hiding this comment.
don't allow cstring(513)
| } | ||
| for (ii = 0; ii < tbl->nix; ii++) { | ||
| tmpidxsz = dyns_get_idx_size(ii); | ||
| if (tmpidxsz < 1 || tmpidxsz > MAXKEYLEN) { |
There was a problem hiding this comment.
tmpidxsz is not the correct key size
There was a problem hiding this comment.
Think this check is redundant
There was a problem hiding this comment.
Only use it if using old logic (still redundant even then)
| cleanup_newdb(tbl); | ||
| return NULL; | ||
| } | ||
| tbl->ix_keylen[ii] = tmpidxsz; /* ix lengths are adjusted later */ |
There was a problem hiding this comment.
ix lengths are adjusted later
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
scindex [failed with core dumped]
sc_truncate [db unavailable at finish]
consumer_non_atomic_default_consumer_generated **quarantined**
longreq_stats
tunables
|
Need to add tunable to revert to prev behavior if needed |
| int sz; | ||
| if (gbl_max_key_size_new) | ||
| sz = keyondisksize2(macc_globals->workkey) - | ||
| 1; /* COMDB2 CURRENTLY SUPPORTS 512 byte KEYS (not including null byte) */ |
There was a problem hiding this comment.
keyondisk basically subtracts 1 inside its function. Subtract 1 on outside here to not include one null byte
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
logfill [db unavailable at finish] **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
remsql_locks_rte_connect_generated **quarantined**
remsql_locks **quarantined**
tunables
reco-ddlk-sql [timeout] **quarantined**
|
cdb2test Apr 29 17:21:56 2026 success keysize.R20260429.9 |
Signed-off-by: Salil Chandra <schandra107@bloomberg.net>
|
^^ update tunables test |
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
sc_resume_logicalsc_generated **quarantined**
reco-ddlk-sql **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
remsql_locks_rte_connect_generated **quarantined**
remsql_locks **quarantined**
sc_downgrade [timeout] **quarantined**
2 participants
Max key size was based on client sizes for some types, not server.
Max key size is defined as 512, but internally really 513 so we can have a key on byte(512) (+1 for null byte).
Don't allow cstring 513 though, even though this takes up 513 - 1 + 1 = 513 bytes. Subtract one because we don't store '\0' on disk.