maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

On Wed, Nov 30, 2016 at 3:26 AM, kentoku <kentokushiba@xxxxxxxxx> wrote:
> Hi!
>
>> If this is ok, I will add this and close MDEV-7700
>
> It looks connect string is visible, but never used. This is different
> from what I think.
> I just added patch for connect string to MDEV-7698. Please review it.
> This changes do not cause error for federated_partition test.

Let's discuss this today when you are at my place.

Some quick comments and questions:
- Did you see my patch to remember table->s->connect_string, even if
partition tables are using it.  Why isn't this fixing the problem?

About the patch:

> diff -Narup ./server/sql/ha_partition.cc ./server-with-connect-string-patch/sql/ha_partition.cc
> --- ./server/sql/ha_partition.cc    2016-11-29 19:56:47.037326899 +0900
> +++ ./server-with-connect-string-patch/sql/ha_partition.cc    2016-11-30 09:57:24.000000000 +0900
> @@ -1515,7 +1515,10 @@ int ha_partition::prepare_new_partition(
>    if ((error= set_up_table_before_create(tbl, part_name, create_info, p_elem)))
>      goto error_create;
>
> +  if (!(file->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION))
> +  {
>    tbl->s->connect_string = p_elem->connect_string;
> +  }

I am not sure that adding a HTON is the right way.

Why isn't enough to in all cases just ensure that the handler for the
underlaying tables are not changing the connect string given by the end user?

This is what my patch is doing.

One benefit of my patch is that we will remember the global the connect string
in all cases, which this patch doesn't guarantee.

>    if ((error= file->ha_create(part_name, tbl, create_info)))
>    {
>      /*
> @@ -2554,10 +2560,13 @@ int ha_partition::set_up_table_before_cr
>    }
>    info->index_file_name= part_elem->index_file_name;
>    info->data_file_name= part_elem->data_file_name;
> +  if (!(m_file[0]->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION))
> +  {
>    info->connect_string= part_elem->connect_string;
>    if (info->connect_string.length)
>      info->used_fields|= HA_CREATE_USED_CONNECTION;
>    tbl->s->connect_string= part_elem->connect_string;
> +  }

The above is probably wrong.  If the partition engine had before store
connect strings for the engine, it should be able to read them in the future
too, even if the flags has changed value.


> @@ -3488,11 +3497,17 @@ int ha_partition::open(const char *name,
>     {
>        create_partition_name(name_buff, name, name_buffer_ptr, NORMAL_PART_NAME,
>                              FALSE);
> +      if (!((*file)->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION))
> +      {
>        table->s->connect_string = m_connect_string[(uint)(file-m_file)];
> +      }

I assume this is the real reason for your patch.

You want to ensure that if the partition doesn't support connect
strings and there was no connect strings given to the partition, we
should send in the main table's partition string.

ok, there is a point. I will fix this.

>        if ((error= (*file)->ha_open(table, name_buff, mode,
>                                     test_if_locked | HA_OPEN_NO_PSI_CALL)))
>          goto err_handler;
> +      if (!((*file)->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION))
> +      {
>        bzero(&table->s->connect_string, sizeof(LEX_STRING));
> +      }

I think the above is always wrong. Better to always restore the connect_string
to it's original value.


<cut>

> --- ./server/sql/sql_table.cc    2016-11-29 19:56:47.118326899 +0900
> +++ ./server-with-connect-string-patch/sql/sql_table.cc    2016-11-30 09:47:28.000000000 +0900
> @@ -7978,6 +7978,11 @@ mysql_prepare_alter_table(THD *thd, TABL
>      create_info->comment.str= table->s->comment.str;
>      create_info->comment.length= table->s->comment.length;
>    }
> +  if (!create_info->connect_string.str)
> +  {
> +    create_info->connect_string.str= table->s->connect_string.str;
> +    create_info->connect_string.length= table->s->connect_string.length;
> +  }

We should not need this as we have already in the same function:

  if (!(used_fields & HA_CREATE_USED_CONNECTION))
    create_info->connect_string= table->s->connect_string;

> diff -Narup ./server/sql/table.cc ./server-with-connect-string-patch/sql/table.cc
> --- ./server/sql/table.cc    2016-11-29 19:56:47.129326899 +0900
> +++ ./server-with-connect-string-patch/sql/table.cc    2016-11-30 09:50:45.000000000 +0900
> @@ -3758,6 +3758,7 @@ void update_create_info_from_table(HA_CR
>    create_info->default_table_charset= share->table_charset;
>    create_info->table_charset= 0;
>    create_info->comment= share->comment;
> +  create_info->connect_string= share->connect_string;
>    create_info->transactional= share->transactional;
>    create_info->page_checksum= share->page_checksum;
>    create_info->option_list= share->option_list;

Why is the above needed?
Do you have a test case that breaks if the above is not done?

Regards,
Monty