Hi, Sergei. Would like a bit of your opinion before the revised patch. 21.10.2014 22:22, Sergei Golubchik пишет:
First, please, please, *do not* send patches from Thunderbird.
Sorry about that.
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_field> &create_fields) +{ + uint image_size= 0; + uint n_field= 0; + List_iterator<Create_field> 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?
the 'length' - it's the coordinate length, not the field length parameter that is presently stored in the FRM. Decimals can't be stored for the GEOMETRY field in ordinary way. #define FIELDFLAG_BLOB 1024 // mangled with decimals! #define FIELDFLAG_GEOM 2048 // mangled with decimals! n_field - well that is not really needed. Planned to use it as a link to the field initially, but that's lost it's sence.
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?
I thought the way to add new properties is to introduce more EXTRA2_GIS_something sections in the 'extra' part. Do you think i'd better implement similar sections to be inside the EXTRA2_GIS block? Didn't think about removing ones. That 'sections inside EXTRA2_GIS' let's the removal to some extent for a little price, so doing that...
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.
+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
But then the 'srid_option' is used in the 'type' definition like this: type: .... | spatial_type float_options srid_option { #ifdef HAVE_SPATIAL Lex->charset=&my_charset_bin; Lex->uint_geom_type= (uint)$1; $$=MYSQL_TYPE_GEOMETRY; #else my_error(ER_FEATURE_DISABLED, MYF(0), sym_group_geom.name, sym_group_geom.needed_define); MYSQL_YYABORT; #endif } if the SRID is not stored in the Lex how do you think it should be transferred to the add_field_to_list?
--- 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?
The 10.1 linking fails on my computer withouth that stdc++ thing. I don't actually plan to push that anyhow with this task. Will try to understand later what's going on. Best regards. HF