Re: [Maria-developers] MDEV-14849 CREATE + ALTER with user-invisible columns produce ...

[Sergei Golubchik]

Hi, Sachin!

On Jan 29, Sachin Setiya wrote:
> commit 5bd97f22094f3b6a658d503f98022ccd9076bb19
> Author: Sachin Setiya 
> Date:   Mon Jan 29 12:31:07 2018 +0530
> 
> MDEV-14849 CREATE + ALTER with user-invisible columns produce ...
> 
> Problem:-
>   create or replace table t1 (pk int auto_increment primary key 
> invisible, i int);
>   alter table t1 modify pk int invisible;
>  This last alter makes a invisible column which is not null and does not
>  have default value.
> 
> Analysis:-
>  This is caused because our error check for NOT_NULL_FLAG and
>  NO_DEFAULT_VALUE_FLAG flag misses this sql_field , but this is not the 
> fault
>  of error check :).Actually this field come via mysql_prepare_alter_table
>  and it does not have NO_DEFAULT_VALUE_FLAG flag turned on. (If it was 
> create
>  table NO_DEFAULT_VALUE_FLAG would have turned on 
> Column_definition::check)
>  and this would have generated error.
> 
> Solution:-
>  I have moved the error check to kind last of mysql_prepare_create_table
>  because upto this point we have applied NO_DEFAULT_VALUE_FLAG to required
>  column.

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

Re: [Maria-developers] MDEV-14785 SYSTEM_INVISIBLE behaviour not consistent ---Initial Patch

[Sergei Golubchik]

Hi, Sachin!

On Jan 29, Sachin Setiya wrote:
> commit 5e2f5aa78a5f53b000a2b9c6989c9ed7076e2527
> Author: Sachin Setiya 

Note the typo in your email ^

> Date:   Mon Jan 29 17:09:32 2018 +0530
> 
> MDEV-14785 SYSTEM_INVISIBLE behaviour not consistent
> 
> TODO Commit message
> 
> diff --git a/sql/item.h b/sql/item.h
> index d178d9f..0072dfd 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -2907,11 +2907,17 @@ class Item_field :public Item_ident
>bool check_vcol_func_processor(void *arg)
>{
>  context= 0;
> -if (field && (field->unireg_check == Field::NEXT_NUMBER))
> +if (field)
>  {
> -  // Auto increment fields are unsupported
> -  return mark_unsupported_function(field_name.str, arg, VCOL_FIELD_REF | 
> VCOL_AUTO_INC);
> +  if (field->unireg_check == Field::NEXT_NUMBER)
> +// Auto increment fields are unsupported
> +return mark_unsupported_function(field_name.str, arg,
> +VCOL_FIELD_REF | VCOL_AUTO_INC);
> +  if (field->invisible > INVISIBLE_USER)
> +return mark_unsupported_function(field_name.str, arg,
> +VCOL_FIELD_REF | VCOL_SYSTEM_INVISIBLE);

Why would you do that, instead of hiding system invisible fields in
CREATE TABLE? From the user point of view they aren't fields, they don't
exist in the table, and the error should be "no such field", not
"Function or expression 'on SYSTEM_INVISIBLE column' cannot be used"

>  }
> +
>  return mark_unsupported_function(field_name.str, arg, VCOL_FIELD_REF);
>}
>bool set_fields_as_dependent_processor(void *arg)
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index 17ff7eb..89ba2ed 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -8128,6 +8128,11 @@ fill_record(THD *thd, TABLE *table_arg, List 
> , List ,
>  value=v++;
>  DBUG_ASSERT(value);
>  Field *rfield= field->field;
> +if (rfield->invisible == INVISIBLE_SYSTEM)
> +{
> +  my_error(ER_BAD_FIELD_ERROR, MYF(0), rfield->field_name.str, 
> thd->where);

The error is correct. But do you need to do it in fill_record()?
I'd thought you can do it when the statement is prepared, once, not in
fill_record() which is called for every row, many times per statement.

> +  goto err;
> +}
>  TABLE* table= rfield->table;
>  if (table->next_number_field &&
>  rfield->field_index ==  table->next_number_field->field_index)
> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index d2fd2ce..e4971d7 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -16639,6 +16639,7 @@ Field *create_tmp_field_from_field(THD *thd, Field 
> *org_field,
>  new_field->next_equal_field= NULL;
>  new_field->option_list= NULL;
>  new_field->option_struct= NULL;
> +new_field->invisible= org_field->invisible;

What is it for? It'd mean that temporary tables can have invisible
fields. What bugs does this change fix?

>}
>return new_field;
>  }
> diff --git a/sql/sql_update.cc b/sql/sql_update.cc
> index 7b4938f..7bdedc7 100644
> --- a/sql/sql_update.cc
> +++ b/sql/sql_update.cc
> @@ -1497,6 +1497,9 @@ int mysql_multi_update_prepare(THD *thd)
>TABLE_LIST *table_list= lex->query_tables;
>TABLE_LIST *tl;
>List *fields= >select_lex.item_list;
> +  List_iterator it(*fields);
> +  Item *item;
> +  Item_field *field;
>table_map tables_for_update;
>bool update_view= 0;
>/*
> @@ -1564,7 +1567,21 @@ int mysql_multi_update_prepare(THD *thd)
>{
>  DBUG_RETURN(TRUE);
>}
> -
> +  //Check if we got any INVISIBLE_SYSTEM field
> +  while ((item= it++))
> +  {
> +if (Item::FIELD_ITEM == item->type())
> +{
> +  field= (Item_field *)item;
> +  if (field->field->invisible == INVISIBLE_SYSTEM)
> +  {
> +my_error(ER_BAD_FIELD_ERROR,MYF(0),
> +field->field->field_name.str,
> +field->field->table->s->table_name.str);
> +DBUG_RETURN(TRUE);
> +  }
> +}
> +  }


Dunno. What if I do

  UPDATE ... SET normal_field = row_start

you'll have an invisible field, but it's not updated, so should be ok,
right? May be something like

   if (field->field->invisible == INVISIBLE_SYSTEM &&
   bitmap_is_set(field->table->write_set, field->field_index))
   {
 ... my_error
   }

And why here? I'd think this belongs into check_fields() function.

>thd->table_map_for_update= tables_for_update= get_table_map(fields);

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] [Commits] Mdev-15085 Rebased to latest 10.3

[Sergei Golubchik]

Hi, Sachin!

On Jan 29, Sachin Setiya wrote:
> commit 905806689979ae91c89115f18b107ebacbc489e2
> Author: Sachin Setiya 
> Date:   Fri Jan 26 22:58:34 2018 +0530
> 
> Mdev-15085 Invisible Column Non-constant Default value results...
> 
> Problem:- If we create table field with dynamic default value then that
>  field always gets NULL value.
> 
> Analyze:- This is because in fill_record we simple continue at Invisible
>  column because we though that share->default_values(default value is
>  always copied into table->record[0] before insert) will have a default
>  value for them(which is true for constant defaults , but not for dynamic
>  defaults).
> 
> Solution:- In case of invisible field we will assign default value to 
> 'value'
>  variable, but we do this only for columns which have non dynamic default 
> value.
>  We do not have to worry for Auto_increment field , storage engine will 
> take
>  of it.
> 
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index 45efa63..17ff7eb 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -8382,7 +8382,12 @@ fill_record(THD *thd, TABLE *table, Field **ptr,
> List ,
> 
>  if (field->invisible)
>  {
> -  continue;
> +  if(field->flags & AUTO_INCREMENT_FLAG ||
> +  !field->default_value)
> +continue;
> +  else
> +value= new (thd->mem_root) Item_default_value(thd,
> +>lex->select_lex.context);

No-no, you cannot create items in fill_record(). It would create a new
item for every inserted row.

I think the only thing you need to do here (and it doesn't depend on
AUTO_INCREMENT_FLAG or field->default_value), you need to set
all_fields_have_values to false. Like

  if (field->invisible)
  {
all_fields_have_values= false;
continue;
  }

Indeed, hidden columns did not get a value, so all_fields_have_values
should be false. I'd expect the rest will work automatically :)

>  }
>  else
>value=v++;

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

[Maria-developers] Wording suggestion?

[Sergei Golubchik]

Hi, Ian, Russell,

As you know, UPDATE in MariaDB is executed left-to-right, say, in the
case of

  UPDATE t1 SET a=b, b=a

both columns will have the same value, old value of `b`. We're now
introducing a new sql_mode to have standard compatible behavior, where
all assignments are executed kind of "simultaneously" (and the statement
above would swap the values).

What could the name for this new sql_mode be?

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-13790 UNHEX() of a somewhat complicated CONCAT() returns NULL

[Sergei Golubchik]

Hi, Alexander!

On Jan 29, Alexander Barkov wrote:
> Hi Sergei,
> 
> Thanks for review. Comments follow inline:

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

Re: [Maria-developers] MDEV-13790 UNHEX() of a somewhat complicated CONCAT() returns NULL

[Alexander Barkov]

Hi Sergei,

Thanks for review. Comments follow inline:

On 01/28/2018 07:22 PM, Sergei Golubchik wrote:

Hi, Alexander!

On Nov 03, Alexander Barkov wrote:

Hello Sergei,

Please review a patch for MDEV-13790.

It removes the attempt to search the best suitable buffer
and just returns the result in "str" passed to val_str().

I'm quite sure this optimization gives nothing.
In can be useful only under very rare conditions,
but generates so many bugs.


There're bunch of tricks. What do they all do, could you list them?

One handles the case when an argument is a substring of the already
concatenated string - this is very weird and rare, I agree.

Other accumulates the result in the first argument's result - this
avoids one memcpy per row. Doesn't sound like much, but it's not rare,
so should be very easy to benchmark.


In my patch the first argument goes directly to "str"
parameter of Item_func_concat::val_str().
There should not be an extra memcpy here.



Yet another doubles the buffer size every time - this might be useful to
keep, it easily fits in your new code.


As discussed on IRC, I kept this code.
Please find a new version attached.



What are other tricks that concat was using?

Btw, one comment about your patch — please don't call current_thd in a
loop.


Fixed.



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

diff --git a/mysql-test/r/func_concat.result b/mysql-test/r/func_concat.result
index b87ee7bfc52..9ab6f74653e 100644
--- a/mysql-test/r/func_concat.result
+++ b/mysql-test/r/func_concat.result
@@ -262,3 +262,9 @@ c2
 abcdefghi-abcdefghi
 DROP TABLE t1;
 SET optimizer_switch=@save_optimizer_switch;
+#
+# MDEV-13790 UNHEX() of a somewhat complicated CONCAT() returns NULL
+#
+SELECT UNHEX(CONCAT('414C2', HEX(8 + ROUND(RAND()*7)), SUBSTR(SHA(UUID()),6,33),HEX(2+ROUND(RAND()*8 IS NULL AS c1;
+c1
+0
diff --git a/mysql-test/t/func_concat.test b/mysql-test/t/func_concat.test
index be573f494a2..69dd2c4063e 100644
--- a/mysql-test/t/func_concat.test
+++ b/mysql-test/t/func_concat.test
@@ -236,3 +236,9 @@ SELECT CONCAT(t2,'-',t2) c2 FROM (SELECT TRIM(t) t2 FROM t1) sub;
 DROP TABLE t1;
 
 SET optimizer_switch=@save_optimizer_switch;
+
+--echo #
+--echo # MDEV-13790 UNHEX() of a somewhat complicated CONCAT() returns NULL
+--echo #
+
+SELECT UNHEX(CONCAT('414C2', HEX(8 + ROUND(RAND()*7)), SUBSTR(SHA(UUID()),6,33),HEX(2+ROUND(RAND()*8 IS NULL AS c1;
diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc
index c0d60d7ef8b..378f23f3109 100644
--- a/sql/item_strfunc.cc
+++ b/sql/item_strfunc.cc
@@ -630,138 +630,84 @@ String *Item_func_decode_histogram::val_str(String *str)
 
 ///
 
+/*
+  Realloc the result buffer.
+  NOTE: We should be prudent in the initial allocation unit -- the
+  size of the arguments is a function of data distribution, which
+  can be any. Instead of overcommitting at the first row, we grow
+  the allocated amount by the factor of 2. This ensures that no
+  more than 25% of memory will be overcommitted on average.
+
+  @param IN/OUT str- the result string
+  @param IN length - new total space required in "str"
+  @retval   false  - on success
+  @retval   true   - on error
+*/
+
+bool Item_func_concat::realloc_result(String *str, uint length) const
+{
+  if (str->alloced_length() >= length)
+return false; // Alloced space is big enough, nothing to do.
+
+  if (str->alloced_length() == 0)
+return str->alloc(length);
+
+  /*
+Item_func_concat::val_str() makes sure the result length does not grow
+higher than max_allowed_packet. So "length" is limited to 1G here.
+We can't say anything about the current value of str->alloced_length(),
+as str was initially set by args[0]->val_str(str).
+So multiplication by 2 can overflow, if args[0] for some reasons
+did not limit the result to max_alloced_packet. But it's not harmful,
+"str" will be realloced exactly to "length" bytes in case of overflow.
+  */
+  uint new_length= MY_MAX(str->alloced_length() * 2, length);
+  return str->realloc(new_length);
+}
+
+
 /**
   Concatenate args with the following premises:
   If only one arg (which is ok), return value of arg;
-  Don't reallocate val_str() if not absolute necessary.
 */
 
 String *Item_func_concat::val_str(String *str)
 {
   DBUG_ASSERT(fixed == 1);
-  String *res,*res2,*use_as_buff;
-  uint i;
-  bool is_const= 0;
+  THD *thd= current_thd;
+  String *res;
 
   null_value=0;
-  if (!(res=args[0]->val_str(str)))
+  if (!(res= args[0]->val_str(str)))
 goto null;
-  use_as_buff= _value;
-  /* Item_subselect in --ps-protocol mode will state it as a non-const */
-  is_const= args[0]->const_item() || !args[0]->used_tables();
-  for (i=1 ; i < arg_count ; i++)
-  {
-if (res->length() == 0)
-{
-  if (!(res=args[i]->val_str(str)))
-	goto null;
-  /*
-   CONCAT accumulates its result in the result of its the first
-