maria-discuss team mailing list archive

[Oleksandr Byelkin <sanja@xxxxxxxxxxxx>]

Hi!

Sorry for delay, but I found that the answer could not be postponed.

28 марта 2011, в 16:19, Michael Widenius написал(а):

> 
> Hi!
> 
> I am cc:ing maria-discuss as you accidently forgot to also email the
> patch there.
> 
>>>>>> "Oleksandr" == Oleksandr Byelkin <sanja@xxxxxxxxxxxx> writes:
> 
> Oleksandr> Hi!
> Oleksandr> Here it is dynamic columns library. I do not think that you will have time to review it completely, but please look at it.
> 
> Oleksandr> Some notes:
> 
> Oleksandr> - I changed indices from int to uint + some other minor changes in prototypes.
> 
> Oleksandr> - dynamic_column_exists() has no way to report error (what to do?)
> 
> I assume that same problem exists for all the other functions.
> 
> Two options:
> - Change function type from my_bool to int where -1 means something
>  went bad (like bad format for the blob).
> - Add an 'error' argument to all functions that is set to <> 0 if
>  something goes wrong.

OK, then imho better to have something like this:
#define ER_DYNCOL_EOM  -1
#define ER_DYNCOL_FORMAT  -2
...

[skip]
>> 
>> +my_bool dynamic_column_init_str(DYNAMIC_COLUMN *str, size_t size)
>> +{
>> +  if (!size)
>> +    size= DYNCOL_SYZERESERVE;
>> +  /*
>> +    Make string with no fields (empty header)
>> +    - First \0 is flags
>> +    - other four \0 is 0 fileds counter
>> +  */
>> +  if (init_dynamic_string(str, NULL,
>> +                              size + 5, DYNCOL_SYZERESERVE))
>> +    return TRUE;
>> +  return dynstr_append_mem(str, "\0\0\0\0\0", 5);
>> +}
> 
> Why is an empty data 5 bytes instead of 1 or even 0?

We have fixed size part of headers - flags and number of columns.

[skip]
> 
>> +  for (i= 0; i < column_count - 1; i++)
>> +    if (columns_order[i][0] == columns_order[i+1][0])
>> +      goto err;
>> +
>> +  DBUG_ASSERT(str->length == FIXED_HEADER_SIZE);
>> +  str->str[0]|= (offset_size - 1); // size of offset
>> +  int4store(str->str + 1, non_null_count); // columns number
> 
> Lets limit the number of columns to 65536 (2 bytes)
> 
> Don't understand why we store non_null_count in header
> Don't we need to store total number of columns ?

No. We need number of columns stored in the string and we do not store NULLs.  we even decided that NULL mean removing the column.

> 
> I think we should store also null's in the dynamic columns as someone
> may want to distinguish between a non existing columns and a column
> with not a set value.
> 
> We could store this either as an extended type or as datetime with
> size 0.

We discussed it and decided that NULL mean nothing (BTW it was your idea). In case of we need it we reserved offset of all 1 bits.

[skip]
>> 
> 
> We also need functions:
> 
> Initialize the DYNAMIC_COLUMN. (Basicly just a call to
> init_dynamic_string())

I think about it, but creation is that function.

> 
>> +  /* rewrite header*/
>> +  str->str[0]|= (new_offset_size - 1); // size of offset
>> +  int4store(str->str + 1, column_count - 1); // columns number
>> +  for (i= 0, write= read= (uchar *)str->str + FIXED_HEADER_SIZE;
>> +       i < column_count;
>> +       i++, read+= entry_size, write+= new_entry_size)
>> +  {
>> +    size_t offs;
>> +    uint nm;
>> +    DYNAMIC_COLUMN_TYPE tp;
>> +    if (read == header_entry)
>> +    {
>> +#ifndef DBUG_OFF
>> +      nm= uint2korr(read);
>> +      type_and_offset_read(&tp, &offs, read + 2,
>> +                           offset_size);
>> +      DBUG_ASSERT(nm == column_nr);
>> +      DBUG_ASSERT(offs == deleted_entry_offset);
>> +#endif
>> +      write-= new_entry_size; // do not move writer
>> +      continue; // skip removed field
>> +    }
>> +
>> +    nm= uint2korr(read),
>> +    type_and_offset_read(&tp, &offs, read + 2,
>> +                         offset_size);
>> +
>> +    if (offs > deleted_entry_offset)
>> +      offs-= length; // data stored after removed data
>> +
>> +    int2store(write, nm);
>> +    type_and_offset_store(write + 2, new_offset_size, tp, offs);
>> +  }
> 
> Please optimize that if new_offset_size == old_offset_size then
> we should start from header_entry.
> 
> You should also make the above a function as update also needs to do this!
> 
>> +
>> +  /* move data */
>> +  {
>> +    size_t first_chunk_len= (data - (uchar *)str->str) -
>> +      FIXED_HEADER_SIZE - header_size;
>> +    size_t second_chunk_len= new_data_size - first_chunk_len;
>> +    if (first_chunk_len)
>> +      memmove(str->str + FIXED_HEADER_SIZE + new_header_size,
>> +              str->str + FIXED_HEADER_SIZE + header_size,
>> +              first_chunk_len);
>> +    if (second_chunk_len)
>> +      memmove(str->str +
>> +              FIXED_HEADER_SIZE + new_header_size + first_chunk_len,
>> +              str->str +
>> +              FIXED_HEADER_SIZE + header_size + first_chunk_len + length,
>> +              second_chunk_len);
>> +  }
>> +
>> +  /* fix str length */
>> +  DBUG_ASSERT(str->length >=
>> +              FIXED_HEADER_SIZE + new_header_size + new_data_size);
>> +  str->length= FIXED_HEADER_SIZE + new_header_size + new_data_size;
>> +
>> +  return FALSE;
>> +}
>> +
>> +
>> +my_bool dynamic_column_update(DYNAMIC_COLUMN *str, uint column_nr,
>> +                              DYNAMIC_COLUMN_VALUE *value)
>> +{
>> +  size_t offset_size, new_offset_size, data_size, new_data_size,
>> +         add_data_size, entry_size, new_entry_size,
>> +         header_size, new_header_size, additional_size, next_offs,
>> +         data_ins_offset;
>> +  uint column_count, i;
>> +  uchar *read, *write;
>> +  my_bool added;
>> +
>> +  /* TODO: optimize update without deleting */
>> +  if (dynamic_column_delete(str, column_nr))
>> +    return TRUE;
> 
> The problem with doing delete + insert later is that the whole
> function needs to be rewritten and you can't use any of the old code
> (for columns that exists).
> 
> So I suggest you write this ASAP!


IMHO SQL layer is mor important now :)

> 
>> +
>> +  if (value->type == DYN_COL_NULL)
>> +    return FALSE;
> 
> Should be stored.

It is strange that first you persuaded me to make as I did, then request to do it as I had wanted. Let me live it as is and change it if it will be need by somebody really. 


>> +
>> +  if (!str || str->length < FIXED_HEADER_SIZE ||
>> +      (str->str[0] & (~DYNCOL_FLG_KNOWN)))
>> +    return TRUE;
> 
> length of 0 should be regarded as ok.

Why? Any column that went through create functions will be at least size 5. If it is not true that string is malformed.

[skip]
> 
> 
>> +  /* rewrite header */
>> +  next_offs= str->length - FIXED_HEADER_SIZE - new_header_size;
>> +  for (i= column_count,
>> +       read= (uchar *)str->str + FIXED_HEADER_SIZE + header_size - entry_size,
>> +       write= ((uchar *)str->str + FIXED_HEADER_SIZE +
>> +               (new_entry_size * column_count));
>> +       i > 0;
>> +       i--, read-= entry_size, write-= new_entry_size)
>> +  {
>> +    size_t offs;
>> +    uint nm;
>> +    DYNAMIC_COLUMN_TYPE tp;
>> +
>> +    nm= uint2korr(read);
>> +    type_and_offset_read(&tp, &offs, read + 2, offset_size);
>> +
>> +    if (!added && column_nr > nm)
>> +    {
>> +      /* place to put the new column */
>> +      int2store(write, column_nr);
>> +      type_and_offset_store(write + 2, new_offset_size,
>> +                            value->type,
>> +                            (data_ins_offset= next_offs - add_data_size));
>> +      /* move previous data */
>> +      memmove(str->str + FIXED_HEADER_SIZE + new_header_size,
>> +              str->str + FIXED_HEADER_SIZE + new_header_size + add_data_size,
>> +              next_offs - add_data_size);
>> +      added= TRUE;
>> +      write-= new_entry_size;
>> +    }
>> +    int2store(write, nm);
>> +    if (!added)
>> +      offs+= add_data_size;
>> +    type_and_offset_store(write + 2,  new_offset_size, tp, offs);
>> +    next_offs= offs;
>> +  }
> 
> We should just have one function to rewrite the headers!
> 
> To make life easer, maybe we should assume that header size change
> operation is a very unlikely one and accept to do an extra memmove
> when header offset changes?
> 
> This would allow us to have a simple function that just rewrites the
> header (from a given point) and another that increases/decreases the
> header size + all data ?

Why it should be rare? (We can't know balance of delete/update/insert)

> 
> Then all operations are basicly:
> 
> - If offset_pointer_size changes, fix data
> - Add / remove things from header and data (with new offset size)
> - Update header
> 
> The whole things above would then be very trivial:
> 
> - realloc str->length if it needs to grow
> - If offset_pointer_size changes, fix data and update entry_pos
> - Update data (and create new header entry last)
> - memmove(entry, entry + entry_size, header_end - entry);
> - Write data at entry
> 

[skip]