Hi, Sanja!
On Oct 04, sanja(a)askmonty.org wrote:
> ------------------------------------------------------------
> revno: 3202
> revision-id: sanja(a)askmonty.org-20111004111459-1i44vpkdm1j3lisl
> parent: psergey(a)askmonty.org-20110929130743-yj0zl4hdvoydnmda
> committer: sanja(a)askmonty.org
> branch nick: work-maria-5.3-frm2
> timestamp: Tue 2011-10-04 14:14:59 +0300
> message:
> open_binary_frm() decomposed an methods of TABLE_SHARE to allow
> engine to create TABLE_SHARE.
>
> utype and geometry_type moved out of Field.
Thanks!
Here're my thoughts.
The summary is:
1. I'd like to have it simpler. It's going to be an external API for
engines to use, after all.
2. Don't try to support all optimizations that open_binary_frm() uses,
and old frm formats - keep that out of the API, open_binary_frm()
can modify TABLE_SHARE directly to achieve the necessary effect.
3. Try to create a usage example - it will show the unnecessary
complexity
See all the comments below:
> === modified file 'sql/handler.h'
> --- a/sql/handler.h 2011-07-14 13:44:37 +0000
> +++ b/sql/handler.h 2011-10-04 11:14:59 +0000
> @@ -298,6 +298,31 @@ enum legacy_db_type
> DB_TYPE_FIRST_DYNAMIC=42,
> DB_TYPE_DEFAULT=127 // Must be last
> };
> +
> +/*
> + We use three additional unireg types for TIMESTAMP to overcome limitation
> + of current binary format of .frm file. We'd like to be able to support
> + NOW() as default and on update value for such fields but unable to hold
> + this info anywhere except unireg_check field. This issue will be resolved
> + in more clean way with transition to new text based .frm format.
> + See also comment for Field_timestamp::Field_timestamp().
> +*/
> +enum utype
> +{
> + UT_NONE, UT_DATE, UT_SHIELD, UT_NOEMPTY, UT_CASEUP, UT_PNR,
> + UT_BGNR, UT_PGNR, UT_YES, UT_NO, UT_REL,
> + UT_CHECK, UT_EMPTY, UT_UNKNOWN_FIELD, UT_CASEDN,
> + UT_NEXT_NUMBER, UT_INTERVAL_FIELD,
> + UT_BIT_FIELD, UT_TIMESTAMP_OLD_FIELD, UT_CAPITALIZE,
> + UT_BLOB_FIELD, UT_TIMESTAMP_DN_FIELD, UT_TIMESTAMP_UN_FIELD,
> + UT_TIMESTAMP_DNUN_FIELD
> +};
Why wouldn't you remove all unused values from utype enum?
> +enum geometry_type
> +{
> + GEOM_GEOMETRY = 0, GEOM_POINT = 1, GEOM_LINESTRING = 2, GEOM_POLYGON = 3,
> + GEOM_MULTIPOINT = 4, GEOM_MULTILINESTRING = 5, GEOM_MULTIPOLYGON = 6,
> + GEOM_GEOMETRYCOLLECTION = 7
> +};
> /*
> Better name for DB_TYPE_UNKNOWN. Should be used for engines that do not have
> a hard-coded type value here.
> === modified file 'sql/table.cc'
> --- a/sql/table.cc 2011-09-02 12:10:10 +0000
> +++ b/sql/table.cc 2011-10-04 11:14:59 +0000
> @@ -675,6 +684,797 @@ err_not_open:
> }
>
>
> +/**
> + Set some general table parameters (should be called first after the
> + TABLE SHARE initialization)
> +
> + @param frm_ver Version of the frm (FRM_VER_TRUE_VARCHAR)
no need to pass it as a parameter, set frm version always to the latest one.
if needed - overwrite it later from open_binary_frm().
don't add support for historical features into the API.
> + @param db_create_opt Flags of CREATE options
> + @param avg_row_len Average row length
> + @param transact CREATE option TRANSACTION
> + @param page_chcksum CREATE option PAGE_CHECK_SUM
> + @param rtype CREATE option ROW_TYPE (used by aria only)
> + @param charset default charset for the table.
> + @param null_fld_first Should be TRUE for modern frm
same as frm_version
> + @param mi_rows CREATE option MIN_ROWS
> + @param ma_rows CREATE option MAX_ROWS
> + @param reclen Record buffer length
> + @param extra_rec_buf_len Extra record buffer length (???)
> + @param key_blck_size CREATE option KEY_BLOCK_SIZE
> + @param new_fld_pck_flg Should be 2 for modern frm
same as frm_version
> + @param error Error code assigned here in case of error
> +
> + @retval FALSE OK
> + @retval TRUE Error (error assigned)
> +*/
> +
> +bool TABLE_SHARE::set_init_parameters(uchar frm_ver, ulong mysql_ver,
> + uint db_create_opt,
> + ulong avg_row_len,
> + enum ha_choice transact,
> + enum ha_choice page_chcksum,
> + enum row_type rtype,
> + CHARSET_INFO *charset,
> + bool null_fld_first,
> + ha_rows mi_rows,
> + ha_rows ma_rows,
> + ulong reclen,
> + uint extra_rec_buf_len,
> + uint key_blck_size,
> + uint new_fld_pck_flg,
> + int &error)
use a pointer, not a reference. Or, better, just return the error code
> +{
> + frm_version= frm_ver;
> + mysql_version= mysql_ver;
> + db_options_in_use= db_create_options= db_create_opt;
> + if (db_create_options & HA_OPTION_LONG_BLOB_PTR)
> + blob_ptr_size= portable_sizeof_char_ptr;
> + avg_row_length= avg_row_len;
> + transactional= transact;
> + page_checksum= page_chcksum;
> + row_type= rtype;
> + table_charset= charset;
> + null_field_first= null_fld_first;
> + db_record_offset= 1;
> + min_rows= mi_rows;
> + max_rows= ma_rows;
> + stored_rec_length= reclength= reclen;
> + key_block_size= key_blck_size;
> + new_field_pack_flag= new_fld_pck_flg;
> + rec_buff_length= ALIGN_SIZE(reclength + 1 + extra_rec_buf_len);
> + if (!(default_values= (uchar *) alloc_root(&mem_root,
> + rec_buff_length)))
> + {
> + error= 4;
> + return TRUE;
> + }
> + /* Default values */
> + system= 0;
> + crypted= 0;
> +
> + return FALSE;
> +}
> +
> +
> +/**
> + Allocate space for indices and its parts
> +
> + @param nkeys Number of keys
> + @param nkey_parts Number of key parts of all keys
> + @param names_len Sum of lengths of all indices names (string ends
> + are not counted)
> + @param error Error code assigned here in case of error\
return the error code instead
> +
> + @retval FALSE OK
> + @retval TRUE Error (error assigned)
> +*/
> +
> +bool TABLE_SHARE::allocate_indices_space(uint nkeys, uint nkey_parts,
> + uint names_len, int &error)
> +{
> + uint n_length= (nkeys * sizeof(KEY) +
> + nkey_parts * sizeof(KEY_PART_INFO));
> +
> + DBUG_ASSERT(key_info == NULL); // prevent reallocation
> + DBUG_ASSERT(nkeys <= nkey_parts);
> +
> + keys= nkeys;
> + key_parts= nkey_parts;
> + keys_for_keyread.init(0);
> + keys_in_use.init(keys);
> +
> +
> + if (!(rec_per_key= (ulong *) alloc_root(&mem_root,
> + sizeof(ulong) * key_parts +
> + n_length +
> + (names_len + nkeys + 1))))
> + {
> + error= 4;
> + return TRUE; /* purecov: inspected */
> + }
> + key= key_info=
> + my_reinterpret_cast(KEY*) (rec_per_key + key_parts);
> + bzero((char*) key_info, n_length);
> + first_key_part= key_part=
> + my_reinterpret_cast(KEY_PART_INFO*) (key_info + keys);
> + first_keyname= keyname=
> + my_reinterpret_cast(char *) (first_key_part + key_parts);
> + is_keys_allocated= TRUE;
> + if (key_parts == 0)
> + {
> + DBUG_ASSERT(keys == 0);
> + is_keynames_ready= is_keyparts_ready= TRUE;
> + if (is_fieldnames_ready)
> + parse_names();
> + if (is_fields_ready &&
> + connect_indexes_and_fields(error))
> + return TRUE;
> + }
> + if (is_fields_allocated && final_fields_keys_allocation())
> + {
> + error= 4;
> + return TRUE;
> + }
> + return FALSE;
> +}
> +
> +
> +/**
> + Add index header
> +
> + @param name Name of the index (could be NULL if it is
> + already copied by add_index_names_as_onestring())
> + @param name_len Length of the index name (used only if name passed)
> + @param flags Key flags
> + @param length Length of the key (in its buffer)
> + @param parts Number of parts in the index
> + @param alg Index algorithm
> + @param blk_size Index block size
> +
> + @retval pointer on the allocated index descriptor
> +*/
> +
> +KEY *TABLE_SHARE::add_index(char *name, uint name_len,
> + uint flags, uint length, uint parts,
> + enum ha_key_alg alg, uint blk_size)
> +{
> + DBUG_ASSERT(key < (key_info + keys));
> + DBUG_ASSERT(key->key_length == 0); // prevent redefinition
> + DBUG_ASSERT(is_keys_allocated);
> + key->flags= flags;
> + key->key_length= length;
isn't key_length equal to the sum of keypart lengths?
> + key->key_parts= parts;
> + key->algorithm= alg;
> + key->block_size= blk_size;
> + if (name)
> + {
> + /*
> + TODO: should be check bounds of the allocated string?
> +
> + Check for illegal separating characters
> + */
> + DBUG_ASSERT(memchr(name, '\0', name_len) == NULL);
> + DBUG_ASSERT(memchr(name, NAMES_SEP_CHAR, name_len) == NULL);
> +
> + if (key == key_info)
> + *(keyname++)= NAMES_SEP_CHAR;
why is that?
> +
> + memcpy(keyname, name, name_len);
> + keyname+= name_len;
> + *(keyname++)= NAMES_SEP_CHAR;
please, don't do that. Don't ask the caller for the total length of all
key names, and don't allocate them in one alloc_root.
just set the pointer here: key->name= name.
the caller can allocate them in one array, or independently, or whatever -
don't bother enforcing anything specific here.
> +
> + if (key == key_info + (keys - 1))
> + {
> + is_keynames_ready= TRUE;
> + if (is_fieldnames_ready)
> + parse_names();
> + }
> +
> + /* names should be all or none */
> + DBUG_ASSERT(key == key_info || (key - 1)->name != NULL);
> + }
> + else
> + {
> + /* names should be all or none */
> + DBUG_ASSERT(key == key_info || (key - 1)->name == NULL);
> + }
> + return key++;
> +}
> +
> +
> +/**
> + Add part description to the index
> +
> + @param keyinfo The index where to add part
> + @param fieldnr Number of the field used in the index
> + @param offset Offset of the part in the key biffer
do you need that? The keypart starts where the previous one ends, doesn't it?
> + @param keytype Key type
> + @param length Length of the part
> + @param flags Flags of the part
> + @param error Error code assigned here in case of error
return the error instead
> +
> + @retval FALSE OK
> + @retval TRUE Error (error assigned)
> +*/
> +
> +bool TABLE_SHARE::add_index_part(KEY *keyinfo, uint16 fieldnr,
> + uint offset, uint16 keytype,
> + uint16 length, uint16 flags,
> + int &error)
> +{
> + if (!keyinfo->key_part)
> + {
> + // first key part for the key
> + keyinfo->key_part= key_part;
> + keyinfo->rec_per_key= rec_per_key;
this can be done as
key[i].rec_per_key=key[i-1].rec_per_key+key[i-1].key_parts
not in add_index_part() but later, in something like
finalize_share() - function that is called at the end. where you fix
all typelibs and stuff.
> + }
> + *rec_per_key++= 0;
> + key_part->fieldnr= fieldnr;
> + key_part->offset= offset;
> + key_part->key_type= keytype;
> + key_part->length= length;
> + key_part->key_part_flag= flags;
> + key_part->store_length= key_part->length; // will be fixed
> + key_part++;
> +
> + if (key_part == first_key_part + key_parts)
> + {
> + is_keyparts_ready= TRUE;
> + if (is_fields_ready &&
> + connect_indexes_and_fields(error))
> + return TRUE;
> + }
you don't need any is_fields_ready, etc
move all that code to finalize_share()
and let the caller to call it at the end, when all fields and keys
are parsed. No need to try to detect this moment automatically
with various cursors.
> + return FALSE;
> +}
> +
> +/**
> + Allocate space for field information
> +
> + @param fields_count Number of the fields
> + @param null_flds Number of the fields which could be NULL
> + @param enum_set_count Number of fields which are ENUM or SET
> + @param enum_set_values_count Sum of number of values of all fields which
> + are ENUM or SET
> + @param field_names_length Sum length of all fields names (string ends are not
> + counted)
don't ask for this (see above)
> + @param enum_set_names_length Sum length of all values of all fields which
> + are ENUM or SET (string ends are not counted)
> + @param com_length Sum ength of the all field comments
nor for this, nor enum_set_count, nor enum_set_values_count, nor
enum_set_names_length
> + @param handler_file handler - used to check fields/indices features
> + @param tbl_com_length Length of the table comment (if present) or 0
> + @param tbl_com reference on the table comment (if present)
move table comment related parameters somewhere
out of "allocate_fields_space" method.
> +
> + @retval FALSE OK
> + @retval TRUE Error (error assigned)
> +*/
> +
> +bool TABLE_SHARE::allocate_fields_space(uint fields_count,
> + uint null_flds,
> + uint enum_set_count,
> + uint enum_set_values_count,
> + uint field_names_length,
> + uint enum_set_names_length,
> + uint com_length,
> + handler *handler_file,
> + size_t tbl_com_length,
> + char *tbl_com)
> +{
> + stored_fields= fields= fields_count;
> + vfields= 0;
> + tmp_handler_file= handler_file;
> + null_fields= null_flds;
> + enum_set_values= enum_set_values_count;
> + enums_sets= enum_set_count;
> + field_names_length+= fields_count + 1; //add space for separators
> + comments_length= com_length;
> + /* add space for separators between names and intervals */
> + enum_set_names_length+= enum_set_values_count + enum_set_count * 2;
> + if (!(field_ptr= field = (Field **)
> + alloc_root(&mem_root,
> + (uint) ((fields_count + 1) * sizeof(Field*) +
> + enum_set_count * sizeof(TYPELIB) +
> + field_names_length + enum_set_names_length +
> + comments_length))))
> + return TRUE; /* purecov: inspected */
> +
> + field_ptr[fields]= 0; // End marker
> + curr_interval= intervals= (TYPELIB*) (field + fields + 1);
> + names= (char *) (intervals + enum_set_count);
> + curr_interval_names= names + field_names_length;
> + first_comment= comment_pos=
> + (char *) (names + field_names_length + enum_set_names_length);
> + if (!enum_set_count)
> + intervals= 0; // For better debugging
> + is_fields_allocated= TRUE;
> + if (is_keys_allocated && final_fields_keys_allocation())
> + return TRUE;
> + comment.str= strmake_root(&mem_root, tbl_com,
> + (comment.length= tbl_com_length));
> + /* init counters */
> + null_bits_are_used= (null_fields != 0);
> + if (null_field_first)
> + {
> + null_flags= null_pos= (uchar*) default_values;
> + null_bit_pos= (db_create_options & HA_OPTION_PACK_RECORD) ? 0 : 1;
> + /*
> + null_bytes below is only correct under the condition that
> + there are no bit fields. Correct values is set below after the
> + table struct is initialized
> + */
> + null_bytes= (null_fields + null_bit_pos + 7) / 8;
> + }
> +#ifndef WE_WANT_TO_SUPPORT_VERY_OLD_FRM_FILES
> + else
> + {
> + null_bytes= (null_fields + 7) / 8;
> + null_flags= null_pos= (uchar*) (default_values + reclength - null_bytes);
> + null_bit_pos= 0;
> + }
> +#endif
we don't want to support anything but the latest frm version here.
all compatibility hacks belong to open_binary_frm directly.
> + use_hash= fields >= MAX_FIELDS_BEFORE_HASH;
> + if (use_hash)
> + use_hash= !hash_init(&name_hash,
> + system_charset_info,
> + fields, 0, 0,
> + (hash_get_key) get_field_name, 0, 0);
> +
> + return FALSE;
> +}
> +
> +bool TABLE_SHARE::final_fields_keys_allocation()
see above - move this and parse_names and the likes into
one function, that the caller will call at the end.
don't try to detect when "the end" is.
> +{
> + DBUG_ASSERT(is_fields_allocated && is_keys_allocated);
> + if (!(interval_array=
> + (const char **) alloc_root(&mem_root,
> + (uint) ((fields + enum_set_values +
> + keys + 3) * sizeof(char *)))))
> + return TRUE;
> + curr_interval_array= interval_array + fields + 1;
> + return FALSE;
> +}
> +
> +/**
> + Internal function triggeren when all names
> + (indices, fields, enum and set) are allocated and copied
> +
> +
> + @retval FALSE OK
> + @retval TRUE Error
> +*/
> +
> +bool TABLE_SHARE::parse_names()
> +{
> + TYPELIB *interval;
> + DBUG_ASSERT(is_keys_allocated && is_fields_allocated &&
> + is_keynames_ready);
> + fix_type_pointers(&interval_array, &fieldnames, 1, &names);
> + if (is_fieldnames_ready)
> + fix_type_pointers(&interval_array, intervals, enums_sets, &names);
> + else
> + {
> + /* Intervals were passed to the add_field without preparation */
> + names= curr_interval_names;
> + interval_array= curr_interval_array;
> + is_fieldnames_ready= TRUE;
> + }
> + if ((keyname= first_keyname))
> + fix_type_pointers(&interval_array, &keynames, 1, &keyname);
> + if (fieldnames.count != fields)
> + return TRUE;
> +
> + /* Set ENUM and SET lengths */
> + for (interval= intervals;
> + interval < intervals + enums_sets;
> + interval++)
> + {
> + uint count= (uint) (interval->count + 1) * sizeof(uint);
> + if (!(interval->type_lengths= (uint *) alloc_root(&mem_root, count)))
> + return TRUE;
> + for (count= 0; count < interval->count; count++)
> + {
> + char *val= (char*) interval->type_names[count];
> + interval->type_lengths[count]= strlen(val);
> + }
> + interval->type_lengths[count]= 0;
> + }
> +
> + return FALSE;
> +}
> +
> +/**
> + Add field description to the TABLE_SHARE
> +
> + @param name Field name
> + @param pos Field position in the record buffer
isn't it equal the end of the previous field?
(with special treatment for BIT)
> + @param field_length Length of the field
> + @param pack_flag Flags of the field
what's that?
I mean, I understand that it's some set of cryptic bits, but how would an
engine use that?
You know what, I think we're missing a usage example.
could you add it to the example engine (ha_example) support for discovery?
Something like that - a table with a fixed name, say, "example_discovered_table"
is created by discovery() on the example engine. That is, a fixed table name,
keep it as simple as possible. Fixed table structure.
Few fields, say, an int, primary key, and enum (Y,N) - or whatever.
because you haven't yet done the step 2 in our plan (extracting that part
of ALTER TABLE that converts TABLE_SHARE into Create_field), this
example discovery won't create frm files - it'll be completely in memory.
which is fine, for now.
> + @param field_type Field type
> + @param charset Caracter set infoemation of the field
> + @param geom_type Geometry data type (for geometry fields)
> + @param auto_increment Is the field auto increment
> + @param enum_set Definition of ENUM or SET (for appropriate type)
a complete typelib? probably it's an overkill. better to pass an array
of names, and you can build a typelib internally.
> + @param comm_pos Reference on the field commentary
> + @param comment_length Length of the field commentary
> + @param error Error code assigned here in case of error
as always, you can return the error code
> +
> + @retval FALSE OK
> + @retval TRUE Error (error assigned)
> +*/
> +
> +Field **TABLE_SHARE::add_field(const char *name,
> + uchar *pos,
> + uint32 field_length,
> + uint pack_flag,
> + enum_field_types field_type,
> + CHARSET_INFO *charset,
> + geometry_type geom_type,
> + bool auto_increment,
> + TYPELIB* enum_set,
> + char *comm_pos,
> + uint comment_length,
> + int &error)
> +{
> + DBUG_ASSERT(is_fields_allocated);
> + utype unireg_type= get_unireg_type(auto_increment,
> + !f_no_default(pack_flag),
> + FALSE,
> + FALSE, f_maybe_null(pack_flag),
> + field_type);
> +
> + return add_field_int(name, pos, field_length,
> + pack_flag, field_type,
> + charset, geom_type, unireg_type,
> + enum_set, comm_pos, comment_length,
> + NULL, TRUE, error);
> +}
> +
> +
> +/**
> + Add field description to the TABLE_SHARE (internal)
> +
> + @param name Field name
> + @param pos Field position in the record buffer
> + @param field_length Length of the field
> + @param pack_flag Flags of the field
> + @param field_type Field type
> + @param charset Caracter set infoemation of the field
> + @param geom_type Geometry data type (for geometry fields)
> + @param unireg_type Unireg type (or check flags)
> + @param auto_increment Is the field auto increment
> + @param enum_set Definition of ENUM or SET (for appropriate type)
> + @param comm_pos Reference on the field commentary
> + @param comment_length Length of the field commentary
> + @param vcol_info Virtual column information
> + @param fld_stored_in_db TRUE for normal fields and persistent virtual fields
> + @param error Error code assigned here in case of error
> +
> + @retval FALSE OK
> + @retval TRUE Error (error assigned)
> +*/
> +
> +Field **TABLE_SHARE::add_field_int(const char *name,
> + uchar *pos,
> + uint32 field_length,
> + uint pack_flag,
> + enum_field_types field_type,
> + CHARSET_INFO *charset,
> + geometry_type geom_type,
> + utype unireg_type,
> + TYPELIB* enum_set,
> + char *comm_pos,
> + uint comment_length,
> + Virtual_column_info *vcol_info,
> + bool fld_stored_in_db,
> + int &error)
> +{
> + Field* reg_field;
> + DBUG_ASSERT(is_fields_allocated);
> + if (!is_fieldnames_ready)
> + {
> + if (enum_set)
> + {
> + copy_typelib(curr_interval,
> + &curr_interval_array, enum_set,
> + &curr_interval_names, NAMES_SEP_CHAR);
See? it's better to get an array of names from the caller and build the
typelib here - as you won't use the typelib that caller provided anyway
> + enum_set= curr_interval++;
> + }
> + {
> + uchar *tmp= (uchar*) strmov((char*) *curr_field_names, name);
> + *tmp++= (uchar)NAMES_SEP_CHAR;
> + *tmp= 0;
> + name= curr_field_names;
> + curr_field_names= (char*)tmp;
> + }
just like with other names - let the caller handle the allocations,
you simply use the pointer that the caller provides.
> + }
> + reg_field = *field_ptr=
> + make_field(this, pos,
> + (uint32) field_length,
> + null_pos, null_bit_pos,
> + pack_flag,
> + field_type,
> + charset,
> + geom_type,
> + (utype) MTYP_TYPENR(unireg_type),
add the cast to MTYP_TYPENR definition and remove it here (and elsewhere)
> + enum_set,
> + name);
> +
> + if (!reg_field) // Not supported field type
> + {
> + error= 4;
> + return NULL;
> + }
> + reg_field->field_index= (field_ptr - field);
> + reg_field->vcol_info= vcol_info;
> + reg_field->stored_in_db= fld_stored_in_db;
> + DBUG_ASSERT(reg_field->field_index < fields);
> + if (comm_pos && comment_length)
> + {
> + memcpy(comment_pos, comm_pos, comment_length);
same here and everywhere. I'll stop repeating "just use the pointer,
let the caller handle the allocation"
alternatively (here and everywhere) do alloc_root() - that is
the caller gives you a pointer, and you call alloc_root() anc copy
the data. Yes, it means that every string will be allocated in its
own chunk. And in the _int version of the function you use the pointers.
I mean something like that:
TABLE_SHARE::add_field(char *name, ...)
{
...
return add_field_int(strdup_root(name), ... );
so, if one uses add_field, he does not need to think about memory
allocations, and does not need to calculate the sum of lengths,
and stuff like that.
while open_binary_frm() will use add_field_int() can will pass the pointers
directly into the big allocated chunk, as always.
The above applies to all strings, comments, enum/set, everything.
> + DBUG_ASSERT(!is_comments_ready);
> + }
> + else
> + DBUG_ASSERT(is_comments_ready || comment_length == 0);
> + if (comment_length)
> + {
> + reg_field->comment.str= comment_pos;
> + reg_field->comment.length= comment_length;
> + comment_pos+= comment_length;
right, that's what I mean by "use the pointer directly, let the caller
to handle the allocations".
> + }
> + else
> + {
> + reg_field->comment.str= (char *)"";
> + reg_field->comment.length= 0;
very minor stilistic details:
in these cases I prefer to use
reg_field->comment= empty_lex_string;
it was introduced for exactly this purpose.
> + }
> + if (f_no_default(pack_flag))
> + reg_field->flags|= NO_DEFAULT_VALUE_FLAG;
> +
> + if (reg_field->unireg_check == UT_NEXT_NUMBER)
> + found_next_number_field= field_ptr;
> + if (timestamp_field == reg_field)
> + timestamp_field_offset= reg_field->field_index;
> +
> + if (use_hash)
> + {
> + if (my_hash_insert(&name_hash,
> + (uchar*) field_ptr))
> + {
> + /*
> + Set return code 8 here to indicate that an error has
> + occurred but that the error message already has been
> + sent (OOM).
> + */
> + error= 8;
> + return NULL;
> + }
> + }
> +
> + /* move null bit/byte pointers */
> + if (field_type == MYSQL_TYPE_BIT && !f_bit_as_char(pack_flag))
> + {
> + null_bits_are_used= 1;
> + if ((null_bit_pos+= field_length & 7) > 7)
> + {
> + null_pos++;
> + null_bit_pos-= 8;
> + }
> + }
> + if (!(reg_field->flags & NOT_NULL_FLAG))
> + {
> + if (!(null_bit_pos= (null_bit_pos + 1) & 7))
> + null_pos++;
> + }
> +
> + if (reg_field->field_index == fields - 1)
> + {
> + if (!is_fieldnames_ready)
> +
> + is_fields_ready= TRUE;
> + if (is_keyparts_ready &&
> + connect_indexes_and_fields(error))
> + return NULL;
> + }
> + return field_ptr++;
> +}
> +
> +/**
> + Internal function which connect fields and indices (its parts) triggered
> + when fields and index parts are ready
> +
> + @param error Error code assigned here in case of error
> +
> + @retval FALSE OK
> + @retval TRUE Error (error assigned)
> +*/
> +
> +bool TABLE_SHARE::connect_indexes_and_fields(int &error)
> +{
> + KEY *keyinfo;
> + uint i;
> + /* Fix key->name and key_part->field */
> + if (key_parts)
> + {
> + KEY_PART_INFO *key_part= first_key_part;
> + uint prm_key=(uint) (find_type((char*) primary_key_name,
> + &keynames, 3) - 1);
> + longlong ha_option= tmp_handler_file->ha_table_flags();
> + keyinfo= key_info;
> +
> + for (uint key=0 ; key < keys ; key++, keyinfo++)
> + {
> + uint usable_parts= 0;
> + keyinfo->name=(char*) keynames.type_names[key];
> + keyinfo->name_length= strlen(keyinfo->name);
> + keyinfo->cache_name=
> + (uchar*) alloc_root(&mem_root,
> + table_cache_key.length+
> + keyinfo->name_length + 1);
> + if (keyinfo->cache_name) // If not out of memory
> + {
> + uchar *pos= keyinfo->cache_name;
> + memcpy(pos, table_cache_key.str, table_cache_key.length);
> + memcpy(pos + table_cache_key.length, keyinfo->name,
> + keyinfo->name_length+1);
> + }
> +
> + /* Fix fulltext keys for old .frm files */
> + if (key_info[key].flags & HA_FULLTEXT)
> + key_info[key].algorithm= HA_KEY_ALG_FULLTEXT;
> +
> + if (prm_key >= MAX_KEY && (keyinfo->flags & HA_NOSAME))
> + {
> + /*
> + If the UNIQUE key doesn't have NULL columns and is not a part key
> + declare this as a primary key.
> + */
> + prm_key= key;
> + for (i= 0 ; i < keyinfo->key_parts; i++)
> + {
> + uint fieldnr= key_part[i].fieldnr;
> + if (!fieldnr ||
> + field[fieldnr-1]->null_ptr ||
> + field[fieldnr-1]->key_length() !=
> + key_part[i].length)
> + {
> + prm_key=MAX_KEY; // Can't be used
> + break;
> + }
> + }
> + }
> +
> + for (i=0 ; i < keyinfo->key_parts ; key_part++,i++)
> + {
> + Field *fld;
> + if (new_field_pack_flag <= 1)
> + key_part->fieldnr= (uint16) find_field(field,
> + default_values,
> + (uint) key_part->offset,
> + (uint) key_part->length);
> + if (!key_part->fieldnr)
> + {
> + error= 4; // Wrong file
> + return TRUE;
> + }
> + fld= key_part->field= field[key_part->fieldnr-1];
> + key_part->type= fld->key_type();
> + if (fld->null_ptr)
> + {
> + key_part->null_offset=(uint) ((uchar*) fld->null_ptr -
> + default_values);
> + key_part->null_bit= fld->null_bit;
> + key_part->store_length+=HA_KEY_NULL_LENGTH;
> + keyinfo->flags|=HA_NULL_PART_KEY;
> + keyinfo->key_length+= HA_KEY_NULL_LENGTH;
> + }
> + if (fld->type() == MYSQL_TYPE_BLOB ||
> + fld->real_type() == MYSQL_TYPE_VARCHAR ||
> + fld->type() == MYSQL_TYPE_GEOMETRY)
> + {
> + if (fld->type() == MYSQL_TYPE_BLOB ||
> + fld->type() == MYSQL_TYPE_GEOMETRY)
> + key_part->key_part_flag|= HA_BLOB_PART;
> + else
> + key_part->key_part_flag|= HA_VAR_LENGTH_PART;
> + key_part->store_length+=HA_KEY_BLOB_LENGTH;
> + keyinfo->key_length+= HA_KEY_BLOB_LENGTH;
> + }
> + if (fld->type() == MYSQL_TYPE_BIT)
> + key_part->key_part_flag|= HA_BIT_PART;
> +
> + if (i == 0 && key != prm_key)
> + fld->flags |= (((keyinfo->flags & HA_NOSAME) &&
> + (keyinfo->key_parts == 1)) ?
> + UNIQUE_KEY_FLAG : MULTIPLE_KEY_FLAG);
> + if (i == 0)
> + fld->key_start.set_bit(key);
> + if (fld->key_length() == key_part->length &&
> + !(fld->flags & BLOB_FLAG))
> + {
> + if (tmp_handler_file->index_flags(key, i, 0) & HA_KEYREAD_ONLY)
> + {
> + keys_for_keyread.set_bit(key);
> + fld->part_of_key.set_bit(key);
> + fld->part_of_key_not_clustered.set_bit(key);
> + }
> + if (tmp_handler_file->index_flags(key, i, 1) & HA_READ_ORDER)
> + fld->part_of_sortkey.set_bit(key);
> + }
> + if (!(key_part->key_part_flag & HA_REVERSE_SORT) &&
> + usable_parts == i)
> + usable_parts++; // For FILESORT
> + fld->flags|= PART_KEY_FLAG;
> + if (key == prm_key)
> + {
> + fld->flags|= PRI_KEY_FLAG;
> + /*
> + If this field is part of the primary key and all keys contains
> + the primary key, then we can use any key to find this column
> + */
> + if (ha_option & HA_PRIMARY_KEY_IN_READ_INDEX)
> + {
> + if (fld->key_length() == key_part->length &&
> + !(fld->flags & BLOB_FLAG))
> + fld->part_of_key= keys_in_use;
> + if (fld->part_of_sortkey.is_set(key))
> + fld->part_of_sortkey= keys_in_use;
> + }
> + }
> + if (fld->key_length() != key_part->length)
> + {
> + key_part->key_part_flag|= HA_PART_KEY_SEG;
> + }
> + if (fld->real_maybe_null())
> + key_part->key_part_flag|= HA_NULL_PART;
> + /*
> + Sometimes we can compare key parts for equality with memcmp.
> + But not always.
> + */
> + if (!(key_part->key_part_flag & (HA_BLOB_PART | HA_VAR_LENGTH_PART |
> + HA_BIT_PART)) &&
> + key_part->type != HA_KEYTYPE_FLOAT &&
> + key_part->type == HA_KEYTYPE_DOUBLE)
> + key_part->key_part_flag|= HA_CAN_MEMCMP;
> + }
> + keyinfo->usable_key_parts= usable_parts; // Filesort
> +
> + set_if_bigger(max_key_length,keyinfo->key_length+
> + keyinfo->key_parts);
> + total_key_length+= keyinfo->key_length;
> + /*
> + MERGE tables do not have unique indexes. But every key could be
> + an unique index on the underlying MyISAM table. (Bug #10400)
> + */
> + if ((keyinfo->flags & HA_NOSAME) ||
> + (ha_option & HA_ANY_INDEX_MAY_BE_UNIQUE))
> + set_if_bigger(max_unique_length,keyinfo->key_length);
> + }
> + if (prm_key < MAX_KEY &&
> + (keys_in_use.is_set(prm_key)))
> + {
> + primary_key= prm_key;
> + /*
> + If we are using an integer as the primary key then allow the user to
> + refer to it as '_rowid'
> + */
> + if (key_info[prm_key].key_parts == 1)
> + {
> + Field *fld= key_info[prm_key].key_part[0].field;
> + if (fld && fld->result_type() == INT_RESULT)
> + {
> + /* note that fieldnr here (and rowid_field_offset) starts from 1 */
> + rowid_field_offset= (key_info[prm_key].key_part[0].
> + fieldnr);
> + }
> + }
> + }
> + else
> + primary_key = MAX_KEY; // we do not have a primary key
> + }
> + else
> + primary_key= MAX_KEY;
> +
> + return FALSE;
> +}
> +
> /*
> Read data from a binary .frm file from MySQL 3.23 - 5.0 into TABLE_SHARE
> */
> @@ -747,38 +1544,53 @@ static int open_binary_frm(THD *thd, TAB
> legacy_db_type < DB_TYPE_FIRST_DYNAMIC)
> share->db_plugin= ha_lock_engine(NULL,
> ha_checktype(thd, legacy_db_type, 0, 0));
> - share->db_create_options= db_create_options= uint2korr(head+30);
> - share->db_options_in_use= share->db_create_options;
> - share->mysql_version= uint4korr(head+51);
> - share->null_field_first= 0;
> - if (!head[32]) // New frm file in 3.23
> - {
> - share->avg_row_length= uint4korr(head+34);
> - share->transactional= (ha_choice) (head[39] & 3);
> - share->page_checksum= (ha_choice) ((head[39] >> 2) & 3);
> - share->row_type= (row_type) head[40];
> - share->table_charset= get_charset((uint) head[38],MYF(0));
> - share->null_field_first= 1;
> - }
> - if (!share->table_charset)
> - {
> - /* unknown charset in head[38] or pre-3.23 frm */
> - if (use_mb(default_charset_info))
> - {
> - /* Warn that we may be changing the size of character columns */
> - sql_print_warning("'%s' had no or invalid character set, "
> - "and default character set is multi-byte, "
> - "so character column sizes may have changed",
> - share->path.str);
> - }
> - share->table_charset= default_charset_info;
> +
> +
> + {
> + enum ha_choice transact= HA_CHOICE_UNDEF;
> + enum ha_choice page_chcksum= HA_CHOICE_UNDEF;
> + enum row_type rtype= ROW_TYPE_DEFAULT;
> + CHARSET_INFO *charset= NULL;
> + bool null_fld_first= 0;
> + ulong avg_row_len= 0;
> + if (!head[32]) // New frm file in 3.23
> + {
> + avg_row_len= uint4korr(head+34);
> + transact= (ha_choice) (head[39] & 3);
> + page_chcksum= (ha_choice) ((head[39] >> 2) & 3);
> + rtype= (row_type) head[40];
> + charset= get_charset((uint) head[38],MYF(0));
> + null_fld_first= 1;
> + }
> + if (!charset)
> + {
> + /* unknown charset in head[38] or pre-3.23 frm */
> + if (use_mb(default_charset_info))
> + {
> + /* Warn that we may be changing the size of character columns */
> + sql_print_warning("'%s' had no or invalid character set, "
> + "and default character set is multi-byte, "
> + "so character column sizes may have changed",
> + share->path.str);
> + }
> + charset= default_charset_info;
> + }
> + if (share->
> + set_init_parameters(frm_ver,
> + uint4korr(head + 51), /*MySQL Version of the frm*/
> + uint2korr(head + 30), /*Create options (old)*/
> + avg_row_len, transact, page_chcksum,
> + rtype, charset, null_fld_first,
> + uint4korr(head+22), /*MIN_ROWS*/
> + uint4korr(head+18), /*MAX_ROWS*/
> + uint2korr(head+16), /*record length*/
> + uint2korr(head+59), /*extra_rec_buf_length*/
> + uint2korr(head+62), /*key block size*/
> + new_field_pack_flag,
> + error))
I'd say you can remove this method completely - it's just a set of
assignments, the caller can do them directly.
better try to provide reasonable defaults (in share->init()),
so that the caller would not need to assign most of those.
> + goto free_and_err;
> }
> - share->db_record_offset= 1;
> - if (db_create_options & HA_OPTION_LONG_BLOB_PTR)
> - share->blob_ptr_size= portable_sizeof_char_ptr;
> error=4;
> - share->max_rows= uint4korr(head+18);
> - share->min_rows= uint4korr(head+22);
>
> /* Read keyinformation */
> key_info_length= (uint) uint2korr(head+28);
> @@ -787,84 +1599,85 @@ static int open_binary_frm(THD *thd, TAB
> goto err; /* purecov: inspected */
> if (disk_buff[0] & 0x80)
> {
> - share->keys= keys= (disk_buff[1] << 7) | (disk_buff[0] & 0x7f);
> - share->key_parts= key_parts= uint2korr(disk_buff+2);
> + keys= (disk_buff[1] << 7) | (disk_buff[0] & 0x7f);
> + key_parts= uint2korr(disk_buff+2);
> }
> else
> {
> - share->keys= keys= disk_buff[0];
> - share->key_parts= key_parts= disk_buff[1];
> + keys= disk_buff[0];
> + key_parts= disk_buff[1];
> }
> - share->keys_for_keyread.init(0);
> - share->keys_in_use.init(keys);
> -
> - n_length=keys*sizeof(KEY)+key_parts*sizeof(KEY_PART_INFO);
> - if (!(keyinfo = (KEY*) alloc_root(&share->mem_root,
> - n_length + uint2korr(disk_buff+4))))
> - goto err; /* purecov: inspected */
> - bzero((char*) keyinfo,n_length);
> - share->key_info= keyinfo;
> - key_part= my_reinterpret_cast(KEY_PART_INFO*) (keyinfo+keys);
> - strpos=disk_buff+6;
> -
> - if (!(rec_per_key= (ulong*) alloc_root(&share->mem_root,
> - sizeof(ulong)*key_parts)))
> + if (share->allocate_indices_space(keys, key_parts,
> + uint2korr(disk_buff + 4) - keys - 1,
> + error))
> goto err;
> + strpos=disk_buff+6;
>
> - for (i=0 ; i < keys ; i++, keyinfo++)
> + for (i=0 ; i < keys ; i++)
> {
> if (new_frm_ver >= 3)
> {
> - keyinfo->flags= (uint) uint2korr(strpos) ^ HA_NOSAME;
> - keyinfo->key_length= (uint) uint2korr(strpos+2);
> - keyinfo->key_parts= (uint) strpos[4];
> - keyinfo->algorithm= (enum ha_key_alg) strpos[5];
> - keyinfo->block_size= uint2korr(strpos+6);
> + keyinfo= share->add_index(NULL, 0,
> + (uint) uint2korr(strpos) ^ HA_NOSAME,
> + (uint) uint2korr(strpos + 2),
> + (uint) strpos[4], (enum ha_key_alg) strpos[5],
> + uint2korr(strpos + 6));
make block_size an optional parameter, or let the caller set it directly
> strpos+=8;
> }
> else
> {
> - keyinfo->flags= ((uint) strpos[0]) ^ HA_NOSAME;
> - keyinfo->key_length= (uint) uint2korr(strpos+1);
> - keyinfo->key_parts= (uint) strpos[3];
> - keyinfo->algorithm= HA_KEY_ALG_UNDEF;
> + keyinfo= share->add_index(NULL, 0,
> + ((uint) strpos[0]) ^ HA_NOSAME,
> + (uint) uint2korr(strpos + 1),
> + (uint) strpos[3],
> + HA_KEY_ALG_UNDEF, 0);
> strpos+=4;
> }
>
> - keyinfo->key_part= key_part;
> - keyinfo->rec_per_key= rec_per_key;
> - for (j=keyinfo->key_parts ; j-- ; key_part++)
> + for (j=keyinfo->key_parts ; j-- ; )
> {
> - *rec_per_key++=0;
> - key_part->fieldnr= (uint16) (uint2korr(strpos) & FIELD_NR_MASK);
> - key_part->offset= (uint) uint2korr(strpos+2)-1;
> - key_part->key_type= (uint) uint2korr(strpos+5);
> - // key_part->field= (Field*) 0; // Will be fixed later
> if (new_frm_ver >= 1)
> {
> - key_part->key_part_flag= *(strpos+4);
> - key_part->length= (uint) uint2korr(strpos+7);
> + if (share->add_index_part(keyinfo,
> + (uint16)
> + (uint2korr(strpos) & FIELD_NR_MASK),
> + (uint) uint2korr(strpos + 2) - 1,
> + (uint) uint2korr(strpos + 5),
> + (uint) uint2korr(strpos + 7),
> + *(strpos + 4),
> + error))
> + {
> + DBUG_ASSERT(0); // it is not possible if keys defined first
> + }
> strpos+=9;
> }
> else
> {
> - key_part->length= *(strpos+4);
> - key_part->key_part_flag=0;
> - if (key_part->length > 128)
> + uint16 length= *(strpos+4);
> + uint16 flags= 0;
> + if (length > 128)
> {
> - key_part->length&=127; /* purecov: inspected */
> - key_part->key_part_flag=HA_REVERSE_SORT; /* purecov: inspected */
> + length&= 127; /* purecov: inspected */
> + flags= HA_REVERSE_SORT; /* purecov: inspected */
> }
> + if (share->add_index_part(keyinfo,
> + (uint16)
> + (uint2korr(strpos) & FIELD_NR_MASK),
> + (uint) uint2korr(strpos+2)-1,
> + (uint) uint2korr(strpos+5),
> + length, flags, error))
> + {
> + DBUG_ASSERT(0); // it is not possible if keys defined first
> + }
> strpos+=7;
> }
> - key_part->store_length=key_part->length;
> }
> }
> - keynames=(char*) key_part;
> - strpos+= (strmov(keynames, (char *) strpos) - keynames)+1;
>
> - share->reclength = uint2korr((head+16));
> - share->stored_rec_length= share->reclength;
> + if (share->add_index_names_as_onestring(strpos, i))
> + goto err;
No, please remove this method, let the caller do strmov as before.
it's a weird thing that open_binary_frm is doing, there's no need to
create an api for that, just keep it the old way.
To repeat: we want to have a reasonable and sane API for storage engines to
use, not an API that allows to do everything that frm format was
historically forcing us to do in openfrm().
> + strpos+= i;
> +
> if (*(head+26) == 1)
> share->system= 1; /* one-record-database */
> #ifdef HAVE_CRYPTED_FRM
> @@ -1034,17 +1847,10 @@ static int open_binary_frm(THD *thd, TAB
> }
> DBUG_ASSERT(next_chunk <= buff_end);
> }
> - share->key_block_size= uint2korr(head+62);
>
> error=4;
> - extra_rec_buf_length= uint2korr(head+59);
> - rec_buff_length= ALIGN_SIZE(share->reclength + 1 + extra_rec_buf_length);
> - share->rec_buff_length= rec_buff_length;
> - if (!(record= (uchar *) alloc_root(&share->mem_root,
> - rec_buff_length)))
> - goto free_and_err; /* purecov: inspected */
> - share->default_values= record;
> - if (my_pread(file, record, (size_t) share->reclength,
> + /* TODO: make copy function for handler */
what does this comment mean?
> + if (my_pread(file, share->default_values, (size_t) share->reclength,
> record_offset, MYF(MY_NABP)))
> goto free_and_err; /* purecov: inspected */
>
> @@ -2785,6 +3271,27 @@ fix_type_pointers(const char ***array, T
> } /* fix_type_pointers */
>
>
> +static void
> +copy_typelib(TYPELIB *type_copy,
> + const char ***array, TYPELIB *point_to_type,
> + char **names, char chr)
> +{
> + *((*names)++)= chr; // full compatibility, just to be safe
> + type_copy->name= 0;
> + type_copy->type_names= *array;
> + type_copy->count= point_to_type->count;
> + for (uint i= 0; i < point_to_type->count; i++)
> + {
> + *((*array)++) = *names;
> + uchar *tmp= (uchar*) strmov((char*) *names, type_copy->type_names[i]);
> + *tmp++= (uchar)0;
> + *names= (char *)tmp;
> + }
> + *((*array)++)= NullS; /* End of type */
> + *((*names)++)= 0;
> +}
> +
> +
> TYPELIB *typelib(MEM_ROOT *mem_root, List<String> &strings)
> {
> TYPELIB *result= (TYPELIB*) alloc_root(mem_root, sizeof(TYPELIB));
I don't understand how you handle our new table/field/index options,
those that can be defined per storage engine
> === modified file 'sql/table.h'
> --- a/sql/table.h 2011-08-17 05:48:35 +0000
> +++ b/sql/table.h 2011-10-04 11:14:59 +0000
> @@ -484,6 +485,39 @@ typedef struct st_table_share
> void *ha_data;
> void (*ha_data_destroy)(void *); /* An optional destructor for ha_data. */
>
> + /*
> + Variables of building the TABLE_SHARE.
> +
> + TODO: it might have sense to make separate structure for this variables
> + and deallocate it after building
> + */
Agree!
It would be better to remove all this added functionality (all new methods
and variables) from the TABLE_SHARE and create a separate class
TABLE_SHARE_builder (or whatever), and have it allocated on the stack, like
int open_binary_frm( ... )
{
TABLE_SHARE_builder builder(share);
...
builder->add_field(...);
...
... etc ...
}
this looks like a cleaner approach - the builder and all its memory is
destroyed when the function returns. And the TABLE_SHARE itself
does not have the "building" methods - it always represents a complete
and frozen table structure.
> + KEY_PART_INFO *first_key_part; // pointer on parts array
> + KEY_PART_INFO *key_part; // current position during key parts adding
> + KEY *key; // current position during keys adding
> + ulong *rec_per_key;
> + const char **interval_array;
> + char *first_keyname; // pointer on the first index name string
> + char *keyname; // pointer on the current index name string during adding
> + char *names; // array of pointers to all names used in the definition
> + char *curr_interval_names;
> + TYPELIB *curr_interval;
> + const char **curr_interval_array;
> + char *first_comment, *comment_pos;
> + char *curr_field_names;
> + uchar *null_flags, *null_pos; //pointers to the NULL bits during field adding
> + /* Needed for fixing indices according to handler abilities and features */
> + handler *tmp_handler_file;
> + Field **field_ptr;
> + uint enum_set_values, enums_sets, comments_length;
> + uint new_field_pack_flag;
> + uchar null_bit_pos;
> + bool use_hash; /* use hash for field names */
> + bool null_bits_are_used;
> + /* Flags of the building progress (also used by triggers) */
> + bool is_keys_allocated, is_fields_allocated,
> + is_keynames_ready, is_fieldnames_ready, is_keyparts_ready,
> + is_comments_ready, is_fields_ready;
> + /* end of declaration of variables for building TABLE_SHARE*/
>
> /*
> Set share's table cache key and update its db and table name appropriately.
> @@ -652,6 +686,96 @@ typedef struct st_table_share
> return (tmp_table == SYSTEM_TMP_TABLE || is_view) ? 0 : table_map_id;
> }
>
> + /* Table_share building functions */
> + void init_metadata()
> + {
> + is_keys_allocated= is_fields_allocated=
> + is_keynames_ready= is_fieldnames_ready= is_comments_ready=
> + is_keyparts_ready= is_fields_ready= FALSE;
> + }
> + bool set_init_parameters(uchar frm_ver, ulong mysql_ver, uint db_create_opt,
> + ulong avg_row_len, enum ha_choice transact,
> + enum ha_choice page_chcksum, enum row_type rtype,
> + CHARSET_INFO *charset, bool null_fld_first,
> + ha_rows mi_rows, ha_rows ma_rows,
> + ulong reclen, uint extra_rec_buf_len,
> + uint key_blck_size, uint new_fld_pck_flg,
> + int &error);
> + bool allocate_indices_space(uint keys, uint parts, uint names_len,
> + int &error);
> + KEY *add_index(char *name, uint name_len,
> + uint flags, uint length, uint parts,
> + enum ha_key_alg alg, uint blk_size);
> + bool add_index_part(KEY *keyinfo, uint16 fieldnr,
> + uint offset, uint16 keytype,
> + uint16 length, uint16 flags, int &error);
> + inline bool add_index_names_as_onestring(uchar *pos, uint &len)
> + {
> + DBUG_ASSERT(is_keys_allocated);
> + DBUG_ASSERT(keyname == first_keyname); // add_key did not used names
> + len= (strmov(first_keyname, (char *) pos) - first_keyname) + 1;
> + is_keynames_ready= TRUE;
> + if (is_fieldnames_ready && parse_names())
> + return TRUE;
> + return FALSE;
> + }
> + inline bool add_field_enum_set_names_as_onestring(uchar *pos, uint len)
> + {
> + DBUG_ASSERT(is_fields_allocated);
> + memcpy((char*) names, pos, len);
> + is_fieldnames_ready= TRUE;
> + if (is_keynames_ready && parse_names())
> + return TRUE;
> + return FALSE;
> + }
> + inline void add_comments_as_one_string(const uchar *pos)
> + {
> + DBUG_ASSERT(is_fields_allocated);
> + memcpy(first_comment, pos, comments_length);
> + is_comments_ready= TRUE;
> + }
See above, I don't think that these *_onestring functions are necessary.
if you use the pointers are provided by the caller,
than open_binary_frm can parse these "one strings" as it wants
and pass pointers down. Thus keeping this trickery out of the api.
> +
> + bool allocate_fields_space(uint fields_count,
> + uint null_flds,
> + uint enum_set_count,
> + uint enum_set_values_count,
> + uint field_names_length,
> + uint enum_set_names_length,
> + uint com_length,
> + handler *handler_file,
> + size_t tbl_com_length= 0,
> + char *tbl_com= NULL);
> + Field **add_field(const char *name,
> + uchar *pos,
> + uint32 field_length,
> + uint pack_flag,
> + enum_field_types field_type,
> + CHARSET_INFO *charset,
> + geometry_type geom_type,
> + bool auto_increment,
> + TYPELIB* enum_set,
> + char *comm_pos,
> + uint comment_length,
> + int &error);
> + Field **add_field_int(const char *name,
> + uchar *pos,
> + uint32 field_length,
> + uint pack_flag,
> + enum_field_types field_type,
> + CHARSET_INFO *charset,
> + geometry_type geom_type,
> + utype unireg_type,
> + TYPELIB* enum_set,
> + char *comment_pos,
> + uint comment_length,
> + Virtual_column_info *vcol_info,
> + bool fld_stored_in_db,
> + int &error);
> +private:
> + bool final_fields_keys_allocation();
> + bool parse_names();
> + bool connect_indexes_and_fields(int &error);
> +
> } TABLE_SHARE;
>
Regards,
Sergei
