Skip to content

Commit

Permalink
Correctly handle indexes on column prefixes
Browse files Browse the repository at this point in the history
Summary:
The code used table->field[n] when packing/unpacking index values.
This happens to work most of the time, however the right way to
do it is to use table->key_info[i].key_part[j], which has a field
object that describes the field use in the index.

Differential Revision: https://reviews.facebook.net/D39471
  • Loading branch information
spetrunia authored and inikep committed Jan 17, 2022
1 parent d475e8e commit 5dbd7d0
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 14 deletions.
4 changes: 2 additions & 2 deletions storage/rocksdb/ha_rocksdb.cc
Expand Up @@ -2735,7 +2735,7 @@ bool ha_rocksdb::check_keyread_allowed(uint inx, uint part, bool all_parts) cons
KEY *key_info= &table_share->key_info[inx];

Field_pack_info dummy1;
res= dummy1.setup(key_info->key_part[part].field);
res= dummy1.setup(key_info->key_part[part].field, inx, part);

if (res && all_parts)
{
Expand All @@ -2745,7 +2745,7 @@ bool ha_rocksdb::check_keyread_allowed(uint inx, uint part, bool all_parts) cons
if ((field= key_info->key_part[i].field))
{
Field_pack_info dummy;
if (!dummy.setup(field))
if (!dummy.setup(field, inx, i))
{
/* Cannot do index-only reads for this column */
res= false;
Expand Down
29 changes: 21 additions & 8 deletions storage/rocksdb/rdb_datadic.cc
Expand Up @@ -204,8 +204,11 @@ void RDBSE_KEYDEF::setup(TABLE *tbl)
int max_part_len= 0;
bool simulating_extkey= false;
uint dst_i= 0;

uint keyno_to_set= keyno;
uint keypart_to_set= 0;
/* this loop also loops over the 'extended key' tail */
for (uint src_i= 0; src_i < m_key_parts; src_i++)
for (uint src_i= 0; src_i < m_key_parts; src_i++, keypart_to_set++)
{
Field *field= key_part->field;

Expand All @@ -232,7 +235,7 @@ void RDBSE_KEYDEF::setup(TABLE *tbl)
if (field->real_maybe_null())
max_len +=1; // NULL-byte

pack_info[dst_i].setup(field);
pack_info[dst_i].setup(field, keyno_to_set, keypart_to_set);
pack_info[dst_i].unpack_data_offset= unpack_len;

if (pk_info)
Expand All @@ -258,6 +261,8 @@ void RDBSE_KEYDEF::setup(TABLE *tbl)
if (secondary_key && src_i+1 == key_info->actual_key_parts)
{
simulating_extkey= true;
keyno_to_set= tbl->s->primary_key;
keypart_to_set= (uint)-1;
key_part= pk_info->key_part;
}

Expand Down Expand Up @@ -343,8 +348,9 @@ uint RDBSE_KEYDEF::get_primary_key_tuple(TABLE *table,

if (have_value)
{
Field *field= table->field[pack_info[i].fieldnr];
if (pack_info[i].skip_func(&pack_info[i], field, &reader))
Field_pack_info *fpi= &pack_info[i];
Field *field= fpi->get_field_in_table(table);
if (fpi->skip_func(fpi, field, &reader))
return INVALID_LEN;
}

Expand Down Expand Up @@ -460,7 +466,7 @@ uint RDBSE_KEYDEF::pack_record(TABLE *tbl,

for (uint i=0; i < n_key_parts; i++)
{
Field *field= tbl->field[pack_info[i].fieldnr];
Field *field= pack_info[i].get_field_in_table(tbl);

// Old Field methods expected the record pointer to be at tbl->record[0].
// The quick and easy way to fix this was to pass along the offset
Expand Down Expand Up @@ -629,7 +635,7 @@ int RDBSE_KEYDEF::unpack_record(TABLE *table, uchar *buf,
for (uint i= 0; i < m_key_parts ; i++)
{
Field_pack_info *fpi= &pack_info[i];
Field *field= table->field[pack_info[i].fieldnr];
Field *field= fpi->get_field_in_table(table);

if (fpi->unpack_func)
{
Expand Down Expand Up @@ -1016,12 +1022,14 @@ int skip_variable_length(Field_pack_info *fpi, Field *field, Stream_reader *read
FALSE - Otherwise
*/

bool Field_pack_info::setup(Field *field)
bool Field_pack_info::setup(Field *field, uint keynr_arg, uint key_part_arg)
{
int res= false;
enum_field_types type= field->real_type();

fieldnr= field->field_index;
keynr= keynr_arg;
key_part= key_part_arg;

maybe_null= field->real_maybe_null();
make_unpack_info_func= NULL;
unpack_func= NULL;
Expand Down Expand Up @@ -1107,6 +1115,11 @@ bool Field_pack_info::setup(Field *field)
}


Field *Field_pack_info::get_field_in_table(TABLE *tbl)
{
return tbl->key_info[keynr].key_part[key_part].field;
}

#if 0
void _rdbse_store_blob_length(uchar *pos,uint pack_length,uint length)
{
Expand Down
32 changes: 28 additions & 4 deletions storage/rocksdb/rdb_datadic.h
Expand Up @@ -440,10 +440,34 @@ class Field_pack_info
*/
index_field_skip_t skip_func;

/* Number of field in the table */
int fieldnr;

bool setup(Field *field);
private:
/*
Location of the field in the table (key number and key part number).
Note that this describes not the field, but rather a position of field in
the index. Consider an example:
col1 VARCHAR (100),
INDEX idx1 (col1)),
INDEX idx2 (col1(10)),
Here, idx2 has a special Field object that is set to describe a 10-char
prefix of col1.
We must also store the keynr. It is needed for implicit "extended keys".
Every key in MyRocks needs to include PK columns. Generally, SQL layer
includes PK columns as part of its "Extended Keys" feature, but sometimes
it does not (known examples are unique secondary indexes and partitioned
tables).
In that case, MyRocks's index descriptor has invisible suffix of PK
columns (and the point is that these columns are parts of PK, not parts
of the current index).
*/
uint keynr;
uint key_part;
public:
bool setup(Field *field, uint keynr_arg, uint key_part_arg);
Field *get_field_in_table(TABLE *tbl);
};


Expand Down

0 comments on commit 5dbd7d0

Please sign in to comment.