Hi, Alexey! First, please, please, *do not* send patches from Thunderbird. It took me a lot of time to fix it, the patch was badly garbled. May be broken formatting everywhere in the patch was also caused by this. Then, I didn't see any tests in the patch, not even syntax tests, but also not create field, store, read tests. Please add them. Now, the review:

diff --git a/sql/field.cc b/sql/field.cc --- a/sql/field.cc +++ b/sql/field.cc @@ -7808,6 +7808,35 @@ uint Field_blob::is_equal(Create_field *new_field)

#ifdef HAVE_SPATIAL

+uint gis_field_options_image(uchar *buff, List &create_fields) +{ + uint image_size= 0; + uint n_field= 0; + List_iterator it(create_fields); + Create_field *field; + while ((field= it++)) + { + if (field->sql_type != MYSQL_TYPE_GEOMETRY) + continue; + if (buff) + int2store(buff + image_size, ((uint16) n_field)); + image_size+= 2; + if (buff) + int2store(buff + image_size, ((uint16) field->length)); + image_size+= 2; + if (buff) + int2store(buff + image_size, ((uint16) field->decimals)); + image_size+= 2; + if (buff) + int4store(buff + image_size, ((uint32) field->srid)); + image_size+= 4; + n_field++; + } + + return image_size; +}

Hmm... So, you basically, put an array of properties in frm, under EXTRA2_GIS Two comments: 1. Why n_field, length and decimals? I thought you only add srid, other properties are already stored elsewhere, aren't they? 2. Really think about upgrades downgrades. The "extra" section in the frm doesn't handle them well, I know how these problems happen. Think, what happens when you add more properties here? What if you remove some? How new server will parse old frms, how old server will read new frms?

+ + void Field_geom::sql_type(String &res) const { CHARSET_INFO *cs= &my_charset_latin1; @@ -9181,7 +9210,7 @@ bool Create_field::init(THD *thd, char *fld_name, enum_field_types fld_type, Item *fld_on_update_value, LEX_STRING *fld_comment, char *fld_change, List<String> *fld_interval_list, CHARSET_INFO *fld_charset, uint fld_geom_type, - Virtual_column_info *fld_vcol_info, + char *fld_srid, Virtual_column_info *fld_vcol_info,

fix the formatting, please

engine_option_value *create_opt, bool check_exists) { uint sign_len, allowed_type_modifier= 0; @@ -9232,6 +9261,7 @@ bool Create_field::init(THD *thd, char *fld_name, enum_field_types fld_type, pack_length= key_length= 0; charset= fld_charset; geom_type= (Field::geometry_type) fld_geom_type; + srid= fld_srid ? (uint)atoi(fld_srid) : 0;

and here

interval_list.empty();

comment= *fld_comment; @@ -9635,7 +9665,7 @@ Field *make_field(TABLE_SHARE *share, uchar *ptr, uint32 field_length, uint pack_flag, enum_field_types field_type, CHARSET_INFO *field_charset, - Field::geometry_type geom_type, + Field::geometry_type geom_type, uint srid,

and here, and everywhere, I won't comment on that anymore

Field::utype unireg_check, TYPELIB *interval, const char *field_name) diff --git a/sql/sql_lex.h b/sql/sql_lex.h --- a/sql/sql_lex.h +++ b/sql/sql_lex.h @@ -2358,6 +2358,7 @@ struct LEX: public Query_tables_list char *backup_dir; /* For RESTORE/BACKUP */ char* to_log; /* For PURGE MASTER LOGS TO */ char* x509_subject,*x509_issuer,*ssl_cipher; + char *srid;

really? it can only be used for field declarations, I don't think there's a need to put it in LEX see below.

String *wild; /* Wildcard in SHOW {something} LIKE 'wild'*/ sql_exchange *exchange; select_result *result; diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -6359,6 +6360,16 @@ precision: } ;

+srid_option: + /* empty */ + { Lex->srid= (char *)0; } + | + SRID_SYM EQ NUM + { + Lex->srid=$3.str; + }

return the value of srid here, don't store it in lex. Like $$ = $3.str; or even $$ = atoi($3.str);

+ ; + field_options: /* empty */ {} | field_opt_list {} diff --git a/sql/table.cc b/sql/table.cc --- a/sql/table.cc +++ b/sql/table.cc @@ -1451,6 +1464,20 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, #ifdef HAVE_SPATIAL geom_type= (Field::geometry_type) strpos[14]; charset= &my_charset_bin; + if (gis_options && gis_options_len >= 10) + { + gis_length= uint2korr(gis_options+2); + gis_decimals= uint2korr(gis_options+4); + srid= uint4korr(gis_options+6); + gis_options_len-= 10; + gis_options+= 10;

If you have the function to generate this data in field.cc, I'd put a small function to read it in field.cc too. Just to have all reading/writing code in the same place.

+ } + else + { + srid= 0; + gis_length= 0; + gis_decimals= 0; + } #else goto err; #endif diff --git a/sql/unireg.h b/sql/unireg.h --- a/sql/unireg.h +++ b/sql/unireg.h @@ -190,6 +190,7 @@ enum extra2_frm_value_type { #define EXTRA2_ENGINE_IMPORTANT 128

EXTRA2_ENGINE_TABLEOPTS=128, + EXTRA2_GIS=129,

better make it 2, not 129 (below EXTRA2_ENGINE_IMPORTANT threshold), because you de facto ignore it anyway when HAVE_SPATIAL is not defined.

};

int rea_create_table(THD *thd, LEX_CUSTRING *frm, diff --git a/unittest/mysys/CMakeLists.txt b/unittest/mysys/CMakeLists.txt --- a/unittest/mysys/CMakeLists.txt +++ b/unittest/mysys/CMakeLists.txt @@ -17,7 +17,7 @@ MY_ADD_TESTS(bitmap base64 my_vsnprintf my_atomic my_rdtsc lf my_malloc LINK_LIBRARIES mysys)

MY_ADD_TESTS(ma_dyncol - LINK_LIBRARIES mysqlclient) + LINK_LIBRARIES mysqlclient stdc++)

Why?

IF(WIN32) MY_ADD_TESTS(my_delete LINK_LIBRARIES mysys)

Regards, Sergei