maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Jul 24, Alexander Barkov wrote:
> On 07/03/2014 01:47 PM, Sergei Golubchik wrote:
> > Hi, Alexander!
> >
> > I don't particularly like the idea of the COLLATIONPROPERTY function,
> > The standard way of getting object metadata is to query information
> > schema. Introducing "property" function has long term implications. What
> > would be the way to get sysvar data?
> >
> >    select VARIABLE_VALUE from I_S.GLOBAL_VARIABLES where VARIABLE_NAME=xxx
> >
> > or
> >
> >    select VARIABLEPROPERTY('xxx', 'Value')
> >
> > and the help text?
> >
> >    select VARIABLEPROPERTY('xxx', 'Help')
> >
> > Storage engine of a table:
> >
> >    select TABLEPROPERTY('xxx', 'Engine')
> >
> > See - you're introducing a new way to access object metadata (and, may
> > be, data too - "property" is kind of ambigous). And what might look like
> > making sense now in 10.0 for collations, isn't necessarily reasonable
> > long term.
> >
> > As far as collations are concerned, I would rather see this data
> > in the information_schema. In the new table or in the I_S.COLLATIONS -
> > I don't know what's better, see also MDEV-6138.
> 
> I'm thinking of adding many collation properties in the future
> (e.g. tailoring, case/accent/kana sensitivity, Unicode version,
> comparison strength, sort strength, various Unicode Collation Algorithm 
> options).
> 
> So I didn't want to flood the standard table 
> INFORMATION_SCHEMA.COLLATIONS with all these properties.

Ah, yes, you're right. INFORMATION_SCHEMA.COLLATIONS is the standard
one. Indeed, it's better to add a new table then.

> It should be fine to add a new table. But I have some concerns:
> 
> 1. I don't like mixing usage statistics and static properties into the
> same table.
> 
> Should we add two new tables with about these structures:
> 
> CREATE TABLE COLLATION_STATISTICS
> (
>    ID BIGINT NOT NULL,
>    USE_COUNT BIGINT NOT NULL
> );
> 
> CREATE TABLE COLLATION_PROPERTIES
> (
>    ID BIGINT NOT NULL,
>    TAILORING TEXT CHARACTER SET UTF8MB4
>    -- things like Unicode version, sensitivity to be added here later
> );

I'm fine with one table, actually.

> 2. To search by name, one will need a join by ID,
> e.g. using a NATURAL JOIN:
> 
> SELECT TAILORING FROM
>    INFORMATION_SCHEMA.COLLATIONS
>    NATURAL JOIN
>    INFORMATION_SCHEMA.COLLATION_PROPERTIES
> WHERE
>    COLLATION_NAME='utf8_polish_ci';
> 
> A little bit cumbersome. But should probably work.

Yes, ok.

> 3. Performance should be better with a function.
> With an I_S table, we'll have to populate the table every time
> with all rows, but in fact the caller in many cases may want
> only one row (e.g. searching by name or ID).
> But performance is rather not too much important here.

There's condition pushdown for I_S tables. It's only used for
table-related I_S tables at the moment, but there's no reason why it
cannot be used for collations, if this will become an issue.

> >> === modified file 'mysys/charset.c'
> >> --- mysys/charset.c	2013-11-08 22:20:07 +0000
> >> +++ mysys/charset.c	2014-05-29 05:20:11 +0000
...
> >> +my_bool my_collation_is_known_id(uint id)
> >> +{
> >> +  return id > 0 && id < array_elements(all_charsets) && all_charsets[id] ?
> >> +         TRUE : FALSE;
> >> +}
> >
> > Hmm. I see this function is mostly used in asserts, which is great.
> > But you also use it in COLLATIONPROPERTY function, and I'm not sure it's
> > correct. One can specify an id which is not loaded yet.
> > COLLATIONPROPERTY should not fail because of that.
> 
> It should not fail.
...
> So a not loaded collation should not erroneously be treated as unknown,
> if you mean this.

Yes.

> >> +/*
> >> +  Collation use statistics functions do not lock
> >> +  counters to avoid mutex contention. This can lose
> >> +  some counter increments with high thread concurrency.
> >> +  But this should be Ok, as we don't need exact numbers.
> >> +*/
> >> +static inline void my_collation_statistics_inc_use_count(uint id)
> >> +{
> >> +  DBUG_ASSERT(my_collation_is_known_id(id));
> >> +  my_collation_statistics[id].use_count++;
> >> +}
> >> +
> >> +
> >> +ulonglong my_collation_statistics_get_use_count(uint id)
> >> +{
> >> +  DBUG_ASSERT(my_collation_is_known_id(id));
> >> +  return my_collation_statistics[id].use_count;
> >
> > Alternatively, you can define it to return 0 for unknown ids.
> > Or -1. Would be a bit easier to use in fill_collation_statistics()
> 
> Not sure. I think my way is more future prove.
> First, you check if a collation is known and thus can be queried.
> Second, on success you query for its particular property.

sure, that's up to you.
that was just a suggestion, based on how you use this function
in fill_collation_statistics().

> >> +                                 "Collation_usecount %s",
> >
> > Let's rather call it "Collation %s used", to be in line with plugin
> > rows in the feedback table. Having simply "%s used" would've been
> > even more consistent, but we don't want to have plugins and
> > collations in the same namespace. I wish I had plugins prefixed with
> > the "Plugin", but it's too late to change.
> 
> Collation names have different lengths, so putting
> collation names as the last part of the value name
> would bring more readable results:
> 
> +---------------------------------------+----+
> | Collation used utf8_unicode_ci        | 10 |
> | Collation used utf8_bin               | 20 |
> | Collation used utf8mb4_unicode_520_ci | 30 |
> +---------------------------------------+----+

Fine.
Readability is not the goal, the result is not really for a human
consumption - the script will read and parse it.

But I don't see why we need to make it intentionally unreadable :)
So, "Collation used %s" is good

Regards,
Sergei