maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,


On 01/07/2016 08:53 PM, Sergei Golubchik wrote:
> Hi, Alexander!
>
> On Nov 27, Alexander Barkov wrote:
> >     Hi Serg,
> >
> > Please review a patch for MDEV-8092.
>
> I didn't review a lot of it, sorry. Because my first two comments - if
> we agree on them - might cause quite a few changes (search/replace kind
> of changes, but still). So I'd better look at the whole patch after
> that.

Sure. Thanks.

Please see below.

> > diff --git a/sql/field.h b/sql/field.h
> > index 0591914..a40e307 100644
> > --- a/sql/field.h
> > +++ b/sql/field.h
> > @@ -537,6 +537,115 @@ inline bool is_temporal_type_with_time(enum_field_types type)
> >   }
> >
> >
> > +/**
> > +  @class Name
> > +  An arbitrary SQL name, consisting of a NULL-terminated string
> > +  and its length.
> > +  Later it can be reused for table and schema names.
> > +*/
> > +class Name
> > +{
> > +  const char *m_str;
> > +  uint m_length;
>
> Sorry, we have too many "string" classes already.
> Either use LEX_STRING or create a Lex_string
> which is inherited from LEX_STRING, binary compatible with it
> and only adds C++ methods (and can be casted back and forth automatically).
> So that one could use Lex_string where a LEX_STRING is expected and vice
> versa

Sorry for a long reply.

This is a very convenient moment of time when
we can improve a few things. I disagree to go the easier way
with LEX_STRING.


LEX_STRING is a very unfortunately designed thing for the server side code.

It was a kind of Ok on 32 bit platforms.
But now on 64bit platforms it's just a waste of memory.
Length can never be large enough to use the entire "size_t".

Btw, it can even never be long enough to use "uint". It should be enough
to use uint16 for length and use the available remainder (6 bytes) to
store extra useful things, like string metadata.
It will help to avoid some operations, like character set conversion
in the client-server protocol, and I planned to add some optimization
in the future.


I propose to do the other way around:
- gradually get rid of using LEX_STRING and LEX_CSTRING for names
in the server side and use more suitable Name-alike classes instead.
- keep LEX_XXX only for compatibility with the existing pure C API,
  where it already presents now.


Please note, in the current patch version the Name class has
constructors that can accept LEX_STRING and LEX_CSTRING.
This means that you can pass LEX_STRING and LEX_CSTRING to any
function or method that is designed to accept Name,
and the patch already uses this. So this switch will go very smoothly.


As for "too many string classes", I have heard this a few times already,
both in MariaDB and earlier in Oracle :)
I don't agree with this statement.

I think that:

1. Trying to reuse classes that are not fully suitable for a task is 
more harmful than having more classes, because this is simply
not efficient.

2. Nobody objects to have many Item or Field classes that suite
a particular functionality better. But some people are very afraid
of having more strings. Why?
Various strings that we have in the server code *are* different enough.
Different string do deserve different classes.

For example, table names and column names are different enough.
They are compared differently (case sensitivity). It's very natural
that they have to have different classes behind, with different
comparison rules.

So I really hope that there will be Table_name at some point in the 
future, as I don't like tons of similar:

  if (lower_case_table_names)
    do_something;

This has to go inside Table_name.
Like I moved column name comparison inside Name::eq() in
the MDEV-8092 patch.


> > @@ -602,7 +711,7 @@ class Virtual_column_info: public Sql_alloc
> >     }
> >   };
> >
> > -class Field: public Value_source
> > +class Field: public Value_source, public Column_name
>
> I don't think so,

Ok. I agree to use a composition instead of inheritance here.

Btw, do we have a guidance when to use an inheritance vs a composition?
Sometimes it's not clear which is better.
Inheritance generally does have some benefits:
- it seems to provide more polymorphism and thus to reuse more code.
- it can fill the unused aligned memory of the parent class to store
small members of the children class (composition cannot do that).
  So a result of inheritance is generally smaller than the result of
  a composition.


> this is logically wrong.
> A field *has* a name, that's right. But a field *is not* a name.
> The Field class should include the name, not be inherited from it.

Can I spend a few more seconds of you time, to have some fun?

This reminds me the phrase:

A rabbit IS not only costly fur, but IS also 3-4 kilograms of dietetic meat.

Looks like this phrase is also logically wrong.
Or should I rather say it *has* a logically wrong phrasing? :)

Thanks.


> >   {
> >     Field(const Item &);				/* Prevent use of these */
> >     void operator=(Field &);
>
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx