Re: [Maria-developers] commit 192fea6b104b249bb4764635bb3a52fb9c8a2148 MDEV-27128

[Alexey Botchkov]

Hi, Rucha.

I decided i dislike the overall design of the struct json_schema.
Seems better to do it with C++ classes using virtual methods.
So instead of structures we'd have classes as
   json_schema_const
   json_schema_enum
   json_scnema_string
   json_schema_number
  etc.
  they all inherited from the
  class json_schema_item
 {
 virtual parse()
 virtual check()
}
Parsing the schema we can behave like this:
json_parse_schema()
   checks all the keys on the current level
   looking for 'const' 'enum' and 'typ[e' keywords only.
   If it gets 'const' or 'enum' it creates the
json_schema_const/enum respectively
  For the "type" it creates the appropriate json_schema_xxx and
  callst ->parse() with the same JSON object. So the parse would go over it
once
  again looking for the type-specific keywords.
That way we don't need the hash_list as the destructor of the
json_schema_xxx class
would free it's resources.

Minor comments.
You compare keys like strncmp((const char*)curr_key, "const", key_len)
all over the code. But it is not correct.
That way 'cons' or 'con' or 'co' or even 'c' - they all will be considered
equal to the 'const'.
Just add the 'key_cmp()' function that would compare key lengths then do
the strncmp.

> double val= je->s.cs->strntod((char *) je->value, je->value_len, 
You do this even if the value_type is not number. Not very safe. Better to
avoid doing
that here at all.

That queriy just crashes the server. Didn't yet found why
SET @schema_array= '{
   "typ": "arra",
   "prefixItem": [
   { "typ": "objec"},
   { "typ": "objec",
 "propertie": {
 "number1":
{"typ":"numbe"},
 "string1":
{"typ":"strin"},
 "obj1" :
{"typ":"objec",

 "propertie": {

"key1" : {"type":"string"}

 }
   }
},
  "required":["obj1"]
   }
  ],
"items":false
}';

SELECT JSON_SCHEMA_VALID(@schema_array, '[{"key1":"value1"}, {"number1":1,
"string1":"str1", "obj1":{"key1":"val1"}}]');


Feel free to discuss the design details!

Best regards.
HF
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] b248988c8b6: MDEV-19275 Provide SQL service to plugins.

[Alexey Botchkov]

Hi, Sergei.

Unfortunately the taken approach led to compiler errors on Windows and
(seemingly) the rpl_shutdown_wait_slaves test failure.
So i made the change - to preserve the way we do the includes.
76c4616c2cc47ecc85362d0c5a2bffd9495e4753

Best regards.
HF



On Mon, Sep 13, 2021 at 12:42 PM Sergei Golubchik  wrote:

> Hi, Alexey!
>
> Looks good to me, thanks.
> Please tell Sanja to rebase on top of that, push the result into a
> preview-* branch, and update the TODO.
>
> > So the patch with the abovementioned changes.
> > 9ad45b41d385fca8215880cce1757ebe7f2396eb
> >
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] b248988c8b6: MDEV-19275 Provide SQL service to plugins.

[Alexey Botchkov]

> What security context are you using now?
> root? whatever happen to be in the THD?
> What if you create a new THD?

The thd->security_ctx->skip_grants() is done for the created THD.
Otherwise yes, the current context is used.
Going to fix that with the local empty context.

> Feel free to replace it with any other macro that is
> guaranteed to be set for the server code.
Since at the moment it only affects the SQL service plugin, i'd keep it as
you did.
Just adding the comment.
If something else starts to depend on it, then we'll better add a specific
define for it.


> > +  thd->set_time();

> why? May be it should be executed using the timestamp of the top-level
> statement?

When that can be desirable?
I did that thinking 'what if the set_time() for the current thread wasn't
called at all'.
Though removed this for now.

So the patch with the abovementioned changes.
9ad45b41d385fca8215880cce1757ebe7f2396eb

Best regards.
HF


On Fri, Sep 10, 2021 at 7:54 PM Sergei Golubchik  wrote:

> Hi, Alexey!
>
> On Sep 10, Alexey Botchkov wrote:
> > > For example, I'd think it connects to the current server, internally.
> > > But then, why does it need host and user?
> >
> > Initially i thought it can be put in the thread's security_ctx so
> functions
> > like CURRENT_USER
> > return these values. After some more meditation i decided that it
> > doesn't seem to be useful and just removed these arguments.
>
> What security context are you using now?
> root? whatever happen to be in the THD?
> What if you create a new THD?
>
> > > > +ADD_DEFINITIONS(-DMYSQL_SERVER)
> > > Why?
> > Couldn't figure out how to get rid of it.
> > Well your trick with the '#if !defined(MYSQL_SERVICE_SQL)' in the mysql.h
> > helps.
> > I wasn't bold enough to mention the particular service in such an exposed
> > file as mysql.h :)
>
> Feel free to replace it with any other macro that is guaranteed to be
> set for the server code (including plugins and anything that doesn't
> define MYSQL_SERVER, but still is in the server). Any plugin related
> define should work, I think.
>
> > > > +#include 
> > > shouldn't be needed
> > My idea was that those not using the SQL service don't have to see the
> > 'mysql.h' declarations.
> > Now all the plugins see all these mysql_xxx() functions and related
> > structures. Not sure if it's good.
>
> I simply mean that you already include mysql.h in the service_sql.h.
>
> If you wouldn't, then yes, a separate include would be needed.
>
> > The update pushed 71338ddada9f331af57b737a586f56dc6f2e483f.
>
> Thanks. One unanswered question below:
>
> > diff --git a/sql/sql_class.h b/sql/sql_class.h
> > index e569fcd32d6..964626be3d4 100644
> > --- a/sql/sql_class.h
> > +++ b/sql/sql_class.h
> > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
> > index 09ad632dd98..4725855c130 100644
> > --- a/sql/sql_prepare.cc
> > +++ b/sql/sql_prepare.cc
> > @@ -4857,6 +4870,7 @@
> Prepared_statement::execute_server_runnable(Server_runnable
> *server_runnable)
> >if (!(lex= new (mem_root) st_lex_local))
> >  return TRUE;
> >
> > +  thd->set_time();
>
> why? May be it should be executed using the timestamp of the top-level
> statement?
>
> >thd->set_n_backup_statement(this, _backup);
> >thd->set_n_backup_active_arena(this, _backup);
> >thd->stmt_arena= this;
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] b248988c8b6: MDEV-19275 Provide SQL service to plugins.

[Alexey Botchkov]

> For example, I'd think it connects to the current server, internally.
> But then, why does it need host and user?

Initially i thought it can be put in the thread's security_ctx so functions
like CURRENT_USER
return these values. After some more meditation i decided that it
doesn't seem to be useful and just removed these arguments.


> > +ADD_DEFINITIONS(-DMYSQL_SERVER)
> Why?

Couldn't figure out how to get rid of it.
Well your trick with the '#if !defined(MYSQL_SERVICE_SQL)' in the mysql.h
helps.
I wasn't bold enough to mention the particular service in such an exposed
file as mysql.h :)


> > +#include 
> shouldn't be needed
My idea was that those not using the SQL service don't have to see the
'mysql.h' declarations.
Now all the plugins see all these mysql_xxx() functions and related
structures. Not sure if it's good.

> Why did you test local and global connections?
> What difference in behavior can one expect?

Shouldn't be any difference in results. I test it though as
the 'global' and the 'local' connection work differently.

The 'global' does the mysql_real_connect() in the xxx_plugin_init() and
disconnects in the plugin_deinit(),
while the 'local' inited and closed in one function.

> > +  if (ddl_log_initialize())
> > +unireg_abort(1);

> why?

So that call was moved to be before the 'plugin_init()' call.
The 'CREATE TABLE' query in the test_sql_service_init() fails as the log is
not initialized.


The update pushed 71338ddada9f331af57b737a586f56dc6f2e483f.

Best regards.
HF


On Tue, Sep 7, 2021 at 5:58 PM Sergei Golubchik  wrote:

> Hi, Alexey!
>
> see questions below
>
> On Sep 07, Alexey Botchkov wrote:
> > revision-id: b248988c8b6 (mariadb-10.6.1-74-gb248988c8b6)
> > parent(s): 42ad4fa3464
> > author: Alexey Botchkov
> > committer: Alexey Botchkov
> > timestamp: 2021-09-07 14:08:13 +0400
> > message:
> >
> > MDEV-19275 Provide SQL service to plugins.
> >
> > SQL service added.
> > It provides the limited set of client library functions
> > to be used by plugin.
> >
> > diff --git a/include/mysql/service_sql.h b/include/mysql/service_sql.h
> > new file mode 100644
> > index 000..5050793f655
> > --- /dev/null
> > +++ b/include/mysql/service_sql.h
> > @@ -0,0 +1,99 @@
> > +/* Copyright (C) 2021 MariaDB Corporation
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; version 2 of the License.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program; if not, write to the Free Software
> > +   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02111-1301 USA */
> > +
> > +#if defined(MYSQL_SERVER) && !defined MYSQL_SERVICE_SQL
> > +#define MYSQL_SERVICE_SQL
> > +
> > +#include 
> > +
> > +/**
> > +  @file
> > +  SQL service
> > +
> > +  Interface for plugins to execute SQL queries on the local server.
> > +
> > +  Functions of the service are the 'server-limited'  client library:
> > + mysql_init
> > + mysql_real_connect_local
> > + mysql_real_connect
> > + mysql_errno
> > + mysql_error
> > + mysql_real_query
> > + mysql_affected_rows
> > + mysql_num_rows
> > + mysql_store_result
> > + mysql_free_result
> > + mysql_fetch_row
> > + mysql_close
> > +*/
> > +
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +extern struct sql_service_st {
> > +  MYSQL * STDCALL (*mysql_init)(MYSQL *mysql);
> > +  MYSQL *(*mysql_real_connect_local)(MYSQL *mysql,
> > +const char *host, const char *user, const char *db,
> > +unsigned long clientflag);
> > +  MYSQL * STDCALL (*mysql_real_connect)(MYSQL *mysql, const char *host,
> > +  const char *user, const char *passwd, const char *db, unsigned
> int port,
> > +  const char *unix_socket, unsigned long clientflag);
> > +  unsigned int STDCALL (*mysql_errno)(MYSQL *mysql);
> > +  const char * STDCALL (*mysql_error)(MYSQL *mysql);
> > +  int STDCALL (*mysql_real_query)(MYSQL *mysql, const char *q,
> > +  unsigne

Re: [Maria-developers] MDEV-25822: Re: JSON_TABLE: on default values

[Alexey Botchkov]

> When I see this, I wonder if it was possible to make use of a general
'literal'
> rule instead?
Well the 'literal' rule doesn't accept the -0.5 :)
But basically agree.

> DEFAULT NULL ON {EMPTY|ERROR}
> redundant. I don't see any problem if we support this, though.
Let it be so.

Here is the new patch.

https://github.com/MariaDB/server/commit/8dae7ee02f98e71e9352d73d1da235fd4128d076

Best regards.
HF


On Tue, Jun 8, 2021 at 6:50 PM Sergey Petrunia  wrote:

> Hi Alexey,
>
> > On Sun, May 30, 2021 at 06:44:19AM +0400, Alexey Botchkov wrote:
> > > Hi, Sergey!
> > >
> > > I meditated about it for some time. I remember i was thinking on that
> part
> > > before and
> > > did that so for some reason. Though either i was wrong or didn't finish
> > > what i planned.
> > > This time i'd say we should allow numeric constants there too.
> > > Here's the patch i'd push to fix this:
> > >
> https://github.com/MariaDB/server/commit/9c518e4cc9b0569cae2daa5a4024e209293eca45
>
> In the next patch, please refer to MDEV-25822 in the commit comment.
>
> select * from
> json_table('{"a": "b"}',
>'$' columns(col1 varchar(32) path '$.fooo' default 123456 on
> empty)
>  ) as T;
> ++
> | col1   |
> ++
> | 123456 |
> ++
>
> Ok.
>
> "default 0.5" also works
> "default -0.5" DOESN'T - it produces a parse error.
> "default 8446744073709551615" - works, bigint is accepted
> "default 18446744073709551615" - DOESN'T, bigint unsigned is not accepted
>
> Other kinds of literals like "DATE '20201-01-01'" are not accepted either.
>
>
> The cause of this is the grammar, which allows to accept only certain
> kinds of
> literals:
>
> > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> > index 37cdfc20030ab..192a4879b751d 100644
> > --- a/sql/sql_yacc.yy
> > +++ b/sql/sql_yacc.yy
> > @@ -1324,6 +1324,7 @@ End SQL_MODE_ORACLE_SPECIFIC */
> >  TEXT_STRING
> >  NCHAR_STRING
> >  json_text_literal
> > +json_text_literal_or_num
> >
> >  %type 
> >  opt_table_alias_clause
> > @@ -11550,6 +11551,26 @@ json_text_literal:
> >}
> >  ;
> >
> > +json_text_literal_or_num:
> > +  json_text_literal
> > +| NUM
> > +  {
> > +Lex->json_table->m_text_literal_cs= NULL;
> > +  }
> > +| LONG_NUM
> > +  {
> > +Lex->json_table->m_text_literal_cs= NULL;
> > +  }
> > +| DECIMAL_NUM
> > +  {
> > +Lex->json_table->m_text_literal_cs= NULL;
> > +  }
> > +| FLOAT_NUM
> > +  {
> > +Lex->json_table->m_text_literal_cs= NULL;
> > +  }
> > +;
> > +
> >  join_table_list:
> >derived_table_list { MYSQL_YYABORT_UNLESS($$=$1); }
> >  ;
>
> When I see this, I wonder if it was possible to make use of a general
> 'literal'
> rule instead?
> That rule differs from the current symbols: it produces Item expressions.
> Is
> this a problem? (I don't think it should be).
>
> The only concern about allowing more kinds of literals is NULL literals:
> JSON
> TABLE has special provision for emitting NULL:
>
>   NULL ON {EMPTY|ERROR}
>
> which makes supporting
>
>   DEFAULT NULL ON {EMPTY|ERROR}
>
> redundant. I don't see any problem if we support this, though.
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-25875: RE 02469b: MDEV-17399 JSON_TABLE Accept JSON values for the JSON fields

[Alexey Botchkov]

Hello, Sergey!
> The name "store_json_in_json" doesn't look good to me.
> Do you think store_element_as_json_doc() would be better?
I decided to rename it as store_value_as_json_doc()
The 'element' sounds somewhat unclear to me.


> > +  if (!(error= store_json_in_json(*f, )))
> > +error= er_handler.errors;
> Can there be a case when this assignment assigns any value other than 0?

Well i hardly can imagine anything else than 0. Not-zero happens when
setting a value to a field
produces an error, but there's no realistic errors that can happen when we
set the confirmed valid JSON to the
JSON field. So can probably be removed.
I decided to keep it to behave same way as with other field types, and to
be more future-prone.
Say When we implement the FORMAT JSON.

> If there was an error inside store_json_in_json, we do not
> get to this assignment, as store_json_in_json() returned non-zero error.
> Is this correct?)

No. You can look at the similar code around the store_json_in_field().
store_json_in_jso() returns 0 even if the field->store() failed.
Then we check the er_handler.errors to check if that sort of failure was
important to us.

> select * from  json_table('{"foo": null}', '$' columns( jscol json path
'$.foo')) as T;
> with the above patch, it produces a JSON null value:
> while in MySQL-8 it produces an SQL NULL:
The JSON null seems more correct to me. It's the JSON null in the original
document, why
to replace it with the SQL NULL? And it can be useful to detect cases when
the column was
undefined ans set to the SQL NULL.

Here is the new patch
https://github.com/MariaDB/server/commit/fe0dc6ba769dcb468b37a8c3e3c636c0049f7307

Best regards.
HF


On Tue, Jun 8, 2021 at 2:55 PM Sergey Petrunia  wrote:

> Hi Alexey,
>
> This is review input re
>
> https://github.com/MariaDB/server/commit/02469bdead5753eccb5d70c98a158a07027f4eb2
> .
>
> First, our workflow dictates that this patch should have its own MDEV
> entry.
> I've filed it https://jira.mariadb.org/browse/MDEV-25875.
> Please use it in commit descriptions.
>
> == Updates mixed with another patch ==
>
> The patch has .result changes like:
>
> > @@ -551,10 +551,12 @@ JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x'
> DEFAULT NULL ON ERROR)) jt;
> >  ERROR 42000: You have an error in your SQL syntax; check the manual
> that corresponds to your MariaDB server version for the right syntax to use
> near 'NULL ON ERROR)) jt' at line 2
> >  SELECT * FROM
> >  JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON EMPTY)) jt;
> > -ERROR 42000: You have an error in your SQL syntax; check the manual
> that corresponds to your MariaDB server version for the right syntax to use
> near '0 ON EMPTY)) jt' at line 2
> > +x
> > +0
> >  SELECT * FROM
> >  JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON ERROR)) jt;
> > -ERROR 42000: You have an error in your SQL syntax; check the manual
> that corresponds to your MariaDB server version for the right syntax to use
> near '0 ON ERROR)) jt' at line 2
> > +x
> > +NULL
> >  SELECT * FROM
> >  JSON_TABLE('{}', '$' COLUMNS (x DATE
> >  PATH '$.x'
>
> These are obviously from another patch (the one about "accepting non-string
> literals as default").  Please move them to that patch.
>
> == Comments ==
>
> > @@ -377,6 +378,25 @@ static void store_json_in_field(Field *f, const
> json_engine_t *je)
> >  }
> >
> >
> > +static int store_json_in_json(Field *f, json_engine_t *je)
> > +{
> Please add a comment for the function. Something like
>
> "Store the current JSON element pointed by je into field f, as a separate
> JSON
> document"
>
> The name "store_json_in_json" doesn't look good to me.
> Do you think store_element_as_json_doc() would be better?
>
> > +  const uchar *from= je->value_begin;
> > +  const uchar *to;
> > +
> > +  if (json_value_scalar(je))
> > +to= je->value_end;
> > +  else
> > +  {
> > +int error;
> > +if ((error= json_skip_level(je)))
> > +  return error;
> > +to= je->s.c_str;
> > +  }
> > +  f->store((const char *) from, (uint32) (to - from), je->s.cs);
> > +  return 0;
> > +}
> > +
> > +
> >  bool Json_table_nested_path::check_error(const char *str)
> >  {
> >if (m_engine.s.error)
> > @@ -541,7 +561,12 @@ int ha_json_table::fill_column_values(THD *thd,
> uchar * buf, uchar *pos)
> >}
> >else
> >{
> > -if (!(error= !json_value_scalar()))
> > +if (jc->m_format_json)
> > +{
> > +  if (!(error= store_json_in_json(*f, )))
> > +error= er_handler.errors;
> Can there be a case when this assignment assigns any value other than 0?
> (My logic - er_handler.errors already had some error status, why did we
> call
> store_json_in_json?  If there was an error inside store_json_in_json, we
> do not
> get to this assignment, as store_json_in_json() returned non-zero error.
> Is
> this correct?)
>
> > +}
> > +else if (!(error= 

Re: [Maria-developers] Tips on Writing/Reading JSON

[Alexey Botchkov]

Hello, Michael!

Simple tools for reading JSON are declared in
the include/mysql/service_json.h.
Functions declared there are easy to use and understand, and they can read
any JSON.
Should be enough for you, at least to begin with. You need to look any
deeper only if
you have to make your code really fast, say if you're going to parse entire
columns of a big tables.

These 'simple' functions are used in sql/sql_acl.cc if you need an example.

I didn't develop any specific tools for writing the JSON. Relied on the
existing String class. Was mostly enough,
just few helper functions can be found there in sql/item_jsonfunc.cc.

Sergei Petrunia implemented the Json_writer class that writes the JSON, can
probably be utilized for your needs.

If you have any specific questions on how to parse this or get that value
from the JSON - don't hesitate to ask!


Best regards.
HF


On Wed, Jun 2, 2021 at 5:46 PM Michael Okoko 
wrote:

> Hi Alexey,
>
> I'm Michael Okoko, a GSoC '21 student working on Using JSON as the on-disk
> format for MariaDB histograms(https://jira.mariadb.org/browse/MDEV-21130).
>
> The project requires writing and reading/parsing JSON, and I was wondering
> if you could provide some tips and pointers regarding a proper JSON
> writer/JSON parser to use.
>
> Regards,
> Michael.
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Using JSON_TABLE to create JSON columns?

[Alexey Botchkov]

Hi, Sergey!

I think saving JSON values in the JSON fields (as the MySQL does) makes
sense.
So here is the patch for it
https://github.com/MariaDB/server/commit/02469bdead5753eccb5d70c98a158a07027f4eb2

Best regards.
HF


On Mon, Jun 22, 2020 at 4:48 PM Sergey Petrunia  wrote:

> Hi Alexey and everyone,
>
> I was looking at whether it's possible use JSON_TABLE to extract portions
> of JSON
> document. Apparently it is possible in MySQL with JSON datatype:
>
> Q1:
> select *
> from
>   json_table('[{"color": "blue",  "price": { "high": 10, "low": 5}},
>{"color": "red",   "price": { "high": 20, "low": 8}}]',
>  '$[*]' columns(color varchar(100) path '$.color',
>   price json path '$.price'
> )
>  ) as T;
> +---++
> | color | price  |
> +---++
> | blue  | {"low": 5, "high": 10} |
> | red   | {"low": 8, "high": 20} |
> +---++
>
>
> Note that if one uses any datatype other than JSON, they get NULLs:
>
> Q2:
> select *
> from
>   json_table('[{"color": "blue",  "price": { "high": 10, "low": 5}},
>{"color": "red",   "price": { "high": 20, "low": 8}}]',
>  '$[*]' columns(color varchar(100) path '$.color',
>   price text path '$.price'
> )
>  ) as T;
> +---+---+
> | color | price |
> +---+---+
> | blue  | NULL  |
> | red   | NULL  |
> +---+---+
>
> Oracle-the-database doesn't yet(*) have a JSON datatype. So I can only run
> Q2
> and then I get NULLs in the price column.
>
> MariaDB accepts JSON as datatype so query Q1 is accepted.
> However the logic in MDEV-17399 code doesn't have support for dumping a
> portion
> of JSON document, so one gets empty strings in the price column.
>
> Should we support Q1 with JSON output in the price column?  If yes, should
> we
> do it within the scope of MDEV-17399 or create another task for this?
>
>
> (*) I see this:
>
> https://docs.oracle.com/en/database/oracle/oracle-database/20/newft/new-json-data-type.html
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] JSON_TABLE: on default values

[Alexey Botchkov]

Hi, Sergey!

I meditated about it for some time. I remember i was thinking on that part
before and
did that so for some reason. Though either i was wrong or didn't finish
what i planned.
This time i'd say we should allow numeric constants there too.
Here's the patch i'd push to fix this:
https://github.com/MariaDB/server/commit/9c518e4cc9b0569cae2daa5a4024e209293eca45

Best regards.
HF


On Wed, May 26, 2021 at 8:01 PM Sergey Petrunia  wrote:

> Hi Alexey,
>
> At the moment MariaDB requires that the values in DEFAULT clauses are
> quoted.
> Example:
>
> select *
> from
>   json_table(
> '{"intval": 1000}',
> '$' columns(
>  col1 int path '$.intval_'
> default '100' on empty
>)
>) as T;
>
> here, "100" must be quoted, otherwise one gets a parse error.  However, the
> quoted value is interpreted as an SQL literal.  This looks puzzling.
>
> MySQL-8 also requires that the default value is quoted, but they have a
> (very
> odd) reason for it: they interpret the default value as JSON:
>
> https://docs.oracle.com/cd/E17952_01/mysql-8.0-en/json-table-functions.html
> says:
>
>   DEFAULT json_string ON EMPTY: the provided json_string is parsed as
> JSON, as
>   long as it is valid, and stored instead of the missing value. Column
> type
>   rules also apply to the default value.
>
> I am not sure why MySQL chose to do this. Looking into the SQL Standard,
> one can
> see:
>
>  ::=
>  
>   [ PATH  ]
>   [  ON EMPTY ]
>   [  ON ERROR ]
>
>  ::=
> ERROR
>   | NULL
>   | DEFAULT 
>
> ...
> This doesn't say whether the  should be interepreted as
> JSON
> or just as a value.  But one can find this passage:
>
> 
> Without Feature T826, “General value expression in ON ERROR or ON EMPTY
> clauses”, the  expression> contained in  or  column error behavior>
> contained in a  JTRCD shall be a
>  that can be cast to the
> data type specified by the  contained in JTRCD without raising
> an exception condition
> according to the General Rules of Subclause 6.13, “”.
> 
>
> The important part is:
>
> ... shall be a  that can be cast to the data type specified ...
>
> which means it is not JSON. It is just a literal, and literal can be a
> string
> literal (in quotes, 'string') or an integer literal (without quotes) or
> other
> kind of literal.
>
> Btw, Oracle Database allows non-string literals in the default clause:
>
> https://dbfiddle.uk/?rdbms=oracle_18=9af7e43ede77ee285e1a65f1f419d3bd
>
> What are your thoughts on this?
> Is MariaDB's behavior intentional? Should we follow the standard and allow
> all
> kinds of literals?  What was the reason for the limitation that default
> values
> are quoted?
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] 308c28e8a60: MDEV-15888 Implement FLUSH TABLES tbl_name [, tbl_name] ... WITH READ LOCK for views.

[Alexey Botchkov]

The new patch about it was submitted.
https://github.com/MariaDB/server/commit/a40a36d9b9dc8b78171214905dce6d7fe19961d7

>   it only tests that there's no error.
Tests for locking added.

> > +#endif /*MDEV15887*/
> what's that?

That piece of code seems to do nothing. But let's discuss it later.

Best regards.
HF


On Sun, Mar 21, 2021 at 7:48 PM Sergei Golubchik  wrote:

> Hi, Alexey!
>
> On Mar 21, Alexey Botchkov wrote:
> > revision-id: 308c28e8a60 (mariadb-10.5.2-393-g308c28e8a60)
> > parent(s): 786bc312b85
> > author: Alexey Botchkov 
> > committer: Alexey Botchkov 
> > timestamp: 2021-02-09 00:59:55 +0400
> > message:
> >
> > MDEV-15888 Implement FLUSH TABLES tbl_name [, tbl_name] ... WITH READ
> LOCK for views.
> >
> > Kind of 'naive' solution that surprisingly worked so trying to
> > understand why.
> >
> > diff --git a/mysql-test/main/flush-innodb.result
> b/mysql-test/main/flush-innodb.result
> > index 21e5bda7785..2c886e4f9fc 100644
> > --- a/mysql-test/main/flush-innodb.result
> > +++ b/mysql-test/main/flush-innodb.result
> > @@ -60,7 +60,7 @@ DROP TABLE export;
> >  CREATE VIEW v1 AS SELECT 1;
> >  CREATE TEMPORARY TABLE t1 (a INT);
> >  FLUSH TABLES v1 FOR EXPORT;
> > -ERROR HY000: 'test.v1' is not of type 'BASE TABLE'
> > +UNLOCK TABLES;
>
> good, but it only tests that there's no error.
> may be you need to add tests to verify that tables are actually
> locked?
>
> >  FLUSH TABLES t1 FOR EXPORT;
> >  ERROR 42S02: Table 'test.t1' doesn't exist
> >  FLUSH TABLES non_existent FOR EXPORT;
> > diff --git a/sql/sql_reload.cc b/sql/sql_reload.cc
> > index 8f87d633d19..c146b7693c1 100644
> > --- a/sql/sql_reload.cc
> > +++ b/sql/sql_reload.cc
> > @@ -543,6 +543,7 @@ bool flush_tables_with_read_lock(THD *thd,
> TABLE_LIST *all_tables)
> >
> >if (thd->lex->type & REFRESH_READ_LOCK)
> >{
> > +#ifdef MDEV15887
> >  /*
> >Acquire SNW locks on tables to be flushed. Don't acquire global
> >IX and database-scope IX locks on the tables as this will make
> > @@ -559,6 +560,7 @@ bool flush_tables_with_read_lock(THD *thd,
> TABLE_LIST *all_tables)
> >  for (auto table_list= all_tables; table_list;
> >   table_list= table_list->next_global)
> >table_list->mdl_request.ticket= NULL;
> > +#endif /*MDEV15887*/
>
> what's that?
> btw, the number is MDEV15888
>
> >}
> >
> >thd->variables.option_bits|= OPTION_TABLE_LOCK;
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-13467 review.

[Alexey Botchkov]

One thing i'd still like you to do differently.

+  if (g->get_class_info()->m_type_id == Geometry::wkb_multipoint)
+  {
+const char point_size= 4 + WKB_HEADER_SIZE + POINT_DATA_SIZE+1; //1
for the type
+char point_temp[point_size];
+memset(point_temp+4, Geometry::wkb_point, 1);
+memcpy(point_temp+5, static_cast(g)->get_data_ptr()+5, 4);
+memcpy(point_temp+4+WKB_HEADER_SIZE, g->get_data_ptr()+4+WKB_
HEADER_SIZE,
+   POINT_DATA_SIZE);
+point_temp[point_size-1]= '\0';
+Geometry_buffer gbuff;
+Geometry *gg= Geometry::construct(, point_temp, point_size-1);
> Gis_multi_point::geometry_n(uint32 num, String *result) stores the point
in String *,
> but I need Geometry* to use further, so still would need some
recalculation.

You could call the ::construct with the string->prr() instead of point_temp.
Like
  str->q_append(0); //SRID
  g->geometry_n(str, 1);
  gg= Geometry::construct(, str->ptr(), str->length());

But I propose an even simpler method.
{
  Gis_pont p;
  p.set_data_ptr(g->get_data_ptr()+4+WKB_HEADER_SIZE, POINT_DATA_SIZE);
  calculate_haversine(, radius);
}

> Regarding radius, Daniel has suggestion: when r=NULL, result= NULL, but
r=0,
> how should that be treated?
> Here is MySQL example (no error, but warning)

There i see that MySQL returns the error when the radius is 0 or below.
Right?
I think it's ok for us to do same.

> In a single function we are calculating radian for x,y, I think it is
faster than to call function 2 times for x and y.
I doubt so.
Really would like to compare the speed here. Still i'm ok with that single
function if you like it better.

Best regards.
HF




On Tue, Mar 2, 2021 at 1:16 PM Anel Husakovic  wrote:

> Hi Alexey,
> thanks for your review, hope everything is ok now.
> Here is the new patch 1f618e36577a2a322807
> <https://github.com/MariaDB/server/commit/1f618e36577a2a32280705c640d6f9a74861a3c6>
>  according
> to your review and comments below.
>
> On Sat, Jan 9, 2021 at 2:09 PM Alexey Botchkov 
> wrote:
>
>> Hi, Anel!
>>
>> Sorry for that long delay with this review. Had real difficult weeks of
>> my life :(
>> Now it seems to be over, so getting back to normal.
>> Below is my opinions of the code i see in the branch
>> bb-10.1-anel-MDEV-13467-gis-feature-st_sphere-v2
>> Hope it's the recent version.
>>
>> +  Adapter for functions that takes two or three arguments.
>> +*/
>> +
>> +class Create_func_arg2_or_3 : public Create_func
>> +{
>> Do you think we need the separate Create_fund_arg2_or_3 class?
>> If you don't see any other class to inherit from it,
>> i'd recommend to leave the Create_func_distance_sphere alone.
>> No need to have two constructors in the Item_func_sphere_distance
>> too. Just use the list argument as it's done for the
>> Item_func_json_contains (in 10.2).
>>
>
> Ack ^ droped the Create_func_arg2_or_3 and inherited from
> Create_native_function which can use a variable number of parameters.
> Thanks
>
>
>> Anyway i belive we should get rid of duplicating code
>> about the 'param1' and 'param2'. They can be popped/checked
>> with no if (arg_count == 2) condition.
>>
>> +  double distance= 0.0;
>> +  null_value= (args[0]->null_value || args[1]->null_value);
>> +  if (null_value)
>> +  {
>> +// Returned item must be set to Null
>> +DBUG_ASSERT(maybe_null);
>>don't think it's needed.
>> +return 0;
>> Should be return 0.0 i belive. Or goto handle_errors.
>> Thet goes to all but one 'return' statements in this function.
>>
>> +  }
>>
>> > ...
>> > if (!arg1 || !arg2)
>>
>> This last condition should never happen since we have not-null_value.
>> So i'd remove that 'if' section completely.
>> And anyway it should do the 'return' in that branch.
>>
>> +null_value= args[2]->null_value;
>> +if (null_value)
>> +{
>> +  return 0;
>> +}
>>
>> seems nicer to me this way
>> if (args[2]->null_value)
>> {
>>   null_value= true;
>>   goto handle_errors;
>> }
>>
>>
> Ack.
> Regarding radius, Daniel has suggestion
> <https://github.com/MariaDB/server/commit/e42b69136a31cb596e940d1ceafffa5c1163fc19#r45124982>:
> when r=NULL, result= NULL, but r=0, how should that be treated?
> Here is MySQL
> <https://dbfiddle.uk/?rdbms=mysql_8.0=d467903dce23464df0ebf396220da886>example
> (no error, but warning)
>
>
>
>> +  if (!(g1= Geometry::construct(, arg1->ptr(), arg1->length()))
>> ||
>> +  !(g2= Geometry::construct(, arg2->ptr(), arg2-&g

Re: [Maria-developers] MDEV-17399: JSON_TABLE: final input

[Alexey Botchkov]

The Json_table_nested_path lives only inside the Table_function_json_table
and that last one
is the only class that manipulates the Json_table_nested_path internals.
Using methods
in your version, but it's the only class using these methods anyway.
So to me it's seems more honest to declare classes as friends so we
explicitly allow access to
the internals to that one class and disallow to everyone else.

But yes, it's rather the matter of taste.

Best regards.
HF


On Fri, Mar 12, 2021 at 2:33 AM Sergey Petrunia  wrote:

> On Sat, Mar 06, 2021 at 01:13:53AM +0400, Alexey Botchkov wrote:
> > Hi, Sergei!
> >
> > I pushed the patch to the feature branch for you to take a look.
> > The patch you proposed
> > http://lists.askmonty.org/pipermail/commits/2021-March/014492.html
> >  I liked and adapted with one exception. The nested paths list is built
> > using the **last_sibling_hook
> > instead of *cur_last_sibling. That seems to me nicer and doesn't produce
> > that many repeating lines
> > in the code.
> >
>
> On the other hand, use of "last_sibling_hook" makes
> Table_function_json_table to
> be aware about the internals of Json_table_nested_path (you had to make it
> a
> friend class).
>
> And the price to pay for complete isolation was the if-else in
> start_nested_path(), with two lines in either branch.
> So I would still say that the suggested solution was cleaner.
>
> I don't consider this to be a showstopper issue, though.
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-17399: JSON_TABLE: Incorrect code with table elimination

[Alexey Botchkov]

I've rewritten it using the json->select_lex->leaf_tables as you suggested.
So the JSON_TABLE is calculated properly and all the dependencies seem to
be marked.
Still your testcase query fails the same way, so needs some more
investigation. To me it
looks more like optimizer issue than the JSON_TABLE-s.

Best regards.
HF


On Fri, Mar 12, 2021 at 2:21 AM Sergey Petrunia  wrote:

> Hi Alexey,
>
> > diff --git a/sql/opt_table_elimination.cc b/sql/opt_table_elimination.cc
> > index 3958797ec44..f2497d524ec 100644
> > --- a/sql/opt_table_elimination.cc
> > +++ b/sql/opt_table_elimination.cc
> > @@ -637,6 +637,16 @@ void eliminate_tables(JOIN *join)
> >List_iterator it(join->fields_list);
> >while ((item= it++))
> >  used_tables |= item->used_tables();
> > +
> > +  {
> > +List_iterator it(*join->join_list);
> > +TABLE_LIST *tbl;
> > +while ((tbl= it++))
> > +{
> > +  if (tbl->table_function)
> > +used_tables|= tbl->table_function->used_tables();
> > +}
> > +  }
> >
>
> This only walks the tables that are "at the top level" of the join. if a
> JSON_TABLE(...) is inside a nested outer join, it will not be found.
>
> Please walk select_lex->leaf_tables instead. Please add the testcase (I
> provide
> one below).
>
> Please also add a comment, clarifying what is being done, something like:
>
>   Table function JSON_TABLE() can have references to other tables. Do not
>   eliminate the tables that JSON_TABLE() refers to. Note: the JSON_TABLE
> itself
>   cannot be eliminated as it doesn't have unique keys.
>
> == Testcase ==
>
>
> create table t20 (a int not null);
> create table t21 (a int not null primary key, js varchar(100));
>
> insert into t20 select seq from seq_1_to_100;
> insert into t21 select a, '{"a":100}' from t20;
>
> create table t31(a int);
> create table t32(b int);
> insert into t31 values (1);
> insert into t32 values (1);
>
> explain
> select
>   t20.a, jt1.ab
> from
>   t20
>   left join t21 on t20.a=t21.a
>   join
>   (t31 left join (t32 join JSON_TABLE(t21.js,'$' COLUMNS (ab INT PATH
> '$.a')) AS jt1) on t31.a<3);
>
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-17399: JSON_TABLE: Odd code in table.cc:create_view_field ?

[Alexey Botchkov]

Well i should have removed those changes after i did the last version
of the check for table dependencies. Now we never set the
thd->current_select to the 0.
So that part wrapper_to_set part deleted.

Best regards.
HF

On Fri, Mar 12, 2021 at 1:35 AM Sergey Petrunia  wrote:

> Hi Alexey,
>
> What does the code quoted below do? I don't recall seeing it on previous
> review
> iterations.
>
> In any case,
> * It needs a comment about why such special handling is needed.
> * It needs test coverage - I have reverted these changes and didn't see
> any test to fail?
>
> > diff --git a/sql/table.cc b/sql/table.cc
> > index 4f65dbd65f4..9c205fc4be6 100644
> > --- a/sql/table.cc
> > +++ b/sql/table.cc
> > @@ -6722,6 +6722,8 @@ Item *create_view_field(THD *thd, TABLE_LIST
> *view, Item **field_ref,
> >  LEX_CSTRING *name)
> >  {
> >bool save_wrapper= thd->lex->first_select_lex()->no_wrap_view_item;
> > +  bool *wrapper_to_set= thd->lex->current_select ?
> > +>lex->current_select->no_wrap_view_item : _wrapper;
> >Item *field= *field_ref;
> >DBUG_ENTER("create_view_field");
> >
> > @@ -6737,17 +6739,17 @@ Item *create_view_field(THD *thd, TABLE_LIST
> *view, Item **field_ref,
> >}
> >
> >DBUG_ASSERT(field);
> > -  thd->lex->current_select->no_wrap_view_item= TRUE;
> > +  *wrapper_to_set= TRUE;
> >if (!field->is_fixed())
> >{
> >  if (field->fix_fields(thd, field_ref))
> >  {
> > -  thd->lex->current_select->no_wrap_view_item= save_wrapper;
> > +  *wrapper_to_set= save_wrapper;
> >DBUG_RETURN(0);
> >  }
> >  field= *field_ref;
> >}
> > -  thd->lex->current_select->no_wrap_view_item= save_wrapper;
> > +  *wrapper_to_set= save_wrapper;
> >if (save_wrapper)
> >{
> >  DBUG_RETURN(field);
>
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-17399: JSON_TABLE: final input

[Alexey Botchkov]

forgot about the embedded server.
Fixed.

On Wed, Mar 10, 2021 at 12:47 PM Sergey Petrunia  wrote:

> Hi Alexey,
>
>
> On Sat, Mar 06, 2021 at 01:13:53AM +0400, Alexey Botchkov wrote:
> > Hi, Sergei!
> >
> > I pushed the patch to the feature branch for you to take a look.
>
>
> This broke CI, please fix:
>
>
> http://buildbot.askmonty.org/buildbot/builders/kvm-rpm-centos74-amd64/builds/18427/steps/compile/logs/stdio
>
> CMake Error at cmake/libutils.cmake:108 (ADD_LIBRARY):
>   Cannot find source file:
>
> ../sql/table_function.cc
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-17399: JSON_TABLE: final input

[Alexey Botchkov]

Hi, Sergei!

I pushed the patch to the feature branch for you to take a look.
The patch you proposed
http://lists.askmonty.org/pipermail/commits/2021-March/014492.html
 I liked and adapted with one exception. The nested paths list is built
using the **last_sibling_hook
instead of *cur_last_sibling. That seems to me nicer and doesn't produce
that many repeating lines
in the code.

Best regards.
HF



On Thu, Mar 4, 2021 at 3:11 PM Sergey Petrunia  wrote:

> Hi Alexey,
>
> Please find below final bits of input.  After these are addressed the patch
> will probably be good to push  (But I'll need a final pass before giving
> ok to
> push).
>
> == json_table_mysql* tests ==
>
> * Please remove one of the json_table_mysql and json_table_mysql2 tests.
> * Please move the remaining test to be in the same suite as
> json_table.test.
>
> == Fix json_table.test ==
>
> The test has this somewhere in the middle:
>
> set optimizer_switch='firstmatch=off';
>
> Please restore the original setting.
>
> == ha_json_table::rnd_next() returns 0 on error ==
>
> Consider this query:
>
> select * from
> json_table(
> '[
>   {"name": "X"},
>   {"name2": "Y"}
> ]',
> '$[*]' columns
> (
>   col1 varchar(100) path '$.name2' error on empty
> )) as T;
>
> The query produces an error.
> It happens like so:
>
> * the call to ha_json_table::rnd_next() returns 0.
> * The SQL layer finds out about the error by checking thd->is_error()
> which returns true.
>
> Please make ha_json_table::rnd_next() return an error.
>
> == Rename table_function.* ==
>
> Looking at the contets of sql/table_function.{h,cc}, one can see that
> there's
> very little code there that is applicable to generic table function.
> Almost
> all code is specifi to JSON_TABLE.
> Because of that, please rename the files to e.g. json_table.{h,cc}.
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-17399: JSON_TABLE: Crash with nested path

[Alexey Botchkov]

Found one more ps-protocol issue to fix...

On Thu, Feb 25, 2021 at 5:27 PM Sergey Petrunia  wrote:

> Hi Alexey,
>
> I was looking at Json_table_nested_path::set_position(), wondering why
> does
> it have an assignment
>
>   np->m_null= TRUE;
>
> but doesn't clear the NULL values and trying to come up with an example of
> this
> going wrong when I've hit this crash:
>
> select * from
> json_table(
> '[
>   {"name": "X",
> "colors":["blue"], "sizes": [1,2,3,4],  "prices" : [10,20]},
>   {"name": "Y",
> "colors":["red"], "sizes": [10,11],  "prices" : [100,200,300]}
> ]',
> '$[*]' columns
> (
>   seq0 for ordinality,
>   name varchar(4) path '$.name',
>   nested path '$.colors[*]' columns (
> seq1 for ordinality,
> color text path '$'
>   ),
>   nested path '$.sizes[*]' columns (
> seq2 for ordinality,
> size int path '$'
>   ),
>   nested path '$.prices[*]' columns (
> seq3 for ordinality,
> price int path '$'
>   )
> )
> ) as T order by seq0, name;
>
> Note this==NULL:
>
> (gdb) wher
>   #0  0x560edf72 in Json_table_nested_path::set_position
> (this=0x0, j_start=0x7ffeb0016e68 "[ \n  {\"name\": \"X\", \n
> \"colors\":[\"blue\"], \"sizes\": [1,2,3,4],  \"prices\" : [10,20]},\n
> {\"name\": \"Y\", \n\"colors\":[\"red\"], \"sizes\": [10,11],
> \"prices\" : [100,200,300]}\n]", j_end=0x7ffeb0016f12 "",
> pos=0x7ffeb0035e51 "\245\245\245\245\245\245\245\245\006") at
> /home/psergey/dev-git2/10.6-hf-review6/sql/table_function.cc:239
>   #1  0x560ee12f in Json_table_nested_path::set_position
> (this=0x7ffeb0017060, j_start=0x7ffeb0016e68 "[ \n  {\"name\": \"X\", \n
> \"colors\":[\"blue\"], \"sizes\": [1,2,3,4],  \"prices\" : [10,20]},\n
> {\"name\": \"Y\", \n\"colors\":[\"red\"], \"sizes\": [10,11],
> \"prices\" : [100,200,300]}\n]", j_end=0x7ffeb0016f12 "",
> pos=0x7ffeb0035e48 "") at
> /home/psergey/dev-git2/10.6-hf-review6/sql/table_function.cc:262
>   #2  0x560ee9f0 in ha_json_table::rnd_pos (this=0x7ffeb0014f00,
> buf=0x7ffeb0025570 "\377", pos=0x7ffeb0035e48 "") at
> /home/psergey/dev-git2/10.6-hf-review6/sql/table_function.cc:434
>   #3  0x561ca6a4 in handler::ha_rnd_pos (this=0x7ffeb0014f00,
> buf=0x7ffeb0025570 "\377", pos=0x7ffeb0035e48 "") at
> /home/psergey/dev-git2/10.6-hf-review6/sql/handler.cc:3101
>   #4  0x563852e3 in rr_from_pointers (info=0x7ffeb001f9e0) at
> /home/psergey/dev-git2/10.6-hf-review6/sql/records.cc:615
>   #5  0x55da4a75 in READ_RECORD::read_record (this=0x7ffeb001f9e0)
> at /home/psergey/dev-git2/10.6-hf-review6/sql/records.h:81
>   #6  0x55ee1876 in join_init_read_record (tab=0x7ffeb001f918) at
> /home/psergey/dev-git2/10.6-hf-review6/sql/sql_select.cc:21644
>   #7  0x55edf35a in sub_select (join=0x7ffeb001d948,
> join_tab=0x7ffeb001f918, end_of_records=false) at
> /home/psergey/dev-git2/10.6-hf-review6/sql/sql_select.cc:20666
>   #8  0x55ede8e6 in do_select (join=0x7ffeb001d948, procedure=0x0)
> at /home/psergey/dev-git2/10.6-hf-review6/sql/sql_select.cc:20216
>   #9  0x55eb24e7 in JOIN::exec_inner (this=0x7ffeb001d948) at
> /home/psergey/dev-git2/10.6-hf-review6/sql/sql_select.cc:4484
>   #10 0x55eb1613 in JOIN::exec (this=0x7ffeb001d948) at
> /home/psergey/dev-git2/10.6-hf-review6/sql/sql_select.cc:4264
>
> Please fix.
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-17399: JSON_TABLE: Crash with nested path

[Alexey Botchkov]

Things are fixed now as far as i can see.

Best regards.
HF


On Thu, Feb 25, 2021 at 5:27 PM Sergey Petrunia  wrote:

> Hi Alexey,
>
> I was looking at Json_table_nested_path::set_position(), wondering why
> does
> it have an assignment
>
>   np->m_null= TRUE;
>
> but doesn't clear the NULL values and trying to come up with an example of
> this
> going wrong when I've hit this crash:
>
> select * from
> json_table(
> '[
>   {"name": "X",
> "colors":["blue"], "sizes": [1,2,3,4],  "prices" : [10,20]},
>   {"name": "Y",
> "colors":["red"], "sizes": [10,11],  "prices" : [100,200,300]}
> ]',
> '$[*]' columns
> (
>   seq0 for ordinality,
>   name varchar(4) path '$.name',
>   nested path '$.colors[*]' columns (
> seq1 for ordinality,
> color text path '$'
>   ),
>   nested path '$.sizes[*]' columns (
> seq2 for ordinality,
> size int path '$'
>   ),
>   nested path '$.prices[*]' columns (
> seq3 for ordinality,
> price int path '$'
>   )
> )
> ) as T order by seq0, name;
>
> Note this==NULL:
>
> (gdb) wher
>   #0  0x560edf72 in Json_table_nested_path::set_position
> (this=0x0, j_start=0x7ffeb0016e68 "[ \n  {\"name\": \"X\", \n
> \"colors\":[\"blue\"], \"sizes\": [1,2,3,4],  \"prices\" : [10,20]},\n
> {\"name\": \"Y\", \n\"colors\":[\"red\"], \"sizes\": [10,11],
> \"prices\" : [100,200,300]}\n]", j_end=0x7ffeb0016f12 "",
> pos=0x7ffeb0035e51 "\245\245\245\245\245\245\245\245\006") at
> /home/psergey/dev-git2/10.6-hf-review6/sql/table_function.cc:239
>   #1  0x560ee12f in Json_table_nested_path::set_position
> (this=0x7ffeb0017060, j_start=0x7ffeb0016e68 "[ \n  {\"name\": \"X\", \n
> \"colors\":[\"blue\"], \"sizes\": [1,2,3,4],  \"prices\" : [10,20]},\n
> {\"name\": \"Y\", \n\"colors\":[\"red\"], \"sizes\": [10,11],
> \"prices\" : [100,200,300]}\n]", j_end=0x7ffeb0016f12 "",
> pos=0x7ffeb0035e48 "") at
> /home/psergey/dev-git2/10.6-hf-review6/sql/table_function.cc:262
>   #2  0x560ee9f0 in ha_json_table::rnd_pos (this=0x7ffeb0014f00,
> buf=0x7ffeb0025570 "\377", pos=0x7ffeb0035e48 "") at
> /home/psergey/dev-git2/10.6-hf-review6/sql/table_function.cc:434
>   #3  0x561ca6a4 in handler::ha_rnd_pos (this=0x7ffeb0014f00,
> buf=0x7ffeb0025570 "\377", pos=0x7ffeb0035e48 "") at
> /home/psergey/dev-git2/10.6-hf-review6/sql/handler.cc:3101
>   #4  0x563852e3 in rr_from_pointers (info=0x7ffeb001f9e0) at
> /home/psergey/dev-git2/10.6-hf-review6/sql/records.cc:615
>   #5  0x55da4a75 in READ_RECORD::read_record (this=0x7ffeb001f9e0)
> at /home/psergey/dev-git2/10.6-hf-review6/sql/records.h:81
>   #6  0x55ee1876 in join_init_read_record (tab=0x7ffeb001f918) at
> /home/psergey/dev-git2/10.6-hf-review6/sql/sql_select.cc:21644
>   #7  0x55edf35a in sub_select (join=0x7ffeb001d948,
> join_tab=0x7ffeb001f918, end_of_records=false) at
> /home/psergey/dev-git2/10.6-hf-review6/sql/sql_select.cc:20666
>   #8  0x55ede8e6 in do_select (join=0x7ffeb001d948, procedure=0x0)
> at /home/psergey/dev-git2/10.6-hf-review6/sql/sql_select.cc:20216
>   #9  0x55eb24e7 in JOIN::exec_inner (this=0x7ffeb001d948) at
> /home/psergey/dev-git2/10.6-hf-review6/sql/sql_select.cc:4484
>   #10 0x55eb1613 in JOIN::exec (this=0x7ffeb001d948) at
> /home/psergey/dev-git2/10.6-hf-review6/sql/sql_select.cc:4264
>
> Please fix.
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-17399: JSON_TABLE, commit 85757ecbef44484270e385a8d52998c67f72d11a

[Alexey Botchkov]

Ok, fixed that as you said.
See the last commit to the bb-10.6-mdev17399-hf for details

Best regards.
HF

On Tue, Feb 23, 2021 at 3:57 PM Sergey Petrunia  wrote:

> On Mon, Feb 22, 2021 at 09:59:54PM +0300, Sergey Petrunia wrote:
> > == On table dependencies: table elimination ==
> >
> > create table t20 (a int not null);
> > create table t21 (a int not null primary key, js varchar(100));
> >
> > insert into t20 values (1),(2);
> > insert into t21 values (1, '{"a":100}');
> >
> > explain
> > select
> >   t20.a, jt1.ab
> > from
> >   t20
> >   left join t21 on t20.a=t21.a
> >   join JSON_TABLE(t21.js,'$' COLUMNS (ab INT PATH '$.a')) AS jt1;
> >
> >
> +--+-+---+--+---+--+-+--+--++
> > | id   | select_type | table | type | possible_keys | key  | key_len |
> ref  | rows | Extra  |
> >
> +--+-+---+--+---+--+-+--+--++
> > |1 | SIMPLE  | t20   | ALL  | NULL  | NULL | NULL|
> NULL | 2||
> > |1 | SIMPLE  | jt1   | ALL  | NULL  | NULL | NULL|
> NULL | 40   | Table function: json_table |
> >
> +--+-+---+--+---+--+-+--+--++
> >
> > Here we can see an apparently invalid query plan: table t21 was
> eleminated
> > even if JSON_TABLE uses it.
>
> We can do with a simple rule: "do not eliminate tables that are used as
> argument for any table function".
>
> I think this should be achieved as follows: in eliminate_tables(), look
> at the code that collects a bitmap of tables used in the select list (to
> prevent them from being eliminated).
>
>   /* Add tables referred to from the select list */
>   List_iterator it(join->fields_list);
>   while ((item= it++))
> used_tables |= item->used_tables();
>
> Right below this, add a loop which does the same for table functions.
>
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-17399: Columns of JSON datatype?

[Alexey Botchkov]

>  What do you think?

Returning the JSON subdocument would violate the standard. I think it makes
sense as
the user normally expects the scalar value and if there is the JSON
subdocument in the
searched field, it's rather an error and must be detected.

I'd add the FORMAT JSON keyword to handle this explicitly.
The Oracle implementation of the FORMAT JSON seems weird to me as it
returns NULLs
on the scalar values instead. Didn't decide to myself though if it's the
standard way to handle it.
The scalar value is actually the valid json.

Regards.
HF




On Thu, Feb 4, 2021 at 2:44 PM Sergey Petrunia  wrote:

> Hi Alexey,
>
> Consider this:
>
> select * from
> json_table(
>   '{"a":[1,2]}',
>   '$' columns (
>  jsn_path json path '$.a' default '{}' on empty
> )
> ) as tt;
>
> MySQL produces:
> +--+
> | jsn_path |
> +--+
> | [1, 2]   |
> +--+
>
> MariaDB produces:
> +--+
> | jsn_path |
> +--+
> | NULL |
> +--+
>
> As far as I understand, MySQL is extending the standard here.
> The standard specifies that one can extract JSON subdocuments with "FORMAT
> JSON" syntax:
>
>  ::=
>
> FORMAT 
> [ PATH  ]
>
> as opposed to regular:
>
>  ::=
>
>   [ PATH  ]
>
> OracleDB accepts FORMAT JSON clause:
> https://docs.oracle.com/database/121/SQLRF/functions092.htm#SQLRF56973
>
> select * from
> json_table(
>   '{"a":[1,2]}',
>   '$' columns (
>  jsn_path varchar(100) format json path '$.a'
> )
> ) as tt;
>
> produces the same output as MySQL does.
>
> I think, MariaDB's current behavior - produce NULL when path points to a
> JSON
> sub-document, produce invalid JSON when the path points to a constant - is
> not acceptable.
>
> I'm fine if we just disable this and return ER_NOT_IMPLEMENTED.
>
> Alternatively, we could return JSON subdocuments like MySQL does.
>
> What do you think?
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-17399: Name resolution and handling dependencies

[Alexey Botchkov]

Well i just did that end_lateral_table trick for now.

HF


On Tue, Feb 9, 2021 at 3:05 PM Sergey Petrunia  wrote:

> Hi Alexey,
>
> At the moment, name resolution of JSON_TABLE's first argument is done
> "like in
> the WHERE clause" - one can refer to any table that is defined in the WHERE
> clause.
>
> This allows one to write queries where JSON_TABLE tables have incorrect
> dependencies
> - circular
> - dependency that contradicts the dependency imposed by OUTER JOIN
> - dependency that contradicts STRAIGHT_JOIN
> (WRONG-DEPS)
>
> the patch checks for these cases but I've reported cases where it fails.
>
> I haven't been find any statement in the SQL Standard about this, but it
> makes
> a statement about similar constructs, table functions and LATERAL
> subqueries:
>
>
> https://jira.mariadb.org/browse/MDEV-17399?focusedCommentId=179327=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-179327
>
> Other databases seem to apply the same limitation to JSON_TABLE's argument.
>
> When they do it, this automatically fixes all the wrong-dependency issues -
> a query with (WRONG-DEPS) is rejected at the name resolution phase.
>
> I think we should follow this and modify name resolution process to work
> in the
> same way.
>
> The way MySQL did it is described here:
> https://dev.mysql.com/worklog/task/?id=8867,
> LLD, grep for end_lateral_table. I'm not fond of having implicit parameters
> (end_lateral_table) which change they way name resolution works, but I
> think
> it is an acceptable solution (and we already have other such parameters).
>
> (An alternative option would be to have items in JSON_TABLE's first
> argument to
> use their own Name_resolution_context object that would specify the correct
> first/last table they should look at. This seems to be much harder to do).
>
> What do you think?
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-17399 New patch for JSON_TABLE

[Alexey Botchkov]

Some updates were pushed.
See here for the review.
https://github.com/MariaDB/server/commit/d7ff97bd1343a94430152389f90c4d08f56fce44


On Wed, Nov 18, 2020 at 8:17 PM Sergey Petrunia  wrote:

> On Mon, Nov 16, 2020 at 01:53:39PM +0300, Sergey Petrunia wrote:
> > Input on the latest patch for MDEV-17399.
> ..
> > Please find the first few cases below.
>
> And a few more from the same source:
>
> === JSON_TABLE cannot depend on another one ? ===
>
> drop table t1;
> CREATE TABLE t1(id int, jd varchar(100));
>
> SELECT id, jt1.*, jt2.*
> FROM
>   t1,
>   JSON_TABLE(jd, '$' COLUMNS (data1 JSON PATH '$')) AS jt1,
>   JSON_TABLE(data1, '$[*]' COLUMNS (id2 INT PATH '$')) AS jt2;
>
> ERROR 1054 (42S22): Unknown column 'data1' in 'JSON_TABLE argument'
>
>
> === Dependency caused by STRAIGHT_JOIN is not visible? ===
> This one is interesting: it produces an error in MySQL but crashes MariaDB
> due
> to unability to pick a query plan.
>
> This is a surprise for me, too. I assumed join order dependencies created
> by
> STRAIGHT_JOIN are visible in table dependency map...
>
> drop table t1;
> CREATE TABLE t1(id INT, f1 JSON);
> INSERT INTO t1 VALUES
>  (1, '{\"1\": 1}'),
>  (2, '{\"1\": 2}'),
>  (3, '{\"1\": 3}'),
>  (4, '{\"1\": 4}'),
>  (5, '{\"1\": 5}'),
>  (6, '{\"1\": 6}');
> ANALYZE TABLE t1;
>
> SELECT * FROM t1 as jj1,
>   (SELECT tt2.*
>FROM
>   t1 as tt2,
>   JSON_TABLE(tt3.f1, "$" COLUMNS (id FOR ORDINALITY)) AS tbl
>   STRAIGHT_JOIN
>   t1 AS tt3
>) dt
> ORDER BY 1,3 LIMIT 10;
>
> === COLLATE clause is not supported ===
>
> This fails with an error:
>
> CREATE TABLE t2 SELECT *
>   FROM JSON_TABLE('"test"', '$' COLUMNS(x VARCHAR(10)
> CHARSET utf8mb4 COLLATE utf8mb4_bin
> PATH '$')) AS jt1;
>
> I am not sure if we need to support it (I saw the change_charset() call in
> the
> patch). What's your opinion?
>
> === CHARSET is accepted but ignored ===
> select collation(x) from
>   JSON_TABLE(
> '["abc"]',
> '$[*]' COLUMNS (x VARCHAR(10) CHARSET latin1 PATH '$')
>   ) tbl;
> +-+
> | collation(x)|
> +-+
> | utf8_general_ci |
> +-+
>
> If we don't support columns having different charset, this should not be
> allowed.
>
> === Character set introducers are not supported ===
>
> They are supported in the first parameter, but not in the path or default
> clauses.
>
> SELECT * FROM
>   JSON_TABLE(JSON_OBJECT(),
>  _utf8mb4'$' COLUMNS (NESTED PATH _utf8mb4'$.x' COLUMNS
>(y INT PATH _utf8mb4'$.y'
>   DEFAULT _utf8mb4'1' ON EMPTY
>   DEFAULT _utf8mb4'2' ON ERROR))) jt;
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-17399 New patch for JSON_TABLE

[Alexey Botchkov]

Hello, Sergei!

New patch was made.
I think i addressed what you've noticed in last review.
https://github.com/MariaDB/server/commit/75ae330f877240e903690f282fad3ebdb02a0eeb

> CREATE TABLE t2 SELECT *
 >  FROM JSON_TABLE('"test"', '$' COLUMNS(x VARCHAR(10)
> CHARSET utf8mb4 COLLATE utf8mb4_bin
> PATH '$')) AS jt1;
> I am not sure if we need to support it (I saw the change_charset() call
in the
> patch). What's your opinion?

I decided we should. Not difficult to implement and some can need it.

> This is a surprise for me, too. I assumed join order dependencies created
by
> STRAIGHT_JOIN are visible in table dependency map...

I fixed it in the Table_function_json_table::setup so it doesn't allow
dependencies to the STRAIGHT_JOIN-ed table. But probably should
be fixed in more general way.


A.


On Wed, Nov 18, 2020 at 8:17 PM Sergey Petrunia  wrote:

> On Mon, Nov 16, 2020 at 01:53:39PM +0300, Sergey Petrunia wrote:
> > Input on the latest patch for MDEV-17399.
> ..
> > Please find the first few cases below.
>
> And a few more from the same source:
>
> === JSON_TABLE cannot depend on another one ? ===
>
> drop table t1;
> CREATE TABLE t1(id int, jd varchar(100));
>
> SELECT id, jt1.*, jt2.*
> FROM
>   t1,
>   JSON_TABLE(jd, '$' COLUMNS (data1 JSON PATH '$')) AS jt1,
>   JSON_TABLE(data1, '$[*]' COLUMNS (id2 INT PATH '$')) AS jt2;
>
> ERROR 1054 (42S22): Unknown column 'data1' in 'JSON_TABLE argument'
>
>
> === Dependency caused by STRAIGHT_JOIN is not visible? ===
> This one is interesting: it produces an error in MySQL but crashes MariaDB
> due
> to unability to pick a query plan.
>
> This is a surprise for me, too. I assumed join order dependencies created
> by
> STRAIGHT_JOIN are visible in table dependency map...
>
> drop table t1;
> CREATE TABLE t1(id INT, f1 JSON);
> INSERT INTO t1 VALUES
>  (1, '{\"1\": 1}'),
>  (2, '{\"1\": 2}'),
>  (3, '{\"1\": 3}'),
>  (4, '{\"1\": 4}'),
>  (5, '{\"1\": 5}'),
>  (6, '{\"1\": 6}');
> ANALYZE TABLE t1;
>
> SELECT * FROM t1 as jj1,
>   (SELECT tt2.*
>FROM
>   t1 as tt2,
>   JSON_TABLE(tt3.f1, "$" COLUMNS (id FOR ORDINALITY)) AS tbl
>   STRAIGHT_JOIN
>   t1 AS tt3
>) dt
> ORDER BY 1,3 LIMIT 10;
>
> === COLLATE clause is not supported ===
>
> This fails with an error:
>
> CREATE TABLE t2 SELECT *
>   FROM JSON_TABLE('"test"', '$' COLUMNS(x VARCHAR(10)
> CHARSET utf8mb4 COLLATE utf8mb4_bin
> PATH '$')) AS jt1;
>
> I am not sure if we need to support it (I saw the change_charset() call in
> the
> patch). What's your opinion?
>
> === CHARSET is accepted but ignored ===
> select collation(x) from
>   JSON_TABLE(
> '["abc"]',
> '$[*]' COLUMNS (x VARCHAR(10) CHARSET latin1 PATH '$')
>   ) tbl;
> +-+
> | collation(x)|
> +-+
> | utf8_general_ci |
> +-+
>
> If we don't support columns having different charset, this should not be
> allowed.
>
> === Character set introducers are not supported ===
>
> They are supported in the first parameter, but not in the path or default
> clauses.
>
> SELECT * FROM
>   JSON_TABLE(JSON_OBJECT(),
>  _utf8mb4'$' COLUMNS (NESTED PATH _utf8mb4'$.x' COLUMNS
>(y INT PATH _utf8mb4'$.y'
>   DEFAULT _utf8mb4'1' ON EMPTY
>   DEFAULT _utf8mb4'2' ON ERROR))) jt;
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] MDEV-13467 review.

[Alexey Botchkov]

Hi, Anel!

Sorry for that long delay with this review. Had real difficult weeks of my
life :(
Now it seems to be over, so getting back to normal.
Below is my opinions of the code i see in the branch
bb-10.1-anel-MDEV-13467-gis-feature-st_sphere-v2
Hope it's the recent version.

+  Adapter for functions that takes two or three arguments.
+*/
+
+class Create_func_arg2_or_3 : public Create_func
+{
Do you think we need the separate Create_fund_arg2_or_3 class?
If you don't see any other class to inherit from it,
i'd recommend to leave the Create_func_distance_sphere alone.
No need to have two constructors in the Item_func_sphere_distance
too. Just use the list argument as it's done for the
Item_func_json_contains (in 10.2).


Anyway i belive we should get rid of duplicating code
about the 'param1' and 'param2'. They can be popped/checked
with no if (arg_count == 2) condition.

+  double distance= 0.0;
+  null_value= (args[0]->null_value || args[1]->null_value);
+  if (null_value)
+  {
+// Returned item must be set to Null
+DBUG_ASSERT(maybe_null);
   don't think it's needed.
+return 0;
Should be return 0.0 i belive. Or goto handle_errors.
Thet goes to all but one 'return' statements in this function.

+  }

> ...
> if (!arg1 || !arg2)

This last condition should never happen since we have not-null_value.
So i'd remove that 'if' section completely.
And anyway it should do the 'return' in that branch.

+null_value= args[2]->null_value;
+if (null_value)
+{
+  return 0;
+}

seems nicer to me this way
if (args[2]->null_value)
{
  null_value= true;
  goto handle_errors;
}

+  if (!(g1= Geometry::construct(, arg1->ptr(), arg1->length())) ||
+  !(g2= Geometry::construct(, arg2->ptr(), arg2->length(
+  {
+ goto handle_errors;

we should launch the ER_GIS_INVALID_DATA here.

+// Generate error message in case different geometry is used?
Yes, i think the explainig message here is good to have.

+double Item_func_sphere_distance::spherical_distance(Geometry *g1,
+ Geometry *g2,
+ const double
sphere_radius)
+double Item_func_sphere_distance::spherical_distance_points(Geometry *g1,
+Geometry *g2,
+const double r)
Why these two are member static functions, not simply static?
If you plan any 'external' use for them, please add comments about
what exacly they do.

+double Item_func_sphere_distance::spherical_distance(Geometry *g1,
+ Geometry *g2,
+ const double
sphere_radius)
+{
+  double res= 0.0;
+  switch (g1->get_class_info()->m_type_id)
+  {
+case Geometry::wkb_point:
+  res= spherical_distance_points(g1, g2, sphere_radius);
+  break;
+
+case Geometry::wkb_multipoint:
+  res= spherical_distance_points(g1, g2, sphere_radius);
+  break;

Why do we need this function?
We confirmed already that the type is either wkb_point or wkb_multipolygon,
so just can call spherical_distance_points. No?

>


+class Item_func_sphere_distance: public Item_real_func
+{
+  double sphere_radius= 6370986.0; // Default radius equals Earth radius

Some compilers don't like this syntax, and we didn't use it yet, so
i'd recomment to move this to the constructor or even better to the
::val_real().

+double Gis_point::calculate_haversine(const Geometry *g,
+const double sphere_radius)
+{
+  DBUG_ASSERT(sphere_radius > 0);
+  double x1r= 0.0;
+  double x2r= 0.0;
+  double y1r= 0.0;
+  double y2r= 0.0;
That lot of unnecessary initializations seems excessive.

+  if (g->get_class_info()->m_type_id == Geometry::wkb_multipoint)
+  {
+const char point_size= 4 + WKB_HEADER_SIZE + POINT_DATA_SIZE+1; //1
for the type
+char point_temp[point_size];
+memset(point_temp+4, Geometry::wkb_point, 1);
+memcpy(point_temp+5, static_cast(g)->get_data_ptr()+5, 4);
+memcpy(point_temp+4+WKB_HEADER_SIZE,
g->get_data_ptr()+4+WKB_HEADER_SIZE,
+   POINT_DATA_SIZE);
+point_temp[point_size-1]= '\0';
+Geometry_buffer gbuff;
+Geometry *gg= Geometry::construct(, point_temp, point_size-1);

There is the Gis_multi_point::geometry_n() function, should be used here to
get the point.

I don't like that lot of static_cast-s.
Isn't it nicer to have just the function of
double calculate_harvesine_points(Geometry *p1, Geometry *p2, double radius)
double calculate_harvesine_multipoint_point(Geometry *mp, Geometry *p,
double radius)
double calculate_harvesine_multipoint_multipoint(Geometry *mp1, Geometry
*mp2, double radius)
 which would call each other?

The ::get_xy_radian() method looks unneeded to me.
It's enough to have the to_radian(double d) function.

BTW it's possible to do the 

Re: [Maria-developers] Question about JSONPath and '**' wildcard

[Alexey Botchkov]

  On the second thought, i decided to do it like MySQL does.
There is a value of "124" and it's patch fits the specification, so we
return it.
No need to do it twice.

Are you ok with that?
HF/

On Fri, Nov 20, 2020 at 12:18 AM Sergey Petrunia  wrote:

> Hi Alexey,
>
> I've found this discrepancy in JSONPath evaluation:
>
>
> set @json_doc3 =
> '
> {
>   "root": {
>  "child1" : {
>"child2" : {
>  "child1" : {
>"x":124
>   }
> }
>   }
>}
> }
> ';
>
> select json_extract(@json_doc3, '$**.child1**.x');
>
> MariaDB [test]> select json_extract(@json_doc3, '$**.child1**.x');
> ++
> | json_extract(@json_doc3, '$**.child1**.x') |
> ++
> | NULL   |
> ++
> 1 row in set (0.001 sec)
>
> MySQL-8> select json_extract(@json_doc3, '$**.child1**.x');
> ++
> | json_extract(@json_doc3, '$**.child1**.x') |
> ++
> | [124]  |
> ++
> 1 row in set (0.00 sec)
>
> Which one is right?
>
> My opinion is that MariaDB's answer is definitely incorrect.
>
> As for MySQL's answer, it depends on how the result of JSONPath expression
> is
> defined.
>
> If it is "a set of nodes in the JSON document which match the pattern",
> MySQL's
> result is correct.
>
> Coming from XPath world, I was expecting a semantics in form
>
> find the set of nodes matching the search step#1 ( $**,
>   then apply step #2 ( .child1)
> then apply step #3 ( any children of those)
>   ...
>
> in which case "124" would be in the result twice as it is reachable via two
> possible paths.
>
> Any thoughts about this?
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Question about JSONPath and '**' wildcard

[Alexey Botchkov]

Hi, Sergey.

To me the XPath version seems correct here. So both we and MySQL are wrong.
Though the JSON_Extract function is the MySQL-s invention so it's tempting
to
make it working like they do.
So i'm in doubt how to fix this.

A.

On Fri, Nov 20, 2020 at 12:18 AM Sergey Petrunia  wrote:

> Hi Alexey,
>
> I've found this discrepancy in JSONPath evaluation:
>
>
> set @json_doc3 =
> '
> {
>   "root": {
>  "child1" : {
>"child2" : {
>  "child1" : {
>"x":124
>   }
> }
>   }
>}
> }
> ';
>
> select json_extract(@json_doc3, '$**.child1**.x');
>
> MariaDB [test]> select json_extract(@json_doc3, '$**.child1**.x');
> ++
> | json_extract(@json_doc3, '$**.child1**.x') |
> ++
> | NULL   |
> ++
> 1 row in set (0.001 sec)
>
> MySQL-8> select json_extract(@json_doc3, '$**.child1**.x');
> ++
> | json_extract(@json_doc3, '$**.child1**.x') |
> ++
> | [124]  |
> ++
> 1 row in set (0.00 sec)
>
> Which one is right?
>
> My opinion is that MariaDB's answer is definitely incorrect.
>
> As for MySQL's answer, it depends on how the result of JSONPath expression
> is
> defined.
>
> If it is "a set of nodes in the JSON document which match the pattern",
> MySQL's
> result is correct.
>
> Coming from XPath world, I was expecting a semantics in form
>
> find the set of nodes matching the search step#1 ( $**,
>   then apply step #2 ( .child1)
> then apply step #3 ( any children of those)
>   ...
>
> in which case "124" would be in the result twice as it is reachable via two
> possible paths.
>
> Any thoughts about this?
>
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] New patch on the JSON_TABLE.

[Alexey Botchkov]

Hi again, Sergey!

So i pumped out the patch where all mentioned problems were fixed.
The branch name bb-17399-hf.
Including that optimizer problem and the OPTIMIZER_TRACE

So loo



On Wed, Sep 23, 2020 at 5:06 PM Sergey Petrunia  wrote:

> Hi Alexey,
>
> On Mon, Sep 14, 2020 at 03:38:38PM +0400, Alexey Botchkov wrote:
> > Hi, Sergei!.
> >
> > so the branch name is 'bb-mdef17399-hf'.
> > It has slightly changed since last patch so please pay attention.
> >
> > There i present the patch that resolves issues that you mentioned
> > in your last comments.
> > One important is not fixed though.
> > That is:
> > CREATE TABLE t1(id INT, f1 JSON);
> > INSERT INTO t1 VALUES (1, '{\"1\": 1}'), (2, '{\"1\": 2}'), (3, '{\"1\":
> > 3}'),
> > (4, '{\"1\": 4}'), (5, '{\"1\": 5}'), (6, '{\"1\": 6}');
> >
> > SELECT * FROM t1 WHERE id IN
> >   (SELECT id FROM t1 as tt2,
> >   JSON_TABLE(f1, "$" COLUMNS (jf FOR ORDINALITY)) AS tbl);
> >
> > That SELECT crashes in the optimizer, as the greedy_search() can't find
> any
> > satisfying plan.
> > I got rid of the crash with this line:
> > @@ -9520,7 +9528,7 @@ best_extension_by_limited_search(JOIN  *join,
> >  table_map real_table_bit= s->table->map;
> >  if ((remaining_tables & real_table_bit) &&
> >  (allowed_tables & real_table_bit) &&
> > -!(remaining_tables & s->dependent) &&
> > +//!(remaining_tables & s->dependent) &&
> >
> > But i don't mean it's an acceptable fix.
> >
> > It seems to me the problem is in the optimizer, so i'd like to ask your
> > opinion.
> > Maybe you just know the answer at once.
>
> After some debugging I see that the following happens:
>
> 1. Execution reaches Table_function_json_table::setup
>
> It computes
>
>m_dep_tables= m_json->used_tables();
>
> this is =1 (In the subquery, table tt2 has map=1)
>
> 2. Then, subquery is converted into a semi-join. That is, it is merged
> into its
> parent subquery. There, we have:
>
> table t1: map=1
> table tt2:  map=2
> table JSON_TABLE(...) AS tbl: map=3
>
> 3.  The subquery was uncorrelated, so SJ-Materialization is a possible
> option and
> the join optimizer attempts to construct a join order for the subquery
> tables.
>
> However it can't succeed, as the table "JSON_TABLE(..) AS tbl" is set to be
> dependent on table with map=1, which is not put into the join order
> because it
> is not a part of subquery.
> As a result, we get assertion failure when the optimizer fails to produce
> any
> join orders.
>
> The probem here is on step #2.
> When the subquery's tables are moved to the parent query, they get new
> values
> of TABLE::map, and also all their attributes that contain table maps need
> to
> be updated.
>
> This done here in convert_subq_to_sj() function:
>
> ```
>   /* n. Walk through child's tables and adjust table->map */
>   List_iterator_fast si(subq_lex->leaf_tables);
>   while ((tl= si++))
>   {
> tl->set_tablenr(table_no);
> if (tl->is_jtbm())
> {
>   tl->jtbm_table_no= table_no;
>   Item *dummy= tl->jtbm_subselect;
>   tl->jtbm_subselect->fix_after_pullout(parent_lex, , true);
> ```
>
> In that code one can see: if the subquery has table that has
> 'is_jtbm()==true',
> then ... some processing is done to re-compute its attributes after the
> table
> has been pulled up into the parent select.
>
> The same should be done for TABLE_LIST elements that are JSON_TABLE(..).
>
> If you'll introduce a new function, fix_after_pullout() is a good name. It
> is
> already used for Item-derived classes.
>
> > If not, i'll dig into it.
> > And you're welcome to observe the new patch.
> >
> > Best regards.
> > HF
>
> --
> BR
>  Sergei
> --
> Sergei Petrunia, Software Developer
> MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog
>
>
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] MDEV-16166 RBR breaks with HA_ERR_KEY_NOT_FOUND upon DELETE

[Alexey Botchkov]

Ok to push.

05.06.2018 13:45, Sachin Setiya wrote:

commit bde95bc70d0ac011f55f1b3b55fee1c9276859da
Author: sachin >

Date:   Tue Jun 5 15:14:19 2018 +0530

    MDEV-16166 RBR breaks with HA_ERR_KEY_NOT_FOUND upon DELETE from 
table...

    with spatial index
    So the issue is since it is spatial index , at the time of 
searching index

    for key (Rows_log_event::find_row) we use wrong field image we use
    Field::itRAW while we should be using Field::itMBR

diff --git a/mysql-test/suite/rpl/r/rpl_row_spatial.result 
b/mysql-test/suite/rpl/r/rpl_row_spatial.result

new file mode 100644
index 000..8f546fc479e
--- /dev/null
+++ b/mysql-test/suite/rpl/r/rpl_row_spatial.result
@@ -0,0 +1,14 @@
+include/master-slave.inc
+[connection master]
+CREATE TABLE t1 (g POINT NOT NULL, SPATIAL INDEX(g));
+INSERT INTO t1 VALUES (ST_GEOMFROMTEXT('Point(1 1)'));
+INSERT INTO t1 VALUES (ST_GEOMFROMTEXT('Point(2 1)'));
+INSERT INTO t1 VALUES (ST_GEOMFROMTEXT('Point(1 2)'));
+INSERT INTO t1 VALUES (ST_GEOMFROMTEXT('Point(2 2)'));
+DELETE FROM t1 where MBREqual(g, ST_GEOMFROMTEXT('Point(1 2)'));
+select count(*) from t1;
+count(*)
+3
+DELETE FROM t1;
+drop table t1;
+include/rpl_end.inc
diff --git a/mysql-test/suite/rpl/t/rpl_row_spatial.test 
b/mysql-test/suite/rpl/t/rpl_row_spatial.test

new file mode 100644
index 000..00c3dd7c54d
--- /dev/null
+++ b/mysql-test/suite/rpl/t/rpl_row_spatial.test
@@ -0,0 +1,17 @@
+--source include/have_binlog_format_row.inc
+--source include/master-slave.inc
+
+CREATE TABLE t1 (g POINT NOT NULL, SPATIAL INDEX(g));
+INSERT INTO t1 VALUES (ST_GEOMFROMTEXT('Point(1 1)'));
+INSERT INTO t1 VALUES (ST_GEOMFROMTEXT('Point(2 1)'));
+INSERT INTO t1 VALUES (ST_GEOMFROMTEXT('Point(1 2)'));
+INSERT INTO t1 VALUES (ST_GEOMFROMTEXT('Point(2 2)'));
+DELETE FROM t1 where MBREqual(g, ST_GEOMFROMTEXT('Point(1 2)'));
+
+--sync_slave_with_master
+select count(*) from t1;
+
+--connection master
+DELETE FROM t1;
+drop table t1;
+--source include/rpl_end.inc
diff --git a/sql/key.cc b/sql/key.cc
index 700bf6a05a6..7e5a3309b10 100644
--- a/sql/key.cc
+++ b/sql/key.cc
@@ -145,7 +145,8 @@ void key_copy(uchar *to_key, uchar *from_record, 
KEY *key_info,

     {
       key_length-= HA_KEY_BLOB_LENGTH;
       length= min(key_length, key_part->length);
-      uint bytes= key_part->field->get_key_image(to_key, length, 
Field::itRAW);

+      uint bytes= key_part->field->get_key_image(to_key, length,
+key_info->flags & HA_SPATIAL ? Field::itMBR : Field::itRAW);
       if (with_zerofill && bytes < length)
         bzero((char*) to_key + bytes, length - bytes);
       to_key+= HA_KEY_BLOB_LENGTH;


--
Regards
Sachin Setiya
Software Engineer at  MariaDB


___
commits mailing list
comm...@mariadb.org
https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [External] Re: Questions regarding closing partitions in MDEV-11084

[Alexey Botchkov]

Hello, guys.

> 2) MDEV-11084 makes explicit partition selection work like FLUSH 
TABLE, due to the closing partitions part.
After the discussion, I removed that part of the fix, so now the 
explicit statement won't force the partition close.

So now the 'explicit_partition.test' passes.

Best regards.
HF




26.04.2018 14:59, Mattias Jonsson wrote:

Hi Holyfoot, Sergei and Jacob,

Thank you for looking into this.

Jacob there are no Jira ticket (that I am aware of) also I do see it 
as two different problems with the same source.
1) Spider engine does not handle opening/closing specific partitions 
(as I understand it currently relies on opening/closing all partitions 
at once in order). Currently this is a crashing bug!
2) MDEV-11084 makes explicit partition selection work like FLUSH 
TABLE, due to the closing partitions part.


I think it would be nice if you could create tickets for those two 
issues (or at least for the first one).


Regards
Mattias

On Thu, Apr 26, 2018 at 11:26 AM, Alexey Botchkov 
<holyf...@mariadb.com <mailto:holyf...@mariadb.com>> wrote:


No, there's no JIRA task for that problem.
So i guess you can create one :)

Best regards.
HF

On Thu, Apr 26, 2018 at 4:50 AM, Jacob Mathew
<jacob.mat...@mariadb.com <mailto:jacob.mat...@mariadb.com>> wrote:

Hi Mattias and Holyfoot,

Is there a Jira bug for this problem?  If not I can create a
bug for it and assign it to Holyfoot.

Thanks,
Jacob

Jacob B. Mathew
Spider and Server Developer
MariaDB Corporation
+1 408 655 8999  (mobile)
jacob.b.mathew    (Skype)
jacob.mat...@mariadb.com


On Tue, Apr 17, 2018 at 9:11 AM, Mattias Jonsson
<mattias.jons...@booking.com
<mailto:mattias.jons...@booking.com>> wrote:

Hi Holyfoot,

    On Mon, Apr 16, 2018 at 3:38 PM, Alexey Botchkov
<holyf...@askmonty.org <mailto:holyf...@askmonty.org>> wrote:
> Hi, Mattias, guys!
>
> While investigating the crash, i'd like to discuss that
>
>> it seems to close partitions whenever it
>> is not used in a statement (i.e. require it to be
reopened in the next
>> statement that would use another partition
>
>
> Yes, it does that, handling statements with the
specified 'PARTITION'
> option.
> The patch supposed to solve the problem when there are
too many partitions
> opened,
> so i think it must close the unused partitions sometime.
> No, it doesn't have to happen that often. I planned to
check the
> table_open_cache
> variable before the forced close. But decided not to do
that initially - as
> it simplified testing,
> and i thought if someone uses the PARTITION option, he
would stick to using
> this partition
> anyway. And  i forgot about that issue.

The reason for not closing partitions in this case is that
it turns
the 'SELECT col FROM t PARTITION(p)' almost into a 'FLUSH
TABLES t'
which is kind of unexpected.
I did a test mixing PK selects with and without 'PARTITION
(p)' clause
and that shows it will close all but 1 partition and then
(re)open all
but 1 partition.

Think of the case when a server runs in production with a
heavily
partitioned table serving simple PK queries and then
someone runs a
query with explicit PARTITION selection, then it will
introduce a
short stall. First for the query itself (closing all but 1
partitions)
and then for the next simple PK query using the same table
(opening
all but 1 partition).

As I read the bug report: the reporter wants to avoid
opening all
partitions. Not that it keeps the partitions open in the
table open
cache (which is an issue on the architectural level of
partitioning
not really fitting into the open table cache).

I attached a diff with the test and results (I also added
handler
status variables to show my point). The diff is against
b4a2baffa82e5c07b96a1c752228560dcac1359b.

Here is the part of the result file that shows what I mean
with extra
comments prepended by MJ>:
CREATE TABLE t1 (a int PRIMARY KEY)
ENGINE = InnoDB
PARTITION BY HASH (a) PARTITIONS 10

Re: [Maria-developers] Questions regarding closing partitions in MDEV-11084

[Alexey Botchkov]

Hi, Mattias, guys!

While investigating the crash, i'd like to discuss that


it seems to close partitions whenever it
is not used in a statement (i.e. require it to be reopened in the next
statement that would use another partition


Yes, it does that, handling statements with the specified 'PARTITION' 
option.
The patch supposed to solve the problem when there are too many 
partitions opened,

so i think it must close the unused partitions sometime.
No, it doesn't have to happen that often. I planned to check the 
table_open_cache
variable before the forced close. But decided not to do that initially - 
as it simplified testing,
and i thought if someone uses the PARTITION option, he would stick to 
using this partition

anyway. And  i forgot about that issue.


Best regards.
HF



13.04.2018 19:07, Mattias Jonsson wrote:

Hi MariaDB Devs,

I tried to evaluate spider engine and found an issue where it crashes,
most likely due to MDEV-11084 (Stacktrace and reproducible test case
attached).

That also leads me to wonder about the performance for partitioned
tables after MDEV-11084, when it seems to close partitions whenever it
is not used in a statement (i.e. require it to be reopened in the next
statement that would use another partition, effectively not using the
open table cache).

I cannot see anything mentioned in the final commit message hinting on
that it closes partitions not used in the current query, but in the
previous patches it was mentioned without any reason.

Why does it close the already opened partitions?
https://github.com/MariaDB/server/blob/10.3/sql/ha_partition.cc#L8365

I would not mind opening the partitions only when they are to be used
(even though there are engines that need to be tested more), but
closing them makes no sense to me performance wise. Also notice that
the partitions first will be put back into the open table cache and
then on the next query the non-used partitions will be closed and the
needed ones be (re)-opened.

Regards
Mattias Jonsson




___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Please review MDEV-11292 Item_func_sp::fix_length_and_dec() does not set collation derivation properly

[Alexey Botchkov]

The patch looks ok with me.

Best regards.
HF

17.11.2016 6:58, Alexander Barkov wrote:

Hello Alexey,

Please review a patch for MDEV-11292 (for 10.3).

Thanks.


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] 0310b44: MDEV-10418 Assertion `m_extra_cache' failed in ha_partition::late_extra_cache(uint)

[Alexey Botchkov]

This seems fairly obvious, that it should.
But why MySQL doesn't have this bug (despite not resetting
m_extra_prepare_for_update)?


The difference is in the ha_partition::late_extra_cache() function
MySQL:
  if (m_extra_prepare_for_update)
  {
(void) file->extra(HA_EXTRA_PREPARE_FOR_UPDATE);
  }

Maria:
  if (m_extra_prepare_for_update)
  {
DBUG_ASSERT(m_extra_cache);
(void) file->extra(HA_EXTRA_PREPARE_FOR_UPDATE);
  }

And that DBUG_ASSERT() actually does the crash.

MySQL just blindly does the file->extra(HA_EXTRA_PREPARE_FOR_UPDATE)
even for that 'SELECT * FROM t2;' query.

Best regards.
HF




21.12.2016 0:35, Sergei Golubchik wrote:

Hi, Alexey!

On Dec 20, Alexey Botchkov wrote:

revision-id: 0310b444dba1b9cbd6921963dd3bff689ca23a24 
(mariadb-5.5.53-34-g0310b44)
parent(s): f23b41b9b8a30e0e54a1ec7a8923057b0e57e0f5
committer: Alexey Botchkov
timestamp: 2016-12-20 00:24:20 +0400
message:

MDEV-10418 Assertion `m_extra_cache' failed in 
ha_partition::late_extra_cache(uint)

 The m_extra_prepare_for_update parameter should be set to FALSE
 as the query ends.

This seems fairly obvious, that it should.
But why MySQL doesn't have this bug (despite not resetting
m_extra_prepare_for_update)?


diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
index 15fa7d1..4374354 100644
--- a/sql/ha_partition.cc
+++ b/sql/ha_partition.cc
@@ -6667,6 +6667,7 @@ int ha_partition::reset(void)
DBUG_ENTER("ha_partition::reset");
if (m_part_info)
  bitmap_set_all(_part_info->used_partitions);
+  m_extra_prepare_for_update= FALSE;
file= m_file;
do
{

Regards,
Sergei
Chief Architect MariaDB
and secur...@mariadb.org

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp




___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] 41a12f9: MDEV-8320 Allow index usage for DATE(datetime_column) = const.

[Alexey Botchkov]

Hi, Sergei.

See the new patch here:
http://lists.askmonty.org/pipermail/commits/2016-November/010107.html

Some comments, mostly answering your questions

The patch was a model one, just to show how i'd solve this problem, not
something to be pushed as it is.

> >  +static Item_field *get_local_field (Item *field)
> >  +{
> >  +  Item *ri= field->real_item();
> >  +  return (ri->type() == Item::FIELD_ITEM
> >  + && !(field->used_tables() & OUTER_REF_TABLE_BIT)
> >  +&& !((Item_field *)ri)->get_depended_from()) ? (Item_field *) 
ri : 0;

> >  +}
> Please fix indentation and add comments.
> Does this function do what is_local_field does, or there is some 
difference?


There's no difference in what it does from the is_local_field. Just 
seems more
convenient to return the 'ri'. Rids us from a lot of field->real_item() 
calls.
I think code will be nicer if we change is_local_field with this 
get_local_field

calls. Didn't do that to keep the patch possible smaller.


> >  +bool Item_bool_rowready_func2::add_extra_key_fields(THD *thd,
> >  +   JOIN *join, KEY_FIELD 
**key_fields,

> >  +   uint *and_level,
> >  +   table_map usable_tables,
> >  + SARGABLE_PARAM **sargables)
> >  +{
> >  +  Item_field *f;
> >  +  if ((f= field_in_sargable_func(args[0])) && args[1]->const_item())

> What is the difference between add_key_fields and 
add_extra_key_fields? Any

> cases where one should call one but not the other?

The difference is obvious i guess :) The add_extra_key_fields handle that
new 'reverse-function' opportunity. But yes, the add_extra_key_fields should
ultimately disappear getting inside the add_key_fields.
Only reason i made it separate for now is that it has the THD *thd
argument which 'add_key_fields' doesn't. And again i didn't want to 
pollute this

patch as that lot of 'add_key_fields' lines would have to be changed.


Best regards.
HF


02.11.2016 2:38, Sergey Petrunia пишет:

In-Reply-To: <20160928105123.6D948140DDC@nebo.localdomain>
Hi Alexey,

Thanks for your patience in waiting for the review. Please find it below.

On Wed, Sep 28, 2016 at 02:50:19PM +0400, Alexey Botchkov wrote:

revision-id: 41a12f990519fb68eaa66ecc6860985471e6ba5a 
(mariadb-10.1.8-264-g41a12f9)
parent(s): 28f441e36aaaec15ce7d447ef709fad7fbc7cf7d
committer: Alexey Botchkov
timestamp: 2016-09-28 14:48:54 +0400
message:

MDEV-8320 Allow index usage for DATE(datetime_column) = const.

 Test for 'sargable functions' added.


First, t/range.test crashes after I apply the patch. MTR output is here:
https://gist.github.com/spetrunia/d10165820664e0d18d4a667d44d226ee
but I've got the crash on two different machines so should be easy to repeat


  diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h
  index 6d432bd..516bb07 100644
  --- a/sql/item_cmpfunc.h
  +++ b/sql/item_cmpfunc.h
  @@ -136,6 +136,14 @@ class Item_bool_func :public Item_int_func
   {
   protected:
 /*
  +Some functions modify it's arguments for the optimizer.
  +So for example the condition 'Func(fieldX) = constY' turned into
  +'fieldX = cnuR(constY)' so that optimizer can use an index on fieldX.
  +  */

What's cnuR?
Ok, I eventually got it, but the comments should not have such puzzles.


  +  Item *opt_args[3];
  +  uint opt_arg_count;
  +
  +  /*
  +static Item_field *get_local_field (Item *field)
  +{
  +  Item *ri= field->real_item();
  +  return (ri->type() == Item::FIELD_ITEM
  + && !(field->used_tables() & OUTER_REF_TABLE_BIT)
  +&& !((Item_field *)ri)->get_depended_from()) ? (Item_field *) ri : 0;
  +}

Please fix indentation and add comments.
Does this function do what is_local_field does, or there is some difference?


  +
  +
  +static Item_field *field_in_sargable_func(Item *fn)
  +{
  +  fn= fn->real_item();
  +
  +  if (fn->type() == Item::FUNC_ITEM &&
  +  strcmp(((Item_func *)fn)->func_name(), "cast_as_date") == 0)
  +
  +  {
  +Item_date_typecast *dt= (Item_date_typecast *) fn;
  +return get_local_field(dt->arguments()[0]);
  +  }
  +  return 0;

Please use NULL instead of 0, and !strcmp() instead of strcmp()=0.


  @@ -5036,6 +5060,25 @@ Item_func_like::add_key_fields(JOIN *join, KEY_FIELD 
**key_fields,
   }
   
   
  +bool Item_bool_rowready_func2::add_extra_key_fields(THD *thd,

  +   JOIN *join, KEY_FIELD 
**key_fields,
  +   uint *and_level,
  +   table_map usable_tables,
  +   SARGABLE_PARAM **sargables)
  +{
  +  Item_field *f;
  +  if ((f= field_in_sargable_func(args[0])) && ar

Re: [Maria-developers] new PLUGIN API.

[Alexey Botchkov]

To me both approaches are like indistingushable. The 'service' is too just
the set of callback methods.
I don't know if we should keep that 'plugin type' specification or not,
the very idea is getting server's data and sending commands
to server using the callback functions.
That way plugins get free from the API versions, and the server
is free from fixed structures to send/get data to/from the plugin.

If we need we can limit the access to the 'audit service' to only these
plugins registered with the AUDIT type. There will be other limitations
anyway. The server data can be accessible in some context
inside the audit_notify functions for example)
but not accessible in another.

Best regards.
HF

23.10.2016 15:55, Sergei Golubchik wrote:

Hi, Alexey!

On Oct 23, Alexey Botchkov wrote:

Hi, Sergei.

I'd like to draw your attention to this old issue:
https://jira.mariadb.org/browse/MDEV-7389

The idea was to make a bigger thing - to modify the plugin API
so it is easier to use and let user to do more. Particularly to
notify warnings to the audit plugins for this 7389 task.
That was done with this task: https://jira.mariadb.org/browse/MDEV-5313

Just to refresh our memory:
I proposed to get rid off the API versions and version-dependent
memory structures that are used to transfer data to and from the plugin.
All we need to do is adding new 'audit_plugin_service'. Which is
just the normal service that offer methods to the auditing plugin to
send commands to the server and get the server data. You can look at the
patch http://lists.askmonty.org/pipermail/commits/2016-February/009025.html

So, Serg, do you have anything to say on that subject?

Well...

First, I'll say that I'm sorry for not replying earlier :)

Now, "service" is a server functionality provided openly to all
plugins. Kind of a utility library, like alloc() or printf().

Moving audit to a service, means there is no longer audit plugin type
(we won't remove it, but it becomes useless) because any plugin can use
audit functionality. Is that what we want? Audit is not a
general-purpose thing. May be you mean that we should deprecate all
plugin types and move everything to services (not now, but over time)?
That one possible approach. But just moving audit to a service is not
very logical.

Another approach would be to keep audit plugin as a *type*, but still
use your callback approach. That's fine, callbacks don't need to be done
via services, there are plugin-type-specific callbacks too.
In that case an audit plugin will get some kind of an audit handler from
the server, and will invoke its audit methods, for example

   audit_handler->get_database(thd);

etc.

Regards,
Sergei
Chief Architect MariaDB
and secur...@mariadb.org




___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Rewiew requested.

[Alexey Botchkov]

Right. That's for 10.2.

HF


24.10.2016 23:40, Sergey Petrunia wrote:

Hi Alexey,

I think this change is good.
Which version is it going into? (I assume, 10.2?)

On Mon, Oct 24, 2016 at 05:54:57PM +0400, Alexey Botchkov wrote:

I'd like your opinion about this difference in the result of the null.test:

--

--- /home/hf/wgit/cmt-mdev-9143/mysql-test/r/null.result 2016-10-23
13:33:54.050093010 +0400
+++ /home/hf/wgit/cmt-mdev-9143/mysql-test/r/null.reject 2016-10-24
17:31:55.553248271 +0400
@@ -1584,7 +1584,7 @@
  id select_type table   typepossible_keys   key key_len
ref rowsfilteredExtra
  1  SIMPLE  t1  ALL NULLNULLNULLNULL 3
100.00  Using where
  Warnings:
-Note   1003select `test`.`t1`.`c1` AS `c1` from `test`.`t1`
where (((`test`.`t1`.`c1` is not null) >= ((not(1 is not
null)
+Note   1003select `test`.`t1`.`c1` AS `c1` from `test`.`t1`
where (((`test`.`t1`.`c1` is not null) >= 0) is not null)
  SELECT * FROM t1 WHERE ((c1 IS NOT NULL) >= (NOT TRUE)) IS NOT NULL;
  c1
  1

--

I mean i made a change in the code that changed the test result,
which is normally not good.

Though i think here we rather have an improvement.

What do you think? Would you approve that change in the result?

See below for the patch that caused all this:

--

diff --git a/sql/item.cc b/sql/item.cc
index 448e34b..2388679 100644
--- a/sql/item.cc
   +++ b/sql/item.cc
@@ -2900,6 +2900,14 @@ void Item_int::print(String *str,
enum_query_type query_type)
   }


+Item *Item_bool::neg_transformer(THD *thd)
+{
+  value= !value;
+  name= 0;
+  return this;
+}
+
+
  Item_uint::Item_uint(THD *thd, const char *str_arg, uint length):
Item_int(thd, str_arg, length)
  {
diff --git a/sql/item.h b/sql/item.h
index 7644235..ab70fdb 100644
--- a/sql/item.h
+++ b/sql/item.h
@@ -3008,6 +3008,7 @@ class Item_bool :public Item_int
Item_bool(THD *thd, const char *str_arg, longlong i):
  Item_int(thd, str_arg, i, 1) {}
bool is_bool_type() { return true; }
+  Item *neg_transformer(THD *thd);
};

--


Best regards.

HF





___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] Rewiew requested.

[Alexey Botchkov]

Hi, Sergey.

I'd like your opinion about this difference in the result of the null.test:

--

--- /home/hf/wgit/cmt-mdev-9143/mysql-test/r/null.result 2016-10-23 
13:33:54.050093010 +0400
+++ /home/hf/wgit/cmt-mdev-9143/mysql-test/r/null.reject 2016-10-24 
17:31:55.553248271 +0400

@@ -1584,7 +1584,7 @@
 id select_type table   typepossible_keys   key key_len 
ref rowsfilteredExtra
 1  SIMPLE  t1  ALL NULLNULLNULLNULL 3   
100.00  Using where

 Warnings:
-Note   1003select `test`.`t1`.`c1` AS `c1` from `test`.`t1` where 
(((`test`.`t1`.`c1` is not null) >= ((not(1 is not null)
+Note   1003select `test`.`t1`.`c1` AS `c1` from `test`.`t1` where 
(((`test`.`t1`.`c1` is not null) >= 0) is not null)

 SELECT * FROM t1 WHERE ((c1 IS NOT NULL) >= (NOT TRUE)) IS NOT NULL;
 c1
 1

--

I mean i made a change in the code that changed the test result, which 
is normally not good.


Though i think here we rather have an improvement.

What do you think? Would you approve that change in the result?

See below for the patch that caused all this:

--

diff --git a/sql/item.cc b/sql/item.cc
index 448e34b..2388679 100644
--- a/sql/item.cc
  +++ b/sql/item.cc
@@ -2900,6 +2900,14 @@ void Item_int::print(String *str, enum_query_type 
query_type)

  }


+Item *Item_bool::neg_transformer(THD *thd)
+{
+  value= !value;
+  name= 0;
+  return this;
+}
+
+
 Item_uint::Item_uint(THD *thd, const char *str_arg, uint length):
   Item_int(thd, str_arg, length)
 {
diff --git a/sql/item.h b/sql/item.h
index 7644235..ab70fdb 100644
--- a/sql/item.h
+++ b/sql/item.h
@@ -3008,6 +3008,7 @@ class Item_bool :public Item_int
   Item_bool(THD *thd, const char *str_arg, longlong i):
 Item_int(thd, str_arg, i, 1) {}
   bool is_bool_type() { return true; }
+  Item *neg_transformer(THD *thd);
};

--


Best regards.

HF



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] new PLUGIN API.

[Alexey Botchkov]

Hi, Sergei.

I'd like to draw your attention to this old issue:
https://jira.mariadb.org/browse/MDEV-7389

The idea was to make a bigger thing - to modify the plugin API
so it is easier to use and let user to do more. Particularly to
notify warnings to the audit plugins for this 7389 task.
That was done with this task: https://jira.mariadb.org/browse/MDEV-5313

Just to refresh our memory:
I proposed to get rid off the API versions and version-dependent
memory structures that are used to transfer data to and from the plugin.
All we need to do is adding new 'audit_plugin_service'. Which is
just the normal service that offer methods to the auditing plugin to
send commands to the server and get the server data. You can look at the
patch http://lists.askmonty.org/pipermail/commits/2016-February/009025.html

So, Serg, do you have anything to say on that subject?

Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] review for MDEV-9143 (JSON).

[Alexey Botchkov]

I ran an additional test - created a table in MySQL with the VARCHAR column,

not JSON (as i did in MariaDB), and ran the same query.

So now MySQL had to parse the JSON and then look for path there.

--

MySQL [test]> select count(*) from jt2 where json_extract(j, 
"$.key1[1].key2")='IT';

+--+
| count(*) |
+--+
| 2000 |
+--+
1 row in set (5 min 28.54 sec)

---

Wow, I was surprised. 5.5 minutes against 7.6 seconds.

That i guess answers two questions:

   how rapid is RapidJSON?

   why MySQL bothers with the new JSON field type?


Best regards.

HF




25.07.2016 14:10, Alexey Botchkov wrote:

Hi, Sergei.


I've requested your review on this, here are few additional notes.

I ran some benchmarks comparing against MySQL5.7's JSON field type. 
And it seems


that the difference in performance is negligible.

For instance here are results for 20M rows with rather complicated JSON:


Space occupied:

MariaDB

-rw-rw. 1 hf hf 952000 Jul 25 13:08 jt1.MYD


MySQL

-rw-r-. 1 hf hf 944000 Jul 25 13:28 jt1.MYD


Query time:

MariaDB [test]> select count(*) from jt1 where json_extract(j, 
"$.key1[1].key2")='IT';

+--+
| count(*) |
+--+
| 2000 |
+--+
1 row in set (7.65 sec)


MySQL [test]> select count(*) from jt1 where json_extract(j, 
"$.key1[1].key2")='IT';

+--+
| count(*) |
+--+
| 2000 |
+--+
1 row in set (7.56 sec)


So again i can't think now of usecases where we have to store the JSON 
in some special


binary representation. Only thing i can figure out is that we'd like 
to be able to read the MySQL database


without any specific conversion.


Best regards.

HF




___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] review for MDEV-9143 (JSON).

[Alexey Botchkov]

Hi, Sergei.


I've requested your review on this, here are few additional notes.

I ran some benchmarks comparing against MySQL5.7's JSON field type. And 
it seems


that the difference in performance is negligible.

For instance here are results for 20M rows with rather complicated JSON:


Space occupied:

MariaDB

-rw-rw. 1 hf hf 952000 Jul 25 13:08 jt1.MYD


MySQL

-rw-r-. 1 hf hf 944000 Jul 25 13:28 jt1.MYD


Query time:

MariaDB [test]> select count(*) from jt1 where json_extract(j, 
"$.key1[1].key2")='IT';

+--+
| count(*) |
+--+
| 2000 |
+--+
1 row in set (7.65 sec)


MySQL [test]> select count(*) from jt1 where json_extract(j, 
"$.key1[1].key2")='IT';

+--+
| count(*) |
+--+
| 2000 |
+--+
1 row in set (7.56 sec)


So again i can't think now of usecases where we have to store the JSON 
in some special


binary representation. Only thing i can figure out is that we'd like to 
be able to read the MySQL database


without any specific conversion.


Best regards.

HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] When SP DEFINER is empty.

[Alexey Botchkov]

Hi, Sergey, all.


I'm in doubts about how this one should be fixed:

https://jira.mariadb.org/browse/MDEV-10119


The story goes like this:

-  mysql_install_db script runs 'mysqld --skip-grant-tables'

In this case the 'current_user()' seems to be empty.


- so the CREATE PROCEDURE command in the bootstrap creates the procedure 
with the empty DEFINER.



- as a result, the 'SHOW CREATE PROCEDURE' returns query started with

 'CREATE DEFINER=`` PROCEDURE...',


- that DEFINER=`` gives an error when feed to the server.


That can be fixed on any stage. We can do any of these:

- set some 'current_user()' to be not empty even with the 
--skip-grant-tables option


- specify some non-empty DEFINER for the CREATE PROCEDURE statement

(in both options it's not that clear what user could that be)

- fix the SHOW CREATE PROCEDURE statement so it doesn't add the 
errorneous DEFINER=`` to the query.


- make server handling the 'DEFINER=``' with no error. Maybe assigning 
the 'current_user()' in this case.



So, what can You recommend as a fix in this case?


Best regards.

HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] JSON libraries

[Alexey Botchkov]

http://www.json.org/JSON_checker/


31.05.2016 16:53, Sergey Petrunia wrote:

On Tue, May 31, 2016 at 03:35:22PM +0400, Alexey Botchkov wrote:
..

That JSON_CHECKER was produced by JSON.org itself, is really fast
and nice. But it's not
a parser - it just checks if the text makes the correct JSON.
Well it took some time, but now ready. Now opening it for review.

So where one can look at it?

BR
  Sergei



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] JSON libraries

[Alexey Botchkov]

Hi.

So i spent quite some time trying to get in use an existing JSON-parsing 
libraries.

But surprisingly enough none seemed to be good enough.
Problems i met were like this:

1. Parsers are just slow.
- because they copy data - particularly key names and values.
- because they use memory allocation/deallocations
- because they convert numbers from strings to some numeric formats.
Not that i criticise them - it's all mostly because authors of 
these libraries

were concentrating on the ease of use for the inexperienced user.

2. They have to implement the support for different character sets, and
cannot use our code for charsets directly. That leads to further 
slowdown

of execution and some inconsistencies with the existing behaviour.

3. Their interface is inconvenient. In order to implement the required 
JSON functions
we'd like to start/stop parsing on various stages, skip parts of 
JSON, insert
something to the found place etc. With the usual interface of the 
libraries
doing all this looks really ugly and inefficient. So that MySQL 
doesn't even
try to implement this - it only uses the library to convert the 
JSON to internal

format and then only operates with that internal.

4. Libraries tend to have their own ways of reporting errors while we'd 
like use our
error messages. Now MySQL just prints the library-generated 
messages, they cannot

translate or somehow modyfy them.

5. We have to parse not only JSON, but also json-paths, as parts of 
various JSON functions.
Libraries don't usually parse the json-path. Partly because these 
paths can be very different in

various applications.
Still parts of the path (strings to be specific) should be parsed 
just as JSON, and it'd be good
to use the JSON parser for it, which can require ugly tricks. Like 
MySQL constructs JSON
from that piece of json-path to be able to feed that to the JSON 
parser.


Above all that, i must note that the JSON is pretty simple and compact 
as a data format. And

i seriously belive handling it can be done with great efficiency.
So i decided to invest in local JSON parser, taking JSON_CHECKER as a 
source of inspiration.


That JSON_CHECKER was produced by JSON.org itself, is really fast and 
nice. But it's not

a parser - it just checks if the text makes the correct JSON.
Well it took some time, but now ready. Now opening it for review.

Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] c75cbc1: MDEV-717 LP:1003679 - Wrong binlog order on concurrent DROP schema and

[Alexey Botchkov]

> Your test here expects "either ER_BAD_DB_ERROR or a success".
> I asked - does that mean that even with your debug_sync points the 
result is still non-deterministic?


Ah, got it.
The test is deterministic, so we should only wait for the BAD_DB_ERROR.
Will remove the '0'.

Best regards.
HF.

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] c75cbc1: MDEV-717 LP:1003679 - Wrong binlog order on concurrent DROP schema and

[Alexey Botchkov]

Hm... If the default connection signals "locked" before you start
waiting for it now - will your test still work?


Well the DEBUG_SYNC manual seems to be explicit about it:
"  A signal remains in effect until it is overwritten. If conn1 signals
  'opened' before conn2 reaches 'now', conn2 will still find the 'opened'
  signal. It does not wait in this case.
"


+SET DEBUG_SYNC= "sp_create_routine_started SIGNAL release";
+--error 0,ER_BAD_DB_ERROR



why? Do you mean that even with your sync points the specific execution
order if not guaranteed?


Not sure i understand your question properly.
But i mean, getting the BAD_DB_ERROR is the execution result i desire here.


Best regards.
HF


03.05.2016 21:38, Sergei Golubchik wrote:

Hi, Alexey!

convention below: when I start a comment with "btw" it means that you
don't need to change anything, it's just a thought I had when looking at
that line in the patch.

On May 03, Alexey Botchkov wrote:

revision-id: c75cbc19806223be8e750c27ba0bd4b17ef61f54 
(mariadb-10.1.13-24-gc75cbc1)
parent(s): 94cd0f6c9b3b04db67501ef29d470f32527ceda2
committer: Alexey Botchkov
timestamp: 2016-05-03 13:20:12 +0400
message:

MDEV-717 LP:1003679 - Wrong binlog order on concurrent DROP schema and
CREATE function.

 Test case added. That required setting some DEBUG_SYNC points.

diff --git a/mysql-test/suite/binlog/t/binlog_mdev717.test 
b/mysql-test/suite/binlog/t/binlog_mdev717.test
new file mode 100644
index 000..5e8cce6
--- /dev/null
+++ b/mysql-test/suite/binlog/t/binlog_mdev717.test
@@ -0,0 +1,49 @@
+# MDEV-717 LP:1003679 - Wrong binlog order on concurrent DROP schema and 
CREATE function.
+
+--source include/have_log_bin.inc
+RESET MASTER;
+
+disable_warnings;
+DROP DATABASE IF EXISTS mysqltest;
+enable_warnings;

btw, generally this isn't necessary anymore, after-test checks verify that
every test cleans up after itself.


+connect(con1,localhost,root,,);

btw, this works also without extra commas:

connect(con1,localhost,root);


+connection default;
+
+CREATE DATABASE mysqltest;
+SET DEBUG_SYNC= "wait_drop_db_name_locked SIGNAL locked WAIT_FOR release";
+--send DROP DATABASE mysqltest;
+connection con1;
+SET DEBUG_SYNC= "now WAIT_FOR locked";

Hm... If the default connection signals "locked" before you start
waiting for it now - will your test still work? You can test it by
adding a sleep right after "connection con1". (don't commit the sleep,
of course, it's just to way to verify whether the test case is valid)

Alternatively, in many places tests wait for a connection to reach the
certain debug_sync point with, like:

   let $wait_condition= select count(*) = 1 from information_schema.processlist
where state like "debug sync point: 
wait_drop_db_name_locked%";
   source include/wait_condition.inc;


+SET DEBUG_SYNC= "sp_create_routine_started SIGNAL release";
+--error 0,ER_BAD_DB_ERROR

why? Do you mean that even with your sync points the specific execution
order if not guaranteed?


+CREATE FUNCTION mysqltest.f1() RETURNS INT RETURN 1;
+connection default;
+--reap
+
+CREATE DATABASE mysqltest;
+SET DEBUG_SYNC= "wait_drop_db_name_locked SIGNAL locked1 WAIT_FOR release1";
+--send DROP DATABASE mysqltest;
+connection con1;
+SET DEBUG_SYNC= "now WAIT_FOR locked1";
+SET DEBUG_SYNC= "create_event_started SIGNAL release1";
+--error 0,ER_BAD_DB_ERROR
+CREATE EVENT mysqltest.e1 ON SCHEDULE EVERY 15 MINUTE DO BEGIN END;
+connection default;
+--reap
+
+CREATE DATABASE mysqltest;
+CREATE EVENT mysqltest.e1 ON SCHEDULE EVERY 15 MINUTE DO BEGIN END;
+SET DEBUG_SYNC= "wait_drop_db_name_locked SIGNAL locked1 WAIT_FOR release1";
+--send DROP DATABASE mysqltest;
+connection con1;
+SET DEBUG_SYNC= "now WAIT_FOR locked1";
+SET DEBUG_SYNC= "update_event_started SIGNAL release1";
+--error 0,ER_BAD_DB_ERROR
+ALTER EVENT mysqltest.e1 ON SCHEDULE EVERY 20 MINUTE DO BEGIN END;
+connection default;
+--reap
+
+SET DEBUG_SYNC= "RESET";
+--source include/show_binlog_events.inc
+
diff --git a/sql/events.cc b/sql/events.cc
index 77dbb8b..3c50d6e 100644
diff --git a/sql/sp.cc b/sql/sp.cc
index a518b52..c2e3fd2 100644
--- a/sql/sp.cc
+++ b/sql/sp.cc
@@ -1043,6 +1043,8 @@ sp_create_routine(THD *thd, stored_procedure_type type, 
sp_head *sp)
DBUG_ASSERT(type == TYPE_ENUM_PROCEDURE ||
type == TYPE_ENUM_FUNCTION);
  
+  DEBUG_SYNC(thd, "sp_create_routine_started");

+

could you use "before_wait_locked_pname" sync point instead?
It's at the beginning of lock_object_name().


/* Grab an exclusive MDL lock. */
if (lock_object_name(thd, mdl_type, sp->m_db.str, sp->m_name.str))
{
diff --git a/sql/sql_db.cc b/sql/sql_db.cc
index 2ba67cb..fdb49f2 100644
--- a/sql/sql_db.cc
+++ b/sql/sql_db.cc
@@ -837,6 +837,8 @@ mysql_rm_db_internal(THD *thd,char *d

Re: [Maria-developers] ad8b439: MDEV-9618 solaris sparc build fails on 10.1.

[Alexey Botchkov]

Hello, SerG.


diff --git a/include/mysql/plugin_audit.h b/include/mysql/plugin_audit.h
index 31589f0..e96f743 100644
+#ifdef __cplusplus
+extern "C" {

I agree, in general. But why does your short patch have nothing
about audit api?


Because it doesn't lead to any build failures. So there's no actual need 
for this change.

Added just to be consistent.


On the other hand, I don't see a point in moving casts from the
assignment to a new variable definition. So, I'd suggest to keep all
extern "C" hunks in the patch, but remove cast avoidance changes.


I don't know how can i do that nicely.
What i tried by now -
  change
  (uint (*)(unsigned int, unsigned int))my_aes_ctx_size;
  with
  (extern "C" uint (*)(unsigned int, unsigned int))my_aes_ctx_size;
that didn't compile.

Then i tried to put the 'extern "C" {" around the 
initialize_encryption_plugin().
It worked but then i needed to add the 'extern "C"' to the 
initialize_encryption_plugin() declaration.


What would you suggest?

HF.


02.05.2016 19:32, Sergei Golubchik wrote:

Hi, Alexey!

On May 02, Alexey Botchkov wrote:

revision-id: ad8b4399f2fcd1d5e9dcdb8b2a8832cb3a6745ae 
(mariadb-10.1.13-23-gad8b439)
parent(s): ad4239cc3dc7ad5f6f264e1fb3cf6d24084bda90
committer: Alexey Botchkov
timestamp: 2016-05-02 12:03:39 +0400
message:

MDEV-9618 solaris sparc build fails on 10.1.

 Compiler on Solaris is sensitive to C/C++ call models
 differences, so it fails if we try to mix these two in
 '?' operator.
 Fixed by trying to keep the call models uniformity with the
 proper amount of 'extern "C"' hints.

---
  include/my_crypt.h |  6 ++
  include/mysql/plugin_audit.h   |  7 +++
  include/mysql/plugin_encryption.h  |  9 +
  .../example_key_management_plugin.cc   |  2 +-
  plugin/file_key_management/file_key_management_plugin.cc   |  2 +-
  sql/encryption.cc  | 14 ++
  unittest/sql/mf_iocache-t.cc   |  2 +-
  7 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/include/my_crypt.h b/include/my_crypt.h
index e1e94c9..db280ca 100644
--- a/include/my_crypt.h
+++ b/include/my_crypt.h
@@ -82,6 +82,12 @@ static inline uint my_aes_ctx_size(enum my_aes_mode mode 
__attribute__((unused))
return MY_AES_CTX_SIZE;
  }
  
+static inline uint my_aes_ctx_size_for_handler(unsigned int a,

+ unsigned int b __attribute__((unused)))
+{
+  return my_aes_ctx_size((enum my_aes_mode) a);
+}
+

This won't be inlined, because you store a pointer to this function in a
structure. Better use the cast, as before.


  int my_random_bytes(uchar* buf, int num);
  
  #ifdef __cplusplus

diff --git a/include/mysql/plugin_audit.h b/include/mysql/plugin_audit.h
index 31589f0..e96f743 100644
--- a/include/mysql/plugin_audit.h
+++ b/include/mysql/plugin_audit.h
@@ -23,6 +23,10 @@
  
  #include "plugin.h"
  
+#ifdef __cplusplus

+extern "C" {
+#endif

I agree, in general. But why does your short patch have nothing
about audit api?


  #define MYSQL_AUDIT_CLASS_MASK_SIZE 1
  
  #define MYSQL_AUDIT_INTERFACE_VERSION 0x0302

@@ -174,5 +178,8 @@ struct st_mysql_audit
unsigned long class_mask[MYSQL_AUDIT_CLASS_MASK_SIZE];
  };
  
+#ifdef __cplusplus

+}
+#endif
  
  #endif

diff --git a/include/mysql/plugin_encryption.h 
b/include/mysql/plugin_encryption.h
index 3f35c2b..d748c4f 100644
--- a/include/mysql/plugin_encryption.h
+++ b/include/mysql/plugin_encryption.h
@@ -27,6 +27,10 @@
  
  #include 
  
+#ifdef __cplusplus

+extern "C" {
+#endif

...etc

I agree that these extern "C" are the correct fix.
On the other hand, I don't see a point in moving casts from the
assignment to a new variable definition. So, I'd suggest to keep all
extern "C" hunks in the patch, but remove cast avoidance changes.

Then ok to push!

Regards,
Sergei
Chief Architect MariaDB
and secur...@mariadb.org

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] MDEV-5513 merge audit api changes from 5.5.34.

[Alexey Botchkov]

Hello Sergei, all.

I didn't quite understand what exact changes in 5.5.34 are important to us,
so in order to begin i'm going to mention shortly what is new in MySQL 
5.7 audit plugin API.


1. New event classes added:
MYSQL_AUDIT_PARSE_CLASS= 2,
  MYSQL_AUDIT_AUTHORIZATION_CLASS= 3,
  MYSQL_AUDIT_TABLE_ACCESS_CLASS = 4,
  MYSQL_AUDIT_GLOBAL_VARIABLE_CLASS  = 5,
  MYSQL_AUDIT_SERVER_STARTUP_CLASS   = 6,
  MYSQL_AUDIT_SERVER_SHUTDOWN_CLASS  = 7,
  MYSQL_AUDIT_COMMAND_CLASS  = 8,
  MYSQL_AUDIT_QUERY_CLASS= 9,
  MYSQL_AUDIT_STORED_PROGRAM_CLASS   = 10,
These new notifications are sent from various parts of the code. I think 
that part can be most

difficult to keep similar between Maria and My.

New data structures are declared for these classes to send data inside 
notify().
In my opinion these data structures only contain little information, not 
sufficient for many tasks.


2. Plugin can specify the CLASS/SUBCLASS filter in the st_mysql_audit 
structure.
   To implement that nubers of subclasses were changed (i mean 
previously known subclasses).


3. notify() function returns 'int' (it used to be 'void').

   I think 2. and 3. are the reasons MySQL doesn't load plugins with 
older interface.



4. Notification function can affect server.
- Nonzero return from the notify() breaks the query execution in 
some cases.

   - Query can be modified inside the 'notify' function in some cases.

5. Sending notification looks quite crazy in the code. All 
notification-sending functions share
the exactly same name 'mysql_audit_notify'. Naturally the number 
and types of parameters are different.
So that i have complications if i want to find all the places 
where  for instance the MYSQL_AUDIT_PARSE_CLASS

notifications are sent. 'grep' isn't much help anymore.

6. The code is immature. The MYSQL_AUDIT_TABLE_ACCESS_CLASS just is not 
supported.

  st_mysql_audit::class_mask doesn't always work as it's supposed to.



Now my proposals for our audit API.

The 'radical' proposal :

make the notify() function return 'int'

freeze the AUDIT_INTERFACE_VERSION forever.

notify() expects the 'event_class' as a parameter. It does something 
with the classes it is aware of.

if it gets the class it doesn't know about - it should just return.

notify() at the moment is expected to get the server data from the 
'const void *event' sent to it as a paramter,

Which is in my opinion inconvenient for everybody.
My idea is to provide a set of functions like 
'ap_get_query_string(MYSQL_THD thd, const void *event)', or

'ap_get_connection_id(MYSQL_THD thd, const void *event)'.
That set can be extended in newer versions of the server, but old 
functions never change.
If a plugin connects to the old server where that new possibilities are 
not implemented for some reason,

these function still available, just do nothing.
Plugin can check if some functions are available with the server it 
connected to.



We provide the plugin developer with an 'audit_plugin_adaptor.c' file, 
that is built
inside the plugin. It checks the version of the server that loads the 
plugin and links or
implements the 'ap_*' functions respectively. Also it provides proper 
structures for the
connecting server and interceipts notification's call to prepare it for 
the developer's notify() function.



That way the plugin once built can work with variety of servers. And 
getting newer 'audit_plugin_adaptor.c'
is going to make your plugin working with the newer versions of a server 
(be it MariaDB or MySQL).



Less radical proposition is to mimic MySQL's API, to make the plugin 
compilable both in MySQL and MariaDB
environment. So we get rid of the 'maria_declare_plugin', leaving only 
'mysql_declare_plugin'.

The present idea of having both doesn't work well.


Last option i see - get rid of the 'mysql_declare_plugin', leaving only 
'maria_declare_plugin' and
forget about the idea that same plugin is used in MySQL and MariaDB at 
the same time.
In this case i'd provide the set of the beforementioned 'ap_*' functions 
for the plugin to
get server data. As before i belive it'd be convenient, we'd never need 
to check the plugin
API version, and once built the plugin can work with many versions of 
MariaDB. And

we don't need the 'adaptor.c' file as the proper .h file seem to be enough.


Best regards.
HF

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] 50dc8c0: MDEV-8842 add group support to pam_user_map module.

[Alexey Botchkov]

Hi again.

See the next version :)
http://lists.askmonty.org/pipermail/commits/2015-October/008513.html

HF

07.10.2015 13:57, Sergei Golubchik wrote:

Hi, Holyfoot!

On Oct 07, holyf...@askmonty.org wrote:

revision-id: 50dc8c0e8a27d18a3d75ff43a87975fcd5e0e7f6 
(mariadb-10.1.7-67-g50dc8c0)
parent(s): bed4e847950eef50930b44632eea43416e7b37d1
committer: Alexey Botchkov
timestamp: 2015-10-07 00:51:33 +0500
message:

MDEV-8842 add group support to pam_user_map module.
Added to the pam_user_map module.

Looks pretty much ok. Just one suggestion:
I'd keep groups in the pam_sm_authenticate and passed
the pointer down to user_in_group.
Indeed, there may many @group entries in the mapping file, seems like a
waste do repopulate group array every time.


---
  plugin/auth_pam/mapper/pam_user_map.c | 59 +--
  1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/plugin/auth_pam/mapper/pam_user_map.c 
b/plugin/auth_pam/mapper/pam_user_map.c
index e73ab6d..a3008bd 100644
--- a/plugin/auth_pam/mapper/pam_user_map.c
+++ b/plugin/auth_pam/mapper/pam_user_map.c
@@ -13,22 +13,71 @@ authrequiredpam_user_map.so
  
And create /etc/security/user_map.conf with the desired mapping

in the format:  orig_user_name: mapped_user_name
+  @user's_group_name: mapped_user_name
  =
-#comments and emty lines are ignored
+#comments and emtpy lines are ignored
  john: jack
  bob:  admin
  top:  accounting
+@group_ro: readonly
  =
  
  */
  
+#include 

  #include 
  #include 
+#include 
+#include 
+#include 
  #include 
  
  #define FILENAME "/etc/security/user_map.conf"

  #define skip(what) while (*s && (what)) s++
  
+#define GROUP_BUFFER_SIZE 100

+int user_in_group(const char *user, const char *group)
+{
+  gid_t group_buffer[GROUP_BUFFER_SIZE];
+  gid_t *groups= group_buffer;
+  gid_t group_id;
+  gid_t user_group_id;
+  int ng, i;
+
+  {
+struct passwd *pw= getpwnam(user);
+struct group *g= getgrnam(group);
+if (pw == NULL || g == NULL)
+  return 0;
+user_group_id= pw->pw_gid;
+group_id= g->gr_gid;
+  }
+
+  ng= GROUP_BUFFER_SIZE;
+  if (getgrouplist(user, user_group_id, groups, ) < 0)
+  {
+/* The rare case when the user is present in more than */
+/* GROUP_BUFFER_SIZE groups.   */
+groups= (gid_t *) malloc(ng * sizeof (gid_t));
+if (!groups)
+  return 0;
+
+(void) getgrouplist(user, user_group_id, groups, );
+  }
+
+  for (i= 0; i < ng; i++)
+  {
+if (groups[i] == group_id)
+  break;
+  }
+
+  if (groups != group_buffer)
+free(groups);
+
+  return i < ng;
+}
+
+
  int pam_sm_authenticate(pam_handle_t *pamh, int flags,
  int argc, const char *argv[])
  {
@@ -51,10 +100,14 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags,
while (fgets(buf, sizeof(buf), f) != NULL)
{
  char *s= buf, *from, *to, *end_from, *end_to;
+int check_group;
+
  line++;
  
  skip(isspace(*s));

  if (*s == '#' || *s == 0) continue;
+if ((check_group= *s == '@'))
+  s++;
  from= s;
  skip(isalnum(*s) || (*s == '_'));
  end_from= s;
@@ -67,7 +120,9 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags,
  if (end_to == to) goto syntax_error;
  
  *end_from= *end_to= 0;

-if (strcmp(username, from) == 0)
+if (check_group ?
+  user_in_group(username, from) :
+  (strcmp(username, from) == 0))
  {
pam_err= pam_set_item(pamh, PAM_USER, to);
goto ret;

Regards,
Sergei



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] c934b5e: MDEV-8842 add group support to pam_user_map module.

[Alexey Botchkov]

>Eh? Why don't you compare group_id with groups[i] directly ?

D'oh. It's actually i forgot to do the 'git add' with the fixed code.
Never happened to me before.

Anyway, here's the new patch.
http://lists.askmonty.org/pipermail/commits/2015-October/008512.html

Best regards.
HF



06.10.2015 21:43, Sergei Golubchik wrote:

Hi, Holyfoot!

On Oct 06, holyf...@askmonty.org wrote:

revision-id: c934b5e661b19b7934d1a9af40afc70b727c2273 
(mariadb-10.1.7-64-gc934b5e)
parent(s): 90f2c822469a4f88f882ba8974e790f0fb0b2702
committer: Alexey Botchkov
timestamp: 2015-10-06 14:02:29 +0500
message:

MDEV-8842 add group support to pam_user_map module.
Added to the pam_user_map module.

Right, thanks.
A couple of comments, see below.


diff --git a/plugin/auth_pam/mapper/pam_user_map.c 
b/plugin/auth_pam/mapper/pam_user_map.c
index e73ab6d..0787b1c 100644
--- a/plugin/auth_pam/mapper/pam_user_map.c
+++ b/plugin/auth_pam/mapper/pam_user_map.c

...

+  /*
+So below we get the buffer of the NGROUPS_MAX size
+which can take 256K on some systems.
+It supposed to be working differently - we'd get
+the 'ng' from the getgrouplist() and alloc the respective
+size, but the problem is a bug in getgrouplist().
+The glibc 2.3.2 implementation of this function  is  broken:
+it overwrites memory when the actual number of groups is larger
+than *ngroups.
+  */

Forget 2.3.2, use it as it was supposed to be used.
Our oldest builder uses glibc-2.5


+  (void) getgrouplist(user, user_group_id, groups, );
+  for (i= 0; i < ng; i++)
+  {
+struct group *g;
+if ((g= getgrgid(groups[i])) == NULL)
+  return 0;
+if (g->gr_gid == group_id)
+  return 1;

Eh? Why don't you compare group_id with groups[i] directly ?

Regards,
Sergei



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] 7ea735b: MDEV-8431 Feedback plugin needs an option for http proxy.

[Alexey Botchkov]

I mean in order to test it properly i need to check somehow if data 
finds it's

way to our feedback database.
And i don't know yet how to do it.

Best regards.
HF


05.10.2015 19:40, Sergei Golubchik wrote:

Hi, Holyfoot!

On Oct 05, holyf...@askmonty.org wrote:

revision-id: 7ea735baa2df4f4577cf951bc69ccf87fd91f436 
(mariadb-10.1.7-40-g7ea735b)
parent(s): ba0b6685510f4a271da8d386ff75bfd83d3156c2
committer: Alexey Botchkov
timestamp: 2015-10-05 14:46:12 +0500
message:

MDEV-8431 Feedback plugin needs an option for http proxy.
'feedback_http_proxy' system variable added to specify the
proxy server as host:port. Not a dynamic one.

That's a bit more complex that I thought it'll be, but ok.

Still - did you test it? How?

Regards,
Sergei



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] 7ea735b: MDEV-8431 Feedback plugin needs an option for http proxy.

[Alexey Botchkov]

> Still - did you test it? How?

I didn't properly test it.
Just tried to specify a free proxy server and saw the query seemed to be 
sent.


Best regards.
HF



05.10.2015 19:40, Sergei Golubchik wrote:

Hi, Holyfoot! On Oct 05, holyf...@askmonty.org wrote:
revision-id: 7ea735baa2df4f4577cf951bc69ccf87fd91f436 
(mariadb-10.1.7-40-g7ea735b) parent(s): 
ba0b6685510f4a271da8d386ff75bfd83d3156c2 committer: Alexey Botchkov 
timestamp: 2015-10-05 14:46:12 +0500 message: MDEV-8431 Feedback 
plugin needs an option for http proxy. 'feedback_http_proxy' system 
variable added to specify the proxy server as host:port. Not a 
dynamic one. 
That's a bit more complex that I thought it'll be, but ok. Still - did 
you test it? How? Regards, Sergei



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Question regarding MDEV-7682

[Alexey Botchkov]

Hello, Vicențiu.

In the SELECT you mentioned
SELECT 1 FROM t1 NATURAL LEFT JOIN t1 AS t2
the SPATIAL key should not be used, just doesn't fit.

And I must note that i couldn't reproduce the testcase as it was described.
What i got was:
+--+-+---+--+---+--+-+--+--++
| id   | select_type | table | type | possible_keys | key  | key_len | 
ref  | rows | Extra  |

+--+-+---+--+---+--+-+--+--++
|1 | SIMPLE  | t1| ALL  | NULL  | NULL | NULL | NULL 
|2 ||
|1 | SIMPLE  | t2| ALL  | a | NULL | NULL | NULL 
|2 | Range checked for each record (index map: 0x1) |

+--+-+---+--+---+--+-+--+--++

The TABLE:create_key_part_by_field wasn't called during the process.
What function actually returns the key_length of 0? The 
Field_geom::pack_length() returns not-zero for me.


Best regards.
HF


09.03.2015 16:44, Vicențiu Ciorbaru wrote:

Hi Holyfoot!

I've made some changes regarding key_length computation to fix 
MDEV-6838. Changing this seems to have lead to us discovering another 
issue outlined in MDEV-7682 (that I've just created).


It seems that the spatial key returns a key_length of 0. I've spoken 
to Sergei Petrunia and we are not sure about this particular case. 
pack_length returns a bigger number (12). According to the other 
comments in the code, pack_length also returns the length field 
associated with the column, but we are only interested in the length 
of the column field, without any metadata information. The function 
TABLE:create_key_part_by_field adds the metadata length itself afterwards.


Do you have any input on this issue, namely why the key_length is 
returned as 0 for the LINESTRING column?


Regards,
Vicentiu



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] Additions to GIS in 10.1.

[Alexey Botchkov]

Hi all.

What's new about GIS in 10.1. Related tasks in JIRA are:

MDEV-4045 Missing OGC Spatial functions.
MDEV-60 Support for Spatial Reference systems for the GIS data.
MDEV-12 OpenGIS: create required tables: GeometryColumns, related views.


Speaking shortly, the MariaDB GIS part is now OpenGIS compliant, and 
passes all the OpenGIS conformance tests



To get that the next things were done:

Missing functions were added:
IsRing(ln)
The return type is Integer, with a return value of 1 for
TRUE, 0 for FALSE, and –1 for UNKNOWN
corresponding to a function invocation on NULL

arguments;
return TRUE if c is a ring, i.e., if c is closed and simple. A
simple Curve does not pass through the same Point more than 
once.


PointOnSurface(s)
The return type is POINT, returns a Point guaranteed to lie on 
the Surface.


arguments: POLYGON or MYLTIPOLYGON.

NumInteriorRing(p)
The return type is Integer, returns the number of interiorRings

arguments: POLYGON.

Relate(g1 Geometry, g2 Geometry, patternMatrix String)
The return type is Integer, with a return value of 1 for TRUE, 
0 for
FALSE, and –1 for UNKNOWN corresponding to a function 
invocation on NULL arguments;
returns TRUE if the spatial relationship specified by the 
pattern Matrix holds.


ConvexHull(g1 Geometry)
The return is Geometry, returna a geometric object that is the 
convex hull of g1


arguments: GEOMETRY.


The information about the GEOMETRY column's reference system (SRID) is 
now stored in the
field's metadata. Syntax of the CREATE and ALTER was modified to specify 
the SRID

for the geometry columns.

With this information some views required by OpenGIS, were added:

GEOMETRY_COLUMNS, SPATIAL_REF_SYS:
These are links to the INFORMATION_SCHEMA.GEOMETRY_COLUMNS and
INFORMATION_SCHEMA.SPATIAL_REF_SYS respectively.

INFORMATION_SCHEMA.GEOMETRY_COLUMNS:
describes the available feature tables and their Geometry properties.
Fields:
F_TABLE_CATALOG CHARACTER VARYING(256) NOT NULL,
F_TABLE_SCHEMA CHARACTER VARYING(256) NOT NULL,
F_TABLE_NAME CHARACTER VARYING(256) NOT NULL,
the above 3 fields are the fully qualified name of the 
feature table

containing the geometry column;
F_GEOMETRY_COLUMN CHARACTER VARYING(256) NOT NULL,
the name of the column in the feature table that is the 
Geometry Column.

G_TABLE_CATALOG CHARACTER VARYING(256) NOT NULL,
G_TABLE_SCHEMA CHARACTER VARYING(256) NOT NULL,
G_TABLE_NAME CHARACTER VARYING(256) NOT NULL,
the above 3 fields are the name of the geometry table and 
its schema

and catalog. The geometry table implements the geometry column;
STORAGE_TYPE INTEGER,
always 1 in MariaDB (means binary geometry implementation)
GEOMETRY_TYPE INTEGER,
the type of geometry values stored in this column.
0 = GEOMETRY
1 = POINT
3 = LINESTRING
5 = POLYGON
7 = MULTIPOINT
9 = MULTILINESTRING
11 = MULTIPOLYGON

COORD_DIMENSION INTEGER,
the number of dimensions in the spatial reference system.
Always 2 in MariaDB.

MAX_PPR INTEGER,
Always 0 in MariaDB.

SRID INTEGER
the ID of the Spatial Reference System used for the 
coordinate geometry
in this table. It is a foreign key reference to the 
SPATIAL_REF_SYS table.



INFORMATION_SCHEMA.SPATIAL_REF_SYS:
stores information on each spatial reference system used in the 
database.

Fields:
SRID INTEGER NOT NULL PRIMARY KEY,
an integer value that uniquely identifies each Spatial 
Reference System within a database.

AUTH_NAME CHARACTER VARYING(256),
the name of the standard or standards body that is being 
cited for this reference system.

In most cases the value is EPSG.
AUTH_SRID INTEGER TEXT
the Well-known Text Representation of the Spatial Reference 
System.



Required stored procedures added:
AddGeometryColumn(FEATURE_TABLE_CATALOG, FEATURE_TABLE_SCHEMA,
FEATURE_TABLE_NAME, GEOMETRY_COLUMN_NAME, SRID)
adds new column of spatial type to the specified table

DropGeometryColumn(FEATURE_TABLE_CATALOG, FEATURE_TABLE_SCHEMA,
FEATURE_TABLE_NAME, GEOMETRY_COLUMN_NAME)
removes the spatial column from the table.


That's it for now.
Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] MDEV-60 Support for Spatial Reference systems for the GIS data.

[Alexey Botchkov]

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, ListCreate_field 
create_fields)

 +{
 + uint image_size= 0;
 + uint n_field= 0;
 + List_iteratorCreate_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


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] Rev 4271: MDEV-6601 Assertion `!thd-in_active_multi_stmt_transa ction() || thd-in_multi_stmt_transaction_mode()' failed on executing a stored procedure with commit i

[Alexey Botchkov]

Hi, Sergei.

I'd add SERVER_STATUS_LAST_ROW_SENT to the status_backup_mask.
It's not necessary as I don't see it affecting anything, but just to 
make a sign that we don't

want this status to go higher out of the call.

Best regards.
HF


25.08.2014 10:49, Sergei Golubchik wrote:

At lp:~maria-captains/maria/5.5


revno: 4271
revision-id: ser...@pisem.net-20140825064904-h8isjhv963kbxlzy
parent: kniel...@knielsen-hq.org-20140813134639-wk760plnzg5wu4x8
fixes bug: https://mariadb.atlassian.net/browse/MDEV-6601
committer: Sergei Golubchik ser...@pisem.net
branch nick: 5.5
timestamp: Mon 2014-08-25 08:49:04 +0200
message:
   MDEV-6601 Assertion `!thd-in_active_multi_stmt_transa ction() || 
thd-in_multi_stmt_transaction_mode()' failed on executing a stored procedure with 
commit
   
   Don't restore the whole of thd-server_status after a routine invocation,

   only restore SERVER_STATUS_CURSOR_EXISTS, as --ps --embedded needs.
   In particular, don't restore SERVER_STATUS_IN_TRANS.
=== modified file 'mysql-test/r/sp-bugs.result'
--- a/mysql-test/r/sp-bugs.result   2014-01-24 12:50:18 +
+++ b/mysql-test/r/sp-bugs.result   2014-08-25 06:49:04 +
@@ -268,3 +268,9 @@ END $$
  CALL test_5531(1);
  DROP PROCEDURE test_5531;
  DROP TABLE t1;
+create procedure sp() begin
+commit;
+end|
+start transaction;
+call sp();
+drop procedure sp;

=== modified file 'mysql-test/t/sp-bugs.test'
--- a/mysql-test/t/sp-bugs.test 2014-01-24 12:50:18 +
+++ b/mysql-test/t/sp-bugs.test 2014-08-25 06:49:04 +
@@ -285,3 +285,16 @@ DELIMITER ;$$
  CALL test_5531(1);
  DROP PROCEDURE test_5531;
  DROP TABLE t1;
+
+#
+# MDEV-6601 Assertion `!thd-in_active_multi_stmt_transa ction() || 
thd-in_multi_stmt_transaction_mode()' failed on executing a stored procedure with 
commit
+#
+delimiter |;
+create procedure sp() begin
+  commit;
+end|
+delimiter ;|
+start transaction;
+call sp();
+drop procedure sp;
+

=== modified file 'sql/sp_head.cc'
--- a/sql/sp_head.cc2014-04-15 13:17:47 +
+++ b/sql/sp_head.cc2014-08-25 06:49:04 +
@@ -1224,6 +1224,7 @@ sp_head::execute(THD *thd, bool merge_da
Item_change_list old_change_list;
String old_packet;
uint old_server_status;
+  const uint status_backup_mask= SERVER_STATUS_CURSOR_EXISTS;
Reprepare_observer *save_reprepare_observer= thd-m_reprepare_observer;
Object_creation_ctx *saved_creation_ctx;
Warning_info *saved_warning_info;
@@ -1358,7 +1359,7 @@ sp_head::execute(THD *thd, bool merge_da
  It is probably safe to use same thd-convert_buff everywhere.
*/
old_packet.swap(thd-packet);
-  old_server_status= thd-server_status;
+  old_server_status= thd-server_status  status_backup_mask;
  
/*

  Switch to per-instruction arena here. We can do it since we cleanup
@@ -1488,7 +1489,7 @@ sp_head::execute(THD *thd, bool merge_da
thd-spcont-pop_all_cursors(); // To avoid memory leaks after an error
  
/* Restore all saved */

-  thd-server_status= old_server_status;
+  thd-server_status= (thd-server_status  ~status_backup_mask) | 
old_server_status;
old_packet.swap(thd-packet);
DBUG_ASSERT(thd-change_list.is_empty());
old_change_list.move_elements_to(thd-change_list);

___
commits mailing list
comm...@mariadb.org
https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Rev 4077: MDEV-4856 SQL_ERROR_LOG shows 1146 errors which didnt appear in mysql client

[Alexey Botchkov]

Here I tried to come up with what I think is a simpler approach. It
basically uses a buffer in the THD, but just like the old code, this
buffer is in the Diagnostic_area, it's the last error message.


Yep, that's closer to the way it works now. Thus simpler.
What do you want me to do now? To close that issue with your patch?

 Eventually we will need to remove thd-clear_error() completely
Agree.

Best regards.
HF


02.04.2014 2:37, Sergei Golubchik wrote:

Hi, Alexey!

On Mar 20, Alexey Botchkov wrote:

This looks too complex to me. Why would you extend the base
Internal_error_handler class - no other error handler did that or needed
that, fwiw.

Here I tried to come up with what I think is a simpler approach. It
basically uses a buffer in the THD, but just like the old code, this
buffer is in the Diagnostic_area, it's the last error message.

There is also one problem with thd-clear_error() that is used
everywhere in sql_show.cc. I did not fix it, only added a comment about
it. Eventually we will need to remove thd-clear_error() completely, it is
conceptually incompatible with the pluggable auditing.

See the attached patch.

Regards,
Sergei



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] Rev 3995: MDEV-5138 Numerous test failures in mtr --ps --embedded

[Alexey Botchkov]

Sergei,


SERVER_STATUS_CURSOR_EXISTS. Why is it not returned for 'reading
metadata'? Where does the client skip it?


The server status is sent to the client twice - with the recordset
metadata and then with the resulting recordset.

You can see it in the JOIN::exec()
First we do result-send_result_set_metadata(). At this point the server_status
isn't set yet to the SERVER_STATUS_CURSOR_EXISTS. So the server status sent to 
the
client along with the metadata is correct.

Then we call do_select(), were we internally run the SP, which sets the
SERVER_STATUS_CURSOR_EXISTS status. So it's sent to the client with the
resulting recordset only.

Best regards.
HF


31.03.2014 7:33, Sergei Golubchik wrote:

Hi, Alexey!

On Mar 27, Alexey Botchkov wrote:

Hi, Sergei.


Why does embedded fail while a normal client-server protocol is ok with
that?

The difference is that the client checks the server status twice during
execution
- after the metadata reading and after the actual data.
That SERVER_STATUS_CURSOR_EXISTS is returned on the 'reading data' stage
and the usual client just skips it. But for the embedded server
that status is same in both cases. And the 'reading metadata' part
crashes when it sees that server status.
It's in the prepare_to_fetch_result() called from mysql_stmt_execute().

Yes, I see that prepare_to_fetch_result() checks for
SERVER_STATUS_CURSOR_EXISTS. Why is it not returned for 'reading
metadata'? Where does the client skip it?


I see no reason to store both statuses for the embedded server just to
imitate
the usual client's behaviour. It's better not to return the misleading
status in this case.

I agree about not returning the misleading status, but I'd still like to
understand why the regular client works.

Regards,
Sergei



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] Rev 3991: MDEV-5138 Numerous test failures in mtr --ps --embedded

[Alexey Botchkov]

Hi, Sergei.

 Why does Protocol::net_store_data() have to be virtual?

Well it doen't have to be virtual, but it's the nicest solution i came 
up with.

All the related code is in the sql/protocol.cc

In fact only reason is the Protocol::store_string_aux() function that 
calls the ::net_store_data()

internally.
Both Protocol_text::store() and Protocol_binary::store() in turn call 
the Protocol::store_string_aux().
As the ::net_store_data() has to work differently for Protocol_binary 
and Protocol_text,
I saw options either to create two absolutely same 
Protocol_text::store_string_aux() and

Protocol_binary::store_string_aux() or make the net_store_data() virtual.

Best regards.
HF



18.03.2014 20:24, Sergei Golubchik wrote:

Hi, Holyfoot!

On Nov 30, holyf...@askmonty.org wrote:

At file:///home/hf/wmar/mdev-5138/


revno: 3991
revision-id: holyf...@askmonty.org-20131130132432-etlmes8qhhqhr1iu
parent: ele...@wheezy-64.home-20131128160251-vn857wx5qlon6j1p
committer: Alexey Botchkov holyf...@askmonty.org
branch nick: mdev-5138
timestamp: Sat 2013-11-30 17:24:32 +0400
message:
   MDEV-5138 Numerous test failures in mtr --ps --embedded.
   The function Protocol::net_store_data(a, b, CHARSET_A, CHARSET_B) 
should
   be adapted to be working in the embedded server as it's done
   with the Protocol::net_store_data(a, b).
   That new function renamed as net_store_data_cs, so we can make it
   virtual.

Why does Protocol::net_store_data() have to be virtual?
(if it doesn't, Protocol::net_store_data_cs doesn't have to be virtual
either and doesn't have to be renamed)

Regards,
Sergei



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] Rev 3995: MDEV-5138 Numerous test failures in mtr --ps --embedded

[Alexey Botchkov]

Hi, Sergei.


Why does embedded fail while a normal client-server protocol is ok with
that?


The difference is that the client check the server status twice during 
executeion

- after the metadata reading and after the actual data.
That SERVER_STATUS_CURSOR_EXISTS is returned on the 'reading data' stage
and the usual client just skips it. But for the embedded server
that status is same in both cases. And the 'reading metadata' part 
crashes when it sees that server status.

It's in the prepare_to_fetch_result() called from mysql_stmt_execute().
I see no reason to store both statuses for the embedded server just to 
imitate
the usual client's behaviour. It's better not to return the misleading 
status in this case.


Best regards
HF




18.03.2014 20:21, Sergei Golubchik wrote:

Hi, Holyfoot!

On Dec 09, holyf...@askmonty.org wrote:

At file:///home/hf/wmar/mdev-5138/


revno: 3995
revision-id: holyf...@askmonty.org-20131209020845-9o3l30ao90vzf600
parent: holyf...@askmonty.org-20131208143921-3veatgg9wngyzb47
committer: Alexey Botchkov holyf...@askmonty.org
branch nick: mdev-5138
timestamp: Mon 2013-12-09 06:08:45 +0400
message:
   MDEV-5138 Numerous test failures in mtr --ps --embedded.
   If a prepared statement calls an stored procedure,
   the thd-server_status out of the SP goes up
   to the PS and then to the client. So that the
   client gets the SERVER_STATUS_CURSOR_EXISTS status
   if the SP uses a cursor. Which makes the embedded
   server fail.

Why does embedded fail while a normal client-server protocol is ok with
that?


   Fixed by saving/restoring the upper-level server_status
   in sp_head::execute().

Regards,
Sergei



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] Two 10.0-related things.

[Alexey Botchkov]

Hi, Sergei.

I'd like to share my recently obtained knowledge :)

1. Mac OS X and sockets.
As you know there on Labrador machine that mysql-test-run.pl failed
if not run with --tmpdir=/tmp.
The problem apparently is that the /Users directory has the access 
restrictions

that /tmp doesn't. I still don't know what exactly was that, but it helps
if we revoke all the ACLs with 'chmod -R -N /Users/mariadb'.

2. The 10.0 fails building on my machine, with linking errors in
the extra/yassl/, about missing 'operator new[]' and similar.
I fixed it locally with that patch:
=== modified file 'extra/yassl/CMakeLists.txt'
--- extra/yassl/CMakeLists.txt  2014-02-26 14:28:07 +
+++ extra/yassl/CMakeLists.txt  2014-03-25 07:45:46 +
@@ -33,9 +33,10 @@ SET(YASSL_SOURCES  src/buffer.cpp src/ce
 ADD_CONVENIENCE_LIBRARY(yassl ${YASSL_SOURCES})
 RESTRICT_SYMBOL_EXPORTS(yassl)

+TARGET_LINK_LIBRARIES(stdc++)
+
 INSTALL_DEBUG_SYMBOLS(yassl)


Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Rev 4077: MDEV-4856 SQL_ERROR_LOG shows 1146 errors which didnt appear in mysql client

[Alexey Botchkov]

This looks too complex to me. Why would you extend the base
Internal_error_handler class - no other error handler did that or needed
that, fwiw.


No other handlers need it, but these methods are sometimes used
in places where we don't know the type of the handler.
So i think the code is nicer that way - otherwise we had to check
the type somehow manually.
And i don't think that adding three empty virtual functions complicates
the code that much.
 

I'd simply push an error handler, store the first message there and
access it in get_schema_tables_record() via THD::get_internal_handler().


That get_schema_tables_record is not the only place where these 
functions are used.

In the get_table_share_with_discover() for example we need the error number.
(the patch for the reference: 
http://lists.askmonty.org/pipermail/commits/2014-February/005931.html)


Also sql/sql_show.cc, make_table_name_list().

In these we can't be sure the handler is of 'our' type.
In some cases we have another handler above our so we have to implement the
THD::clear_any_error() that would be also not that nice without the
virtual methods.

Best regards.
HF


24.02.2014 23:59, Sergei Golubchik wrote:

Hi, Holyfoot!

On Feb 22, holyf...@askmonty.org wrote:

revno: 4077
revision-id: holyf...@askmonty.org-20140222173543-xkzlp5rpga41evur
parent: s...@mariadb.org-20140218065405-vw3bfhvhrfjsc6hh
committer: Alexey Botchkov holyf...@askmonty.org
branch nick: mdev-4856
timestamp: Sat 2014-02-22 21:35:43 +0400
message:
   MDEV-4856 SQL_ERROR_LOG shows 1146 errors which didnt appear in mysql client.
   The fill_schema_table() function used to call get_table_share()
   for a table name in WHERE then clear the error list. That way
   plugins receive the superfluous error notification if it happens
   in it. Also the problem was that error handler didn't prevent
   the suppressed error message from logging anyway as the logging
   happens in THD::raise_condition before the handler call.
   Fixed by adding the Warnings_only_error_handler before the call.
   raise_condition() also fixed.  The Internal_error_handler class
   was exteded with virtual functions to handle storing the error
   message.

This looks too complex to me. Why would you extend the base
Internal_error_handler class - no other error handler did that or needed
that, fwiw.

I'd simply push an error handler, store the first message there and
access it in get_schema_tables_record() via THD::get_internal_handler().





___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [GSOC 2014] GIS Enhancement

[Alexey Botchkov]

Hi, Bang.

I think yes, the third coordinate operations should be implemented in 
that tool
MariaDB is going to support that later anyway and we can specify the SQL 
syntax

and other ways to input 3d points now.

Best regards.
HF

18.03.2014 22:36, bang honam wrote:

Hi everybody.

I am Honam Bang, a researcher of Korean Institute of Science 
Technology and I will be a master student of UCSD, USA from this Sep. 
I have been working with GIS research for about one year. Therefore, I 
decided to work on the 'GIS Enhancement topic' because I have a lot of 
interest for dealing with geospatial data.


My topic is 'implementing shp to sql command tool for MariaDB'. 
#MDEV-5813.


Most shptosql tools are enable to convert 3D coordinates as I know. 
The shape file white 
paper(http://www.esri.com/library/whitepapers/pdfs/shapefile.pdf) 
specifies a lot of 3D coordinates like POINT Z, MULTIPOINTZ which does 
not exist in MariaDB's Geometry type.


Does my topic include implementing those type converting as well?


Thank you
Honam Bang



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-5681- audit log not rotating

[Alexey Botchkov]

Hi, Tan Tran.

Your patch is quite correct.
I just don't think the 'internal_stop_logging' and 'mark_always_logged' 
lines

are superfluous in this case.
And there are 2 more variables that don't effectively change it's value 
until restart logging.

So i modified the patch with all that, it's here if you're interested:
http://lists.askmonty.org/pipermail/commits/2014-March/006008.html

Thanks for your help.
HF



10.03.2014 9:07, Tan Tran wrote:

Hi maria-developers,

This is my patch for MDEV-5681: 
audit log will not rotate when the file size exceeds global variable setting:


http://bazaar.launchpad.net/~tktran/maria/maria-fix-bug5681/revision/4102 
http://bazaar.launchpad.net/%7Etktran/maria/maria-fix-bug5681/revision/4102


To test,

1. set server_audit_events = 'connect' and keep default rotate size of 
100.
2. connect with a bad password about 20 times: for i in {1..20}; do 
mysql -ufail -pfail; done

3. see that server_audit.log is X bytes
4. set server_audit_rotate_size = 1000
5. connect with bad password for another 20 times
6. see original server_audit.log is still X bytes (meaning log rotated 
immediately) and subsequent server_audit.log.* logs are ~1000 bytes or 
less


Please review.

Thanks,
Tan Tran
Prospective GSoC student


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] GSOC 14: Discussion on GIS enhancements to MariaDB

[Alexey Botchkov]

Hi, Abhishek.

You can look here for the possible tasks.
https://mariadb.atlassian.net/browse/MDEV-5813

They're lower-medium complexity meaning coding scills.
Of course one needs to familiarize oneself with the subject.

Best regards.
HF


06.03.2014 0:02, Abhishek Kumar wrote:

Hi,
I am Abhishek and am interested in knowing about the gsoc project GIS 
enhancements to MariaDB.


I participate often in the programming contents  hosted at these sites:

Codeforces: abhi170893 (Division 1)
TopCoder: abhi170893 ( Divsion 1)
Codechef: abhi_iiit_17 ( Ranked 6 in India in DEC13 . 3 times in top 
20 in country.)

Spoj: abhi170893 (over 100 problems)

The ideas page mentions C as the required skill and I am good at 
coding in C.


Also on the ideas page not much is written about the project. Where 
can i get more details about it..? what things should i explore first 
before trying for this project..? the complexity level..? the 
competition involved..?Do i need to fix bugs first..? If yes, their 
links..?



Thanks
Abhishek Kumar
CSE, IIIT Hyderabad (India)


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Rev 4029: MDEV-4856 SQL_ERROR_LOG shows 1146 errors which didnt appear in mysql client

[Alexey Botchkov]

Hi.

As you didn't react on my last patch about it, i decided to remind:
http://lists.askmonty.org/pipermail/commits/2014-February/005931.html

Best regards.
HF



19.02.2014 20:43, Sergei Golubchik wrote:

Hi, Alexey!

On Feb 19, Alexey Botchkov wrote:

Hi, Sergei.


Could you, please remove that piece of fine creativity and
thd-clear_error() too and use the error handler instead?

There's one puzzle.
This field is set here:
sql_show.cc:4899 (get_schema_tables_record())
if (res || info_error)
{
  /*
If an error was encountered, push a warning, set the TABLE COMMENT
column with the error text, and clear the error so that the operation
can continue.
  */
  const char *error= thd-is_error() ? thd-stmt_da-message() : ;
  table-field[20]-store(error, strlen(error), cs);

  if (thd-is_error())
  {
push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
 thd-stmt_da-sql_errno(), thd-stmt_da-message());
thd-clear_error();
  }
}

As we're supposed to block that error with the Error_handler, the
error message gets lost there and cannot be recovered later in the
get_schema_tables_record.  So we either have stop showing the error
message in that field, or we have to store it somewhere.

The Error_handler looks like a good place to store that error message.
But i can't figure out how to send it to the
get_schema_tables_record() nicely.  Another place is the TABLE_LIST *
parameter. Seems to be convenient for everything.  Though the
TABLE_LIST structure will get even more polluted.

Error handler looks perfect - it could have a char[] buffer and remember
there the first error message for SHOW TABLES.

But I agree that it's difficult to reach from get_schema_tables_record().
One option would be to use

   THD::get_internal_handler()

it'll allow you to access the current error handler. The problem with
this solution - there's no easy way to verify that you've got an object
of the correct class. If someone has pushed another error handler after
you, this will crash.

Another solution could to have a char* pointer in THD or TABLE_LIST and
set it to point to this buffer. This will work even if there's another
handler on top of yours. But it'll need a pointer in THD or TABLE_LIST.

I'd probably use the THD::get_internal_handler() approach and a test
case for it (it'll make sure the calling convention is maintained).
Seems silly to add another member to THD or TABLE_LIST that are used
literally everywhere only to show an open table error in I_S.TABLES.

Regards,
Sergei




___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Rev 4029: MDEV-4856 SQL_ERROR_LOG shows 1146 errors which didnt appear in mysql client

[Alexey Botchkov]

Hi, Sergei.


Could you, please remove that piece of fine creativity and
thd-clear_error() too and use the error handler instead?


There's one puzzle.
We need to fill the INFORMATION_SCHEMA.TABLES.TABLE_COMMENT field.
If a table exists but corrupt, that field presently is filled with the 
error message.

Like here (tx.MYD and tx.MYI files were removed, only tx.frm left):
mysql SELECT TABLE_SCHEMA, TABLE_NAME, TABLE_TYPE, ENGINE, ROW_FORMAT, 
TABLE_ROWS, DATA_LENGTH, TABLE_CO MMENT FROM INFORMATION_SCHEMA.TABLES 
WHERE TABLE_NAME = 'tx';
+--++++++-+-- 
+
| TABLE_SCHEMA | TABLE_NAME | TABLE_TYPE | ENGINE | ROW_FORMAT | 
TABLE_ROWS | DATA_LENGTH | 
TABLE_COMMENT  |
+--++++++-+-- 
+
| test | tx | BASE TABLE | NULL   | NULL |   NULL 
|NULL | Can't find file: 'tx' (errno: 2) |

+--++++++-+--+
1 row in set, 5 warnings (0.00 sec)

This field is set here:
sql_show.cc:4899 (get_schema_tables_record())
  if (res || info_error)
  {
/*
  If an error was encountered, push a warning, set the TABLE COMMENT
  column with the error text, and clear the error so that the operation
  can continue.
*/
const char *error= thd-is_error() ? thd-stmt_da-message() : ;
table-field[20]-store(error, strlen(error), cs);

if (thd-is_error())
{
  push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
   thd-stmt_da-sql_errno(), thd-stmt_da-message());
  thd-clear_error();
}
  }

As we're supposed to block that error with the Error_handler, the error 
message gets lost there

and cannot be recovered later in the get_schema_tables_record.
So we either have stop showing the error message in that field, or we 
have to store it somewhere.
The Error_handler looks like a good place to store that error message. 
But i can't figure out how to

send it to the get_schema_tables_record() nicely.
Another place is the TABLE_LIST * parameter. Seems to be convenient for 
eveything.

Though the TABLE_LIST structure will get even more polluted.

Sergei, what is your opinion here?

Best regards.
HF




17.02.2014 20:30, Sergei Golubchik wrote:
Hi, Alexey! On Feb 14, Alexey Botchkov wrote:

But I don't understand that. Do you mean, old code did *not* suppress
errors here? How could selects from I_S tables work without it?

Here's how it works today:

get_all_tables()
   Trigger_error_handler err_handler;
   thd-push_internal_handler(err_handler);
   fill_schema_table_from_frm
 get_table_share
    /* here we get the error of file not found */
 my_message_sql
   THD::raise_condition
 /* here we test the err_handler */
 /* but it doesn't react on that kind of errors */
 mysql_audit_notify()
 stmt_da-set_error_status
 warning_info-push_warning
   /
 /
    /*error is returned back along the call stack */
 /
 thd-clear_error(); /*which erases all the fileopen errors*/
   /

So basically the error is launched and then erased. Which doesn't help
with the plugin notifications.
To fix that i added one more error handler.

It's worse than that :)
After thd-clear_error() the error flag is cleared, indeed. But all
issued errors are still present in the warning list. I've looked into
this about found this little gem: do_fill_table() function.

It actually copies Warning_info list and removes warnings from there!

Could you, please remove that piece of fine creativity and
thd-clear_error() too and use the error handler instead?

Looking at the logic of do_fill_table(), all you need to do is to
filter out all errors, but keep all warnings. Like

   class Warnings_only_error_handler : public Internal_error_handler
   {
   public:
 bool handle_condition(THD *thd, uint sql_errno, const char* sqlstate,
   MYSQL_ERROR::enum_warning_level level,
   const char* msg, MYSQL_ERROR ** cond_hdl)
 {
   return level == MYSQL_ERROR::WARN_LEVEL_ERROR;
 }
   };


Regards,
Sergei




___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Rev 4029: MDEV-4856 SQL_ERROR_LOG shows 1146 errors which didnt appear in mysql client

[Alexey Botchkov]

But I don't understand that. Do you mean, old code did *not* suppress
errors here? How could selects from I_S tables work without it?


Here's how it works today:

get_all_tables()
Trigger_error_handler err_handler;
thd-push_internal_handler(err_handler);
fill_schema_table_from_frm
get_table_share
 /* here we get the error of file not found */
my_message_sql
THD::raise_condition
/* here we test the err_handler 
*/
/* but it doesn't react on that 
kind of errors */
mysql_audit_notify()
stmt_da-set_error_status
warning_info-push_warning
/
/
 /*error is returned back along the call stack */
/
thd-clear_error(); /*which erases all the fileopen errors*/
/

So basically the error is launched and then erased. Which doesn't help with the 
plugin notifications.
To fix that i added one more error handler.

Best regards.
HF



03.02.2014 20:42, Sergei Golubchik wrote:

Hi, Holyfoot!

On Jan 31, holyf...@askmonty.org wrote:

revno: 4029
revision-id: holyf...@askmonty.org-20140131103303-1rx7ielo83f8iahe
parent: holyf...@askmonty.org-20140124020722-dd5twtwlcc8o1xiy
committer: Alexey Botchkov holyf...@askmonty.org
branch nick: mdev-4856
timestamp: Fri 2014-01-31 14:33:03 +0400
message:
   MDEV-4856 SQL_ERROR_LOG shows 1146 errors which didnt appear in mysql client.
   The fill_schema_table() function used to call get_table_share() for
   a table name in WHERE then clear the error list. That way plugins
   receive the superfluous error notification if it happens in it. Also
   the problem was that error handler didn't prevent the suppressed
   error message from logging anyway as the logging happens in
   THD::raise_condition before the handler call.
   Fixed by adding the No_table_error_handler before the call.
   raise_condition() also fixed.
=== modified file 'sql/sql_class.cc'
--- a/sql/sql_class.cc  2014-01-23 18:21:02 +
+++ b/sql/sql_class.cc  2014-01-31 10:33:03 +
@@ -1145,7 +1145,6 @@ MYSQL_ERROR* THD::raise_condition(uint s
  got_warning= 1;
  break;
case MYSQL_ERROR::WARN_LEVEL_ERROR:
-mysql_audit_general(this, MYSQL_AUDIT_GENERAL_ERROR, sql_errno, msg);
  break;
default:
  DBUG_ASSERT(FALSE);
@@ -1156,6 +1155,8 @@ MYSQL_ERROR* THD::raise_condition(uint s
  
if (level == MYSQL_ERROR::WARN_LEVEL_ERROR)

{
+mysql_audit_general(this, MYSQL_AUDIT_GENERAL_ERROR, sql_errno, msg);
+

This is good.


=== modified file 'sql/sql_show.cc'
--- a/sql/sql_show.cc   2013-11-19 12:16:25 +
+++ b/sql/sql_show.cc   2014-01-31 10:33:03 +
@@ -4199,8 +4199,13 @@ static int fill_schema_table_from_frm(TH
key_length= create_table_def_key(thd, key, table_list, 0);
hash_value= my_calc_hash(table_def_cache, (uchar*) key, key_length);
mysql_mutex_lock(LOCK_open);
-  share= get_table_share(thd, table_list, key,
- key_length, OPEN_VIEW, not_used, hash_value);
+  {
+No_such_table_error_handler no_such_table_handler;
+thd-push_internal_handler(no_such_table_handler);
+share= get_table_share(thd, table_list, key,
+key_length, OPEN_VIEW, not_used, hash_value);
+thd-pop_internal_handler();
+  }

But I don't understand that. Do you mean, old code did *not* suppress
errors here? How could selects from I_S tables work without it?

Regards,
Sergei


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] uint6korr optimization

[Alexey Botchkov]

Thanks for the suggestions, Kristian.
I for some reason didn't notice that __builtin_bswap things.

Best regards.
HF


23.01.2014 19:51, Kristian Nielsen wrote:

Kristian Nielsen kniel...@knielsen-hq.org writes:


Do it like this:
static inline ulonglong
mi_uint6korr(const void *p)
{
   uint32 a= *(uint32 *)p;
   uint16 b= *(uint16 *)(4+(char *)p);
   ulonglong v= ((ulonglong)a | ((ulonglong)b  32))  16;
   asm (bswapq %0 : =r (v) : 0 (v));
   return v;
}

Note that GCC also has __builtin_bswap64() (and __builtin_bswap32()). They
also generate bswap instruction, but would also work on other platforms...

  - Kristian.



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] MDEV-5273

[Alexey Botchkov]

Hi, Serg, all.

I'm not sure how about fixing this one:
https://mariadb.atlassian.net/browse/MDEV-5273
So would like your opinion.

The thing is that the SHOW commands with old-fasioned implementations
(those that aren't mapped to the SELECT FROM information_schema.something)
return stmt-field_count == 0 after the mysql_prepare_statement.

The fix i see involves some boring coding. Is it worth to get into it 
right now?


Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] uint6korr optimization

[Alexey Botchkov]

Hi all.

Only question i have with this is if there's a possibility to make the
hf_mi_uint6korr(dest,src) macro a function.
So we can write as usual
dest= hf_mi_uint6korr(src)

Didn't find how that can nicely be done with the assembler code.

Best regards.
HF

23.01.2014 16:20, Michael Widenius wrote:

Hi!


Alexey == Alexey Botchkov holyf...@askmonty.org writes:

Alexey Hi, Monty.
Alexey I've looked at that part of the code, and tried to make the improved
Alexey versions of uint6korr and mi_uint6korr.

Alexey The small program attached that benchmarks these.
Alexey Depending on enabled macros it loops uint6korr, hf_uint6korr,
Alexey mi_uint6korr and hf_mi_uint6korr respectively. It performs 2 loops on
Alexey each functions first runs it once, the second - twice, so we can
Alexey calculate how much time was spent on the operation itself.

Alexey The results i got so far are:
Alexey elapsed 103 seconds on korr6-1
Alexey elapsed 190 seconds on korr6-2
Alexey elapsed 50 seconds on hf_korr6-1
Alexey elapsed 79 seconds on hf_korr6-2
Alexey elapsed 106 seconds on mi6-1
Alexey elapsed 195 seconds on mi6-2
Alexey elapsed 56 seconds on hf_mi6B-1
Alexey elapsed 88 seconds on hf_mi6-2

Alexey So the
Alexey   hf_uint6korr is 3 times faster than uint6korr.
Alexey   hf_mi_uint6korr is 2.8 times faster than mi_uint6korr.

Alexey You're welcome to check the code out.

Thanks.

What is important is to get fast versions of

mi_uint3korr  (Used a lot in ma_dynrec.c and gis)
mi_uint4korr
mi_uint5korr
mi_uint6korr
mi_uint7korr
mi_uint8korr  (Used for some variables)

uint5korr
uint6korr (Used a lot in Aria)
uint8korr

(I am including the full test so that anyone can comment upon this)

--
#include stdio.h
#include time.h
#include malloc.h

#define TEST_KORR6
#define TEST_HF_KORR6
#define TEST_MI6
#define TEST_HF_MI6

#define uint6korr(A)((ulonglong)(((uint32)((uchar) (A)[0]))  + \
(((uint32)((uchar) (A)[1]))  
8)   + \
(((uint32)((uchar) (A)[2]))  
16)  + \
(((uint32)((uchar) (A)[3]))  
24)) + \
  (((ulonglong) ((uchar) (A)[4]))  32) +   \
  (((ulonglong) ((uchar) (A)[5]))  40))
#define mi_uint6korr(A) ((ulonglong)(((uint32) (((const uchar*) (A))[5])) +\
   (((uint32) (((const uchar*) (A))[4])) 
 8) +\
   (((uint32) (((const uchar*) (A))[3])) 
 16) +\
   (((uint32) (((const uchar*) (A))[2])) 
 24)) +\
 (((ulonglong) (((uint32) (((const uchar*) 
(A))[1])) +\
(((uint32) 
(((const uchar*) (A))[0])  8 \
   32))

#define hf_uint6korr(A) (((ulonglong) ((uint32 *) (A))[0]) + (((ulonglong) ((uint16 
*) (A))[2])  32))
#define hf_mi_uint6korr(src, dest) \
 __asm__ (   \
   bswapq %1;  \
   mov %1, %0; \
   :=r(dest) \
   :r(hf_uint6korr(src)16)\
   : \
 )

typedef unsigned long long int ulonglong;
typedef unsigned int uint32;
typedef unsigned char uchar;
typedef unsigned short uint16;

time_t t0, t1;
ulonglong i;

#define GM 100LL
#define BAS 200

int main()
{
   ulonglong *pb, *pb2;
   char *art, *art2;
   ulonglong ci;

   art= malloc(6*BAS);
   pb= malloc(sizeof(ulonglong)*BAS);
   for (i=0; i6*BAS; i++)
 art[i]= (char)i;

   art2= malloc(6*BAS);
   pb2= malloc(sizeof(ulonglong)*BAS);
   for (i=0; i6*BAS; i++)
 art2[i]= (char)i;

#ifdef TEST_KORR6
   ci= 0;
   t0= time(0);
   for (i=0; iGM; i++)
   {
 if (ci = BAS)
   ci= 0;

 pb[ci]= uint6korr(art+ci*6);
 ci++;
   }
   t1= time(0);
   printf(elapsed %d seconds on korr6-1\n, t1 - t0);

   ci= 0;
   t0= time(0);
   for (i=0; iGM; i++)
   {
 if (ci = BAS)
   ci= 0;
 pb[ci]= uint6korr(art+ci*6);
 pb2[ci]= uint6korr(art2+ci*6);
 ci++;
   }
   t1= time(0);
   printf(elapsed %d seconds on korr6-2\n, t1 - t0);
#endif /*KORR6*/

#ifdef TEST_HF_KORR6
   ci= 0;
   t0= time(0);
   for (i=0; iGM; i++)
   {
 if (ci = BAS)
   ci= 0;

 pb[ci]= hf_uint6korr(art+ci*6);
 ci++;
   }
   t1= time(0);
   printf(elapsed %d seconds on hf_korr6-1\n, t1 - t0);

   ci= 0;
   t0= time(0);
   for (i=0; iGM; i++)
   {
 if (ci = BAS)
   ci= 0;
 pb[ci]= hf_uint6korr(art+ci*6);
 pb2[ci]= hf_uint6korr(art2+ci*6);
 ci++;
   }
   t1= time(0);
   printf(elapsed %d seconds on hf_korr6-2\n, t1 - t0);
#endif /*HF_KORR6*/

#ifdef TEST_MI6
   ci= 0;
   t0= time(0);
   for (i=0; iGM; i++)
   {
 if (ci

Re: [Maria-developers] [Spatial] On current implementation approach

[Alexey Botchkov]

Yes, but my question is not really about location of computational geometry
bits, but about the data management: SQL data type for geometry objects,
input/output routines.


That what i meant. That code do not use any Field data structures.


1. Field is the only place that defines GEOMETRY type (and there is no
CREATE TYPE support)


For the Field the GEOMETRY is just the string of bytes.


2. UDF prototypes will use of GEOMETRY in their prototypes to declare
input/output parameters
then I couldn't understand how it is possible to remove geometry
definitions from Field
and other internal definitions.



That what i guess i missed in your question.
Yes if we remove the GEOMETRY from the server, there's no way to specify the
GEOMETRY type for the UDF. Though instead we can specify strings as 
parameters

and treat them as GEOMETRY inside.

Best regards.
HF



24.09.2013 4:38, Mateusz Loskot wrote:

On 23 September 2013 22:10, Alexey Botchkov holyf...@askmonty.org wrote:

1. Is it possible to implement MariaDB extensions like Spatial (custom
type + set of functions) without such a tight coupling with the
internal implementation of the type system (without messing Field
class with geometry types directly, etc.)?


Yes, it is possible. The core algorithms are separated from the Field
structure and any other database internals.
They are placed in sql/gcalc_slicescan.cc and sql/gcalc_tools.cc files.

Yes, but my question is not really about location of computational geometry
bits, but about the data management: SQL data type for geometry objects,
input/output routines.

Due to my lack of experience with MariaDB/MySQL UDF, I simply assumed that if:
1. Field is the only place that defines GEOMETRY type (and there is no
CREATE TYPE support)
2. UDF prototypes will use of GEOMETRY in their prototypes to declare
input/output parameters
then I couldn't understand how it is possible to remove geometry
definitions from Field
and other internal definitions.

But, I've just found this project [1] with extra spatial UDFs, so I
think I understand the UDF
protocol regarding I/O arguments would not require explicit GEOMETRY type
making it possible to move Spatial Extensions completely out of
built-ins (trunk/sql/ files).

[1] https://github.com/krandalf75/MySQL-Spatial-UDF


2. Is it possible to implement Spatial using User-Defined Functions
(UDF) defined in shared binary?


The spatial functions/operations can be implemented with UDF, but
that makes query optimization and using Spatial keys problemmatic.

So, for real use case, the idea I brainstormed above would not make sense.
Unless, there is workaround for those problems you mean.


3. What is the reason behind using Well-Known-Binary (WKB) stream of
bytes to transport geometry values into/from functions? Is it due to
limitations of MariaDB type system where String is  the only universal
carrier for complex data? This concern is related to necessity of
encoding/decoding WKB when chaining spatial function calls, and
possibilities to avoid it.


The reason was mostly historical. It was sufficient for the first
implementations of the Geometry field types and somewhat convenient as
we don't need to perform conversions
when we need to import/export features in their WKB representation.
But yes, that format is inefficient and difficult to handle properly. I plan
to get rid of it internally - only support importing-exporting it.

I roughly understand, but how do you plan to pass geometry data around,
in what format?

AFAIU, it is not possible to pass user-defined types into/from SQL functions,
so geometries would have to be passed as String objects anyway, wouldn't they?
IOW, there are only 3 types available (integer, real, string), so
String is the only one
usable to pass geometry objects around, regardless of actual encoding format,
WKB, WKT, any other binary stream...

It means, that if I want to pass geometry to my_foo UDF:

MSUDF_API char*
my_foo(UDF_INIT *initid,UDF_ARGS *args, char *buf, unsigned long
*length, char *is_null, char *error);

the only option available is to make geometry into a kind of stream of bytes
and passed as one of args item.
So, a kind of serialising/deserialising is in fact unavoidable.

Is my understanding correct?

Best regards,



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-4472 Audit-plugin. Server-related part of the task

[Alexey Botchkov]

07.09.2013 16:14, Sergei Golubchik wrote:

Hi, Holyfoot!
+  //local_mysql_data_home= (char *)GetProcAddress(0, mysql_data_home);
+//#else
+  //local_mysql_data_home= mysql_data_home;
+//#endif /*_WIN32*/
I think you can completely remove this and the #ifndef above.
For your MySQL plugin you can simply #include this in the plugin body


Right.
I actually meant it but something went wrong with my file versions. Sorry.
The correct patch was resent.



int result;
I believe you've added recently a test to disable rotation if number of
rotations is 0?




Yes.
Though that was added in the SERVER AUDIT tree.
But since i'm recommitting, i moved the change here.

See the new patch here:
http://lists.askmonty.org/pipermail/commits/2013-September/005355.html

Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Rev 3876: MDEV-4472 Audit-plugin. Server-related part of the task

[Alexey Botchkov]

03.09.2013 16:02, Sergei Golubchik wrote:

Hi, Holyfoot!

unsigned long long general_rows;
+  /* Added for 0x302 */
A bit confusing. Better Added in version 0x0302


Fixed.




+  long long query_id;

should it rather be unsigned?


For the THD structure we have:
typedef int64 query_id_t;
So that it's originally unsigned.



+  char *local_mysql_data_home;
+#ifdef _WIN32
+  local_mysql_data_home= (char *)GetProcAddress(0, mysql_data_home);
+#else
+  local_mysql_data_home= mysql_data_home;
+#endif /*_WIN32*/
You shouldn't need it, file_logger.c is part of the server, not a
dynamically loaded plugin.


For now the plugin supposed to handle older Maria's and even MySQL.
So we have to include the file-logger code in there.


  
/*

  I don't think we ever need more rotations,
=== modified file 'sql/sql_audit.h'
--- a/sql/sql_audit.h   2013-04-19 10:50:16 +
+++ b/sql/sql_audit.h   2013-09-03 08:25:21 +
@@ -93,7 +93,8 @@ void mysql_audit_general_log(THD *thd, t
  
  mysql_audit_notify(thd, MYSQL_AUDIT_GENERAL_CLASS, MYSQL_AUDIT_GENERAL_LOG,

 0, time, user, userlen, cmd, cmdlen,
-   query, querylen, clientcs, 0);
+   query, querylen, clientcs, (ha_rows) 0,
+   thd-db, thd-db_length);

no query_id?


}
  }
  
@@ -139,7 +140,8 @@ void mysql_audit_general(THD *thd, uint
  
  mysql_audit_notify(thd, MYSQL_AUDIT_GENERAL_CLASS, event_subtype,

 error_code, time, user, userlen, msg, msglen,
-   query.str(), query.length(), query.charset(), rows);
+   query.str(), query.length(), query.charset(), rows,
+   thd-db, thd-db_length);

no query_id?


No query_id.
An appropriate 'handler' is called inside the mysql_audit_notify for 
each type of events, receiving

the 'thd' as a parameter.
There the event-query_id is actually set. See general_class_handler() 
in sql/sql_audit.cc for example.






}
  }
  
=== modified file 'sql/sql_plugin.cc'

--- a/sql/sql_plugin.cc 2013-06-13 18:19:11 +
+++ b/sql/sql_plugin.cc 2013-09-03 08:25:21 +
@@ -2645,13 +2647,16 @@ static void update_func_longlong(THD *th
  static void update_func_str(THD *thd, struct st_mysql_sys_var *var,
   void *tgt, const void *save)
  {
-  char *old= *(char **) tgt;
-  *(char **)tgt= *(char **) save;
+  char *value= *(char**) save;
if (var-flags  PLUGIN_VAR_MEMALLOC)
{
-*(char **)tgt= my_strdup(*(char **) save, MYF(0));
+char *old= *(char**) tgt;
+if (value)
+  *(char**) tgt= my_strdup(value, MYF(0));

add

else
  *(char**) tgt= 0;


  my_free(old);
}
+  else
+*(char**) tgt= value;
  }
  




Added.

Best regards.
HF




___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Rev 3876: MDEV-4472 Audit-plugin. Server-related part of the task

[Alexey Botchkov]

04.09.2013 16:06, Sergei Golubchik wrote:

Hi, Alexey!

On Sep 04, Alexey Botchkov wrote:

03.09.2013 16:02, Sergei Golubchik wrote:

+  long long query_id;

should it rather be unsigned?

For the THD structure we have:
typedef int64 query_id_t;
So that it's originally unsigned.

You mean originally signed. Yes, I know.
But I think that for API purposes we'd better make it unsigned.


Ok, the event-query_id is of 'unsigned long long' type now.





+  char *local_mysql_data_home;
+#ifdef _WIN32
+  local_mysql_data_home= (char *)GetProcAddress(0, mysql_data_home);
+#else
+  local_mysql_data_home= mysql_data_home;
+#endif /*_WIN32*/
You shouldn't need it, file_logger.c is part of the server, not a
dynamically loaded plugin.

For now the plugin supposed to handle older Maria's and even MySQL.
So we have to include the file-logger code in there.

Yes. But this file is part of the server code, not part of the plugin.
The plugin is not even in the tree.



I removed the local_mysql_data_home. Just the declaration and the use of 
the mysql_data_home left.


Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-4472 (audit plugin)

[Alexey Botchkov]

Sergei,

The complete patch is here:
http://lists.askmonty.org/pipermail/commits/2013-July/004880.html


 #ifndef MARIADB_ONLY
 #undef MYSQL_SERVICE_LOGGER_INCLUDED
 #undef MYSQL_DYNAMIC_PLUGIN



^^^ why undefs?


I want the service_logger.h to be included in no-mysql-dynamic-plugin mode.
But the rest of the services should be included normally, as i belive.
So in the code i first do #define MYSQL_SERVICE_LOGGER_INCLUDED, so
the #include services.h didn't actually included the service_logger.h.

Then I do the #undef and #include service_logger.h separately.


Could you please move HASH into a service?


Ok, will do right after this task (so it won't be forgotten).
But yes, I think it's better to do it separately.

Best regards.
HF


02.07.2013 14:34, Sergei Golubchik wrote:

Hi, Alexey!

On Jul 01, Alexey Botchkov wrote:

Hi, Serg.

Here is the last version of the plugin for your review:
http://myoffice.izhnet.ru/~alex/server_audit/server_audit.c
http://myoffice.izhnet.ru/%7Ealex/server_audit/server_audit.c

I think I addressed there everything we discussed.

Could you send a complete patch instead, please?
Looks ok, a couple of comments:

   #ifndef MARIADB_ONLY
   #undef MYSQL_SERVICE_LOGGER_INCLUDED
   #undef MYSQL_DYNAMIC_PLUGIN

^^^ why undefs?
I wouldn't need to ask this question, if I'd have a complete patch,
that includes file_logger.c

   ...
 if (gethostname(servhost, sizeof(servhost)))
   strcpy(servhost, NA);
^^^
It's usually written as N/A and means not applicable. But the host
name here is most certainly applicable. Better use ??? or unknown or
even localhost.

And two more comments:
1. I presume the complete patch includes test cases for the new plugin.
2. Could you please move HASH into a service? In a separate changeset,
of course. Moving an existing function into a service does not change
the source code of plugins, so you can do it after you've pushed
this audit plugin, if you'd like.

Regards,
Sergei



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-4472 (audit plugin)

[Alexey Botchkov]

Hi, Serg.

Here is the last version of the plugin for your review:
http://myoffice.izhnet.ru/~alex/server_audit/server_audit.c 
http://myoffice.izhnet.ru/%7Ealex/server_audit/server_audit.c


I think I addressed there everything we discussed.

Best regards.
HF

25.06.2013 15:15, Sergei Golubchik wrote:

Hi, Alexey!

On Jun 25, Alexey Botchkov wrote:

Modified version of the file is in the same place:

Ok




___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] MDEV-4472 (audit plugin)

[Alexey Botchkov]

Hi, Sergei.

Here is the plugin code for you to check out.
It constists of a single new file, so i send you the link to it.
http://myoffice.izhnet.ru/~alex/server_audit/server_audit.c

What's missing in it as i'm not sure how it can be done:
- No server host/ip in the log. How get that information?
- SYSLOG logging.
  The SysLog i knew was declared in the syslog.h. That's the 
interface talking to the

  local syslogd instance.
  If it's what we need to use, then i have no idea how to 
specify the Syslog server IP for that
  daemon (which is required by the spec). I see nothing in the 
API about it and i thought
  it should be configured with the daemon's cfg file or 
something like that.



Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Rev 3497: MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE (MWL #252)

[Alexey Botchkov]

Hi, Sergei.


remove your new opt_if_not_exists_ident rule, write it like
-  normal_key_type opt_ident key_alg '(' key_list ')'
+  normal_key_type opt_if_not_exists opt_ident key_alg '(' key_list ')'


I don't think it's a good idea. The 'opt_if_not_exists_ident' actually 
has the body:

opt_if_not_exists_ident:
opt_if_not_exists opt_ident
{
  LEX *lex= Lex;
  if (lex-check_exists  lex-sql_command != SQLCOM_ALTER_TABLE)
  {
my_parse_error(ER(ER_SYNTAX_ERROR));
MYSQL_YYABORT;
  }
  $$= $2;
};

And i'd need to copy this everywhere instead of opt_**_ident call.

Best regards.
HF


25.03.2013 14:40, Sergei Golubchik wrote:

Hi, Holyfoot!

Please push, with one little change:

On Feb 19, holyf...@askmonty.org wrote:

message:
   MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE (MWL #252).
   Syntax modified to allow statements:
 ALTER TABLE ADD/DROP COLUMN
 ALTER TABLE ADD/DROP INDEX
 ALTER TABLE ADD/DROP FOREIGN KEY
 ALTER TABLE ADD/DROP PARTITION
 ALTER TABLE CHANGE COLUMN
 ALTER TABLE MODIFY COLUMN
 DROP INDEX
   to have IF (NOT) EXISTS options.
   Appropriate implementations added to mysql_alter_table().

...

  key_def:
-  normal_key_type opt_ident key_alg '(' key_list ')'
+  normal_key_type opt_if_not_exists_ident key_alg '(' key_list ')'
{ Lex-option_list= NULL; }

remove your new opt_if_not_exists_ident rule, write it like

-  normal_key_type opt_ident key_alg '(' key_list ')'
+  normal_key_type opt_if_not_exists opt_ident key_alg '(' key_list ')'

Regards,
Sergei




___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Rev 3663: MDEV-3819 missing constraints for spatial column types

[Alexey Botchkov]

+ERROR HY000: Incorrect POLYGON value: 'POINT'



I'm sorry, but you got it backwards :)

The value above is POLYGON. The type is POINT.
The error message should be

  ERROR HY000: Incorrect POINT value: 'POLYGON'

Regards,
Sergei



How do you know the difference i wonder :)
But fine, fixing that way...

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Summary of the MDEV-3882 (.deb upgrade from mysql to mariadb fails due to apt-get considering it a downgrade)

[Alexey Botchkov]

Hello.


would help us only temporarely - to the moment Oracle releases even
later' version.

We can use epoch to ensure that our version will always be bigger than the
official Debian/Ubuntu version of MySQL:

 http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Version

Something like 1:5.5.29-mariadb1~wheezy. This will always be prefered by
apt-get over any official Debian package.


Yep, that will always be preferred by apt-get.
So we can easily solve it this way.


I didn't find anything that could affect apt-get and guys on #Debian
didn't recommend anything.

I believe it works to specify desired version explicitly, as mentioned in the
bug report as workaround:

 sudo apt-get install mariadb-server libmysqlclient18=5.5.29-mariadb1~wheezy


Yes, but my intention was to make it working plainly.


So for now i'd do about this:
- change nothing in the existing mariadb-produced debian packages.
- recommend to use 'aptitude' installing the mariadb-server on Debian.
So how do you like my proposal? Any other ideas?

I think it is better if we use the epoch to make MariaDB upgrades work
automatically with both apt-get and aptitude, like they used to.


Agree.




Did you check how Percona handles the similar problem?



Percona deals in similar way as far as i see.
Though they don't specify an Epoch, their Version is just sligthly 
'later' than the native.

libmysqlclient16 in squeeze for instance:
native package: 5.1.66-0+squeeze1
percona's : 5.1.67-rel14.3-506.squeeze

Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE

[Alexey Botchkov]

18.12.2012 15:30, Sergei Golubchik wrote:

And one more thing.
It's easy to imagine a user that likes to do the ALTER IF EXISTS
queries often.

But even if the 'new_field' exists already, ADD FIELD IF EXISTS
new_field still does a lot.
The list of calls is:
   mysql_create_table_no_lock()  - creates the .frm file with the
   temporary name
   mysql_rename_table()
   mysql_rename_table()   once more
   quick_rm_table()  - removes the old .frm
   query_cache_invalidate()
   reopen_table()

Hmm, I don't see it. If the field exists, mysql_alter_table will notice
it in compare_tables() - that is, before mysql_create_table_no_lock().



It's easy to see, just run the ALTER TABLE ADD FIELD IF NOT EXISTS (int 
existing_field).
If the added field already exists, the ALTER TABLE will recreate the 
table anyway.


The can mysql_compare_tables notice it. But in the IF EXISTS case we 
don't break

the command. So that the alter_table process will be continued.
We can check in the mysql_compare_tables() if the tables are equal and
return the specific code for that. But it's not trivial to implement.
It wasn't a problem until this MDEV as the ALTER list could never be 
empty. But

now i think it can be useful to perform the NOOP ALTER possible faster.

Otherwise i'm working now to reform the mysql_alter_table() so we can do 
the IF EXISTS
checks in the same places where we return the errors about (not yet) 
existing objects.
It seems working, but the modification is going to be significant. Are 
we ok with big changes

in this part of code?

Regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE

[Alexey Botchkov]

30.07.2012 18:27, Sergei Golubchik wrote:

Hi, Alexey!

First: You didn't include all my comments in your reply.
Should I assume that all omitted comments you simply agree with and
you will change the patch accordingly?

That's right.


Why did you do that in a separate function? You have basically
duplicated all checks. normal alter table code checks for duplicate
columns/key names and for non-existant columns/keys for DROP.

this code is still in place after your patch. so, basically,
you'll check for duplicates and non-existant names twice.
what's the point of that?

I'd expect you to take check_exists flag into account
in the existing checking code and convert errors into warnings
as appropriate.

Naturally i started doing it that way initially.
Then i realized the code looks rather ugly. These checks are scattered
around the code and parts of the code with these checks are used not
only for ALTER TABLE, but for CREATE and other internal needs.

=== modified file 'sql/sql_table.cc'
--- sql/sql_table.cc2012-04-05 21:07:18 +
+++ sql/sql_table.cc2012-07-11 14:28:40 +
@@ -5788,6 +5788,226 @@ is_index_maintenance_unique (TABLE *tabl
   
   
   /*

+  Preparation for table creation
+
+  SYNOPSIS
+handle_if_exists_option()
I didn't spend on this much time, but grepping for
ER_DUP_FIELDNAME and ER_DUP_KEYNAME (which, I suppose, should show all
checks), didn't show all that many places. Only in sql_table.cc, and
only in mysql_prepare_create_table().


That's not enough. You also should grep for ER_SAME_NAME_PARTITION,
ER_DROP_PARTITION_NON_EXISTENT, ER_CANT_DROP_FIELD_OR_KEY,
ER_BAD_FIELD_ERROR




There we need to be sure these check_if_exist flags aren't set.

Yes, but because it's not set by default, it should be always unset,
unless you set it explicitly in sql_yacc.yy, right?


Yes. Still i can imagine one setting that value and we're having 
mysterious bugs as a result.



And the loops should be done in a different way in these places so we
can remove items from the list in the process.

It is already supported by the current code - both for fields and keys.


Not really.
For fields it works like that:
mysql_alter_table()
calls mysql_prepare_alter_table()
 there the new_alter_info is formed from all the old fields 
that weren't dropped

 and all the added fields. No duplicated fields checks yet.
 then it calls compare_tables()
   which calls mysql_prepare_create_table()
which finally discovers the duplicated fields. The 
code to remove the field with the 'if_exists' mark
from the list look ugly there. Which is worse, 
there it's no use to remove something
from the list there at all as it gets only a 
temporary copy of the Alter_info.
Later in the code we call the same 
mysql_prepare_create_table() with the real data.


For partitions code is not nice for removing something from the list at all.
And we have to provide the 'check_exists' flag to the remote functions
like partition_info::check_partition_info().


And one more thing.
It's easy to imagine an user that likes to do the ALTER IF EXISTS 
queries often.


But even if the 'new_field' exists already, ADD FIELD IF EXISTS 
new_field still does a lot.

The list of calls is:
 mysql_create_table_no_lock()  - creates the .frm file with the 
temporary name

 mysql_rename_table()
 mysql_rename_table()   once more
 quick_rm_table()  - removes the old .frm
 query_cache_invalidate()
 reopen_table()

Using the separate function like i did it's easy to notice that the 
current ALTER is NOOP and return at once.




Thus i decided to do that in the separate function to be called from
the mysql_alter_table exclusively.

In my opinion, the fuction won't affect the performance of the
existing functions at all. And it's easy to understand what and why it
does, so the code duplication will produce the more error-prone code
here.

Well, did i convince you the separate function is nicer?

I'm sorry. Not quite.
If the current code is structured badly, it does not mean that we should
add a special function with duplicate checks for every new task.

If you think the current code is ugly, please, do a little refactoring
and move these checks to separate functions, that you can easily extend
for your purposes.




So i see two options - seriously revise the way mysql_alter_table works or
keep the EXISTS handling in the separate function.
BTW i don't feel the separate function is that bad. I don't duplicate that
much code there besides the loops. But we do such loops in many places 
anyway.


Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE

[Alexey Botchkov]

$ bzr diff dif2
   bzr: ERROR: exceptions.UnboundLocalError: local variable 'diff_file'
referencedbefore assignment

Python is sentisive to indentation levels. May be you've messed up the
whitespace when copy-pasting from the kb? I'll attach the correct
__init__.py to this email.



Yep, that was the reason. Thanks.

Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE

[Alexey Botchkov]

11.07.2012 20:14, Sergei Golubchik wrote:

+Note   1061Duplicate key name 'x_param'
+ALTER TABLE t1 ADD KEY IF NOT EXISTS x_param(x_param);
typo? you test ADD KEY IF NOT EXISTS twice.


Yes, but not this line. The 'KEY x_param' wasn't supposed to be created 
with the CREATE TABLE.



uint8 create_view_check;
bool drop_if_exists, drop_temporary, local_file, one_shot_set;
+  bool check_exists; /* Enabled if the IF [NOT] EXISTS specified for ALTER */
I think, you can remove drop_if_exists and use check_exists instead


Seems to work so far...


+opt_if_not_exists_ident:
+opt_if_not_exists opt_ident
+{
+  LEX *lex= Lex;
+  if (lex-check_exists  lex-sql_command != SQLCOM_ALTER_TABLE)
+  {
+my_parse_error(ER(ER_SYNTAX_ERROR));
+MYSQL_YYABORT;
+  }
+  $$= $2;
+};
+

why did you create opt_if_not_exists_ident rule, instead of adding
opt_if_not_exists before ident in the alter table grammar?


I'm afraid if i didn't create this rule, i had to add this code to 4 
cases of the 'key_def:'

And i'd also had to modify more lines, as numbers of parameters after the
opt_if_not_exists would increase.

  
+opt_column_if_exists:

+opt_column if_exists
+{}
+;
+
same as above - why a special rule?


Here the sole reason is to avoid modifying the code with the increased 
parameter's numbers.
Ok, i can it's not enough to add a separate rule, going to get rid of 
this one.



=== modified file 'sql/sql_table.cc'
--- sql/sql_table.cc2012-04-05 21:07:18 +
+++ sql/sql_table.cc2012-07-11 14:28:40 +
@@ -5788,6 +5788,226 @@ is_index_maintenance_unique (TABLE *tabl
  
  
  /*

+  Preparation for table creation
+
+  SYNOPSIS
+handle_if_exists_option()

Why did you do that in a separate function? You have basically duplicated
all checks. normal alter table code checks for duplicate columns/key names
and for non-existant columns/keys for DROP.

this code is still in place after your patch. so, basically,
you'll check for duplicates and non-existant names twice.
what's the point of that?

I'd expect you to take check_exists flag into account
in the existing checking code and convert errors into warnings
as appropriate.


Naturally i started doing it that way initially.
Then i realized the code looks rather ugly. These checks are scattered 
around the code and
parts of the code with these checks are used not only for ALTER TABLE, 
but for CREATE and other internal needs.

There we need to be sure these check_if_exist flags aren't set.
And the loops should be done in a different way in these places so we 
can remove items from the list in the process.
Thus i decided to do that in the separate function to be called from the 
mysql_alter_table exclusively.


In my opinion, the fuction won't affect the performance of the existing 
functions at all. And it's easy
to understand what and why it does, so the code duplication will produce 
the more error-prone code here.


Well, did i convince you the separate function is nicer?

Best regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-318 IF (NOT) EXIST clauses for ALTER TABLE

[Alexey Botchkov]

Hi, Sergei.

+++ sql/field.cc2012-07-11 14:28:40 +
@@ -9182,6 +9182,7 @@ void Create_field::init_for_tmp_table(en


First: please, see the last section in
http://kb.askmonty.org/en/how-to-get-more-out-of-bzr-when-working-on-mariadb/
and do as it suggests

I had to apply your patch locally and re-run bzr diff
to get the function names in the diff :(



Wasn't able to start that last section.
I created the .bazaar/diff_p/, the __init__.py, and .bazaar/rules files,
then get:

$ bzr diff dif2
 bzr: ERROR: exceptions.UnboundLocalError: local variable 'diff_file' 
referencedbefore assignment



Although i used other tips from that page so my diff includes the 
function's names already. Maybe that's enough.



Regards
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-136 Non-blocking set read_only

[Alexey Botchkov]

Sergei,

Here is the new patch:
http://lists.askmonty.org/pipermail/commits/2012-May/003324.html

I tested things in various ways, and it seems working decent.


  1st client: LOCK TABLE t1 WRITE;

done


  2nd client: SET READ_ONLY=1;

if the t1 is MyISAM, it hangs until t1 unlocked
if the t1 is InnoDB, it completes immediately.


1st client: INSERT t1 VALUES (1);

if the t1 is MyISAM, the INSERT succeedes at once
while the SET READ_ONLY is hanging in the 2nd 
client.


if the t1 is InnoDB, the INSERT fails
   ERROR 1290 (HY000): The MySQL server is running with the 
--read-only
option so it cannot execute this statement



That's right, it' won't work with the mixed types of tables.
But as the customer didn't ask for that, maybe the simplest solution
will do here.

I'm not sure about it. It's basically a gotcha. SET READ_ONLY will not
flush transactional tables, but only if the statement uses no
non-transactional tables. Otherwise it'll flush all tables,
transactional or not. One cannot always know if non-transactional
tables are involved. Think of log tables, tables used in triggers and
stored routines.




Temporary tables shouldn't be a problem here i think.
Otherwise, I wasn't able to come out with something better.

Regards
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-136 Non-blocking set read_only

[Alexey Botchkov]

Sergei,


11.05.2012 19:44, Sergei Golubchik wrote:

=== modified file 'mysql-test/r/read_only.result'
--- a/mysql-test/r/read_only.result 2009-03-06 14:56:17 +
+++ b/mysql-test/r/read_only.result 2012-05-09 18:59:45 +
@@ -59,7 +59,7 @@
  connection con1;
  select @@global.read_only;
  @@global.read_only
-0
+1
This is prone to race conditions.
Please, fix the test to remove send here and below.
(assuming the new result is correct)




But is it correct ?
Why set read_only is not blocked by a write locked myisam table?


It's not blocked as we ceased incrementing the refresh_version in this case:

+/* No need to close the open tables if we just set the readonly state */
+if (!set_readonly_mode)
+  refresh_version++;   // Force close of open tables
+


Right, this test with the 'send set read_only' should be either removed 
or updated.



Where are the tests for the changed functionality?


No specific tests yet.
I was to lazy to add the new test while i was not sure i did the correct 
thing. Hoped to get the quick answer.



=== modified file 'sql/sql_base.cc'
--- a/sql/sql_base.cc   2012-04-19 06:16:30 +
+++ b/sql/sql_base.cc   2012-05-09 18:59:45 +
@@ -933,7 +938,8 @@
for (uint idx=0 ; idx  open_cache.records ; idx++)
{
  TABLE *table=(TABLE*) hash_element(open_cache,idx);
-if (table-in_use)
+if (table-in_use
+(!set_readonly_mode || !table-file-has_transactions()))

I wonder how this could work. The line below sets a flag *on a thread*.
The task description tells not wait for transactional tables, while your
change means not set a flag if all tables used in a thread are
transactional. That is, if a thread uses both transactional and
non-transactional tables, your change does nothing.


That's right, it' won't work with the mixed types of tables.
But as the customer didn't ask for that, maybe the simplest solution 
will do here.





Back to my first question - where are the tests for this new task?



Will develop some ASAP.

HF

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] MDEV-136 Non-blocking set read_only

[Alexey Botchkov]

16.05.2012 12:48, Michael Widenius написал:

Holyfoot, how did you test this feature?


I started a long-running query in one session, then ran the 'SET GLOBAL 
read_lock=1' in another.
After the change the 'SET read_lock' doesn't wait for the INNODB to 
complete the query.


Regards
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] Rev 3145: MDEV-80 Memory engine table full at much less than max_heap_table_size with btree index. in file:///home/hf/wmar/mdev80/

[Alexey Botchkov]

Sergei,

06.04.2012 1:21, Sergei Golubchik wrote:

Hi, Holyfoot!

So, does that mean you've managed to reproduce the bug?
Or you just found a possible cause for such a behavior and fixed it?



I finally found out how to reproduce this bug and verified the fix cures 
the problem.
(thanks to Jason Preuss who gave me an access to the machine where i 
could experiment)


It'd be probably good to have a test for that issue, but i didn't figure 
out how it can
be done in reality. To reproduce this bug we have to grow the RBtree key 
over 4G.


Regards
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] Rev 3145: MDEV-80 Memory engine table full at much less than max_heap_table_size with btree index. in file:///home/hf/wmar/mdev80/

[Alexey Botchkov]

06.04.2012 1:21, Sergei Golubchik wrote:

So, does that mean you've managed to reproduce the bug?



If you're interested i've reproduced this bug on our terrier machine as 
well.
Thought it takes about half-an-hour to happen. Takes the eternity with 
the debug build.


Regards.
HF


___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Please fix: innodb_plugin.innodb_gis fails on 'gcov' builder

[Alexey Botchkov]

Well there is the
--source include/have_innodb_plugin.inc
there in the test.

The problem is that this checks for 'have innodb or xtradb', and we 
don't have

create table t engine=innodb or xtradb;

Will decide something a bit later.

Regards
HF

14.12.2011 19:03, Kristian Nielsen wrote:

Sergei Petruniapser...@askmonty.org  writes:


We have a repeatable failure like this:

https://internal.askmonty.org/buildbot/builders/gcov/builds/830/steps/test_1/logs/stdio
innodb_plugin.innodb_gis w3 [ fail ]
 Test ended at 2011-12-14 06:52:31

CURRENT_TEST: innodb_plugin.innodb_gis
mysqltest: At line 1: query 'SET storage_engine=innodb' failed: 1286: Unknown 
table engine 'innodb'

The result from queries just before the failure was:
SET storage_engine=innodb;

It's probably just missing

 --source include/have_innodb.inc

at the top.

  - Kristian.



___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] [Commits] Rev 2983: WL#77 - INSTALL PLUGIN * in http://bazaar.launchpad.net/~maria-captains/maria/5.2/

[Alexey Botchkov]

I'm fine with this last patch.
One thing i can think of is if we should add a new error instead of 
adding static english texts to the ER_CANT_OPEN_LIBRARY

like 'API version for ... plugin ... not supported'.
Second question if we should have INSTALL PLUGIN ... IF NOT INSTALLED; 
to be even more user-friendly.


Regards
HF


17.06.2011 21:49, s...@askmonty.org wrote:

At http://bazaar.launchpad.net/~maria-captains/maria/5.2/




___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp