maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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_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?

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