maria-developers team mailing list archive

[Alexey Botchkov <holyfoot@xxxxxxxxxxxx>]

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