maria-developers team mailing list archive

[kentoku <kentokushiba@xxxxxxxxx>]

Hi!

> The above test could be simple:
>
> additional_table_flags &= (ulonglong) ((*file)->ha_table_flags();
>
> Should be the same thing.  I did however remove this code, as
> HA_CAN_BG... are not used anywhere in the current spider code. See
> comment below.

ok.

> I also changed so that m_num_locks always contains the upper limit
> number of locks needed.  Without this change, the call to lock_count() would
> have returneed m_total_parts * 'new-counted-locks' which would be been
> way too much.

ok.

> I added this without the define, but I merged it to the loop for cond_push.

ok.

> Instead of having defines in the sql code, I will fix that in spider and vp
> that I will set the proper defines based on the MariaDB version.

ok. Thanks.

> However, I would like to have a clear comment the purpose of the function
> set_top_table_and_fields().

This function is used to get correlating of a parent (table/column)
and children (table/column). When conditions are pushed down to child
table (like child of myisammarge), child table needs to know about
which table/column is my parent for understanding conditions.

> Do you really need HA_CAN_BG_XXX flags?

No, I don't. I planed to use this flags, but this flags are not used
yet. So, it's ok to remove for now. I'll add it when it is needed.

> + virtual int set_top_table_and_fields(TABLE *top_table,
... cut ...
> Why is this int and not void ?
> Do you have in mind that this may be possible to fail in the future?
> If it's never going to change, then better to do this void.
>
> I will make it void for now and change the relevant code. We can make it
> int again if needed in the future.

VP allocates some memories in this function. So this function has
possibility of causing out of memory error. That's why this function
returns int.

> I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which
> allwoed me to make the above tests and the following identical ones
> much
> simpler.

ok. And now everyone can create myisammarge type storage engine!

Regards,
Kentoku


2016-11-28 19:44 GMT+09:00 Michael Widenius <michael.widenius@xxxxxxxxx>:
> Hi!
>
> Next patch, MDEV 7701
>
> +++ ./003_mariadb-10.2.0-vp/sql/ha_partition.cc    2016-05-05
> 21:25:04.289731712 +0900
> @@ -7238,13 +7239,30 @@ int ha_partition::extra(enum ha_extra_fu
>    }
>      /* Category 9) Operations only used by MERGE */
>    case HA_EXTRA_ADD_CHILDREN_LIST:
> +    DBUG_RETURN(loop_extra(operation));
>    case HA_EXTRA_ATTACH_CHILDREN:
> +    int result;
> +    handler **file;
> +    if ((result = loop_extra(operation)))
> +      DBUG_RETURN(result);
> +    m_num_locks = 0;
> +    additional_table_flags =
> +      (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE);
> +    file = m_file;
> +    do
> +    {
> +      m_num_locks += (*file)->lock_count();
> +      additional_table_flags &= ~((ulonglong) ((*file)->ha_table_flags() ^
> +        (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE)));
>
> The above test could be simple:
>
> additional_table_flags &= (ulonglong) ((*file)->ha_table_flags();
>
> Should be the same thing.  I did however remove this code, as
> HA_CAN_BG... are not used anywhere in the current spider code. See
> comment below.
>
> I also changed so that m_num_locks always contains the upper limit
> number of locks needed.  Without this change, the call to lock_count() would
> have returneed m_total_parts * 'new-counted-locks' which would be been
> way too much.
>
>
> @@ -9123,6 +9141,19 @@ const COND *ha_partition::cond_push(cons
>    handler **file= m_file;
>    COND *res_cond = NULL;
>    DBUG_ENTER("ha_partition::cond_push");
> +#ifdef HANDLER_HAS_TOP_TABLE_FIELDS
> +  if (set_top_table_fields)
> +  {
> +    do
> +    {
> +      if ((*file)->set_top_table_and_fields(top_table,
> +                                        top_table_field,
> +                                        top_table_fields))
> +        DBUG_RETURN(cond);
> +    } while (*(++file));
> +    file= m_file;
> +  }
> +#endif
>
> I added this without the define, but I merged it to the loop for cond_push.
>
> Instead of having defines in the sql code, I will fix that in spider and vp
> that I will set the proper defines based on the MariaDB version.
>
> However, I would like to have a clear comment the purpose of the function
> set_top_table_and_fields().
>
>  struct st_mysql_storage_engine partition_storage_engine=
>  { MYSQL_HANDLERTON_INTERFACE_VERSION };
>
> +++ ./003_mariadb-10.2.0-vp/sql/handler.h    2016-05-05 21:25:04.314731713 +0900
> @@ -255,6 +257,11 @@ enum enum_alter_inplace_result {
>   */
>  #define HA_BINLOG_FLAGS (HA_BINLOG_ROW_CAPABLE | HA_BINLOG_STMT_CAPABLE)
>
> +#define HA_CAN_BG_SEARCH       (1LL << 45)
> +#define HA_CAN_BG_INSERT       (1LL << 46)
> +#define HA_CAN_BG_UPDATE       (1LL << 47)
> +#define HA_CAN_MULTISTEP_MERGE (1LL << 48)
>
> Do you really need HA_CAN_BG_XXX flags?
> I checked your spider tree and this is not used anywhere, so I think
> we should leave them out for now.
> If we don't need these, we can remove the loop for setting them in
> HA_EXTRA_ATTACH_CHILDREN too.
>
> If we need them, I would like to have a comment for why they are needed and
> when we will need then add these in a separate patch.
>
> @@ -3525,6 +3540,29 @@ public:
>     Pops the top if condition stack, if stack is not empty.
>   */
>   virtual void cond_pop() { return; };
> + virtual int set_top_table_and_fields(TABLE *top_table,
> +                                      Field **top_table_field,
> +                                      uint top_table_fields)
> + {
> +   if (!set_top_table_fields)
> +   {
> +     set_top_table_fields = TRUE;
> +     this->top_table = top_table;
> +     this->top_table_field = top_table_field;
> +     this->top_table_fields = top_table_fields;
> +   }
> +   return 0;
> + }
>
> Why is this int and not void ?
> Do you have in mind that this may be possible to fail in the future?
> If it's never going to change, then better to do this void.
>
> I will make it void for now and change the relevant code. We can make it
> int again if needed in the future.
>
> diff -Narup ./002_mariadb-10.2.0-spider/sql/sql_base.cc
> ./003_mariadb-10.2.0-vp/sql/sql_base.cc
> --- ./002_mariadb-10.2.0-spider/sql/sql_base.cc    2016-04-17
> 02:47:27.000000000 +0900
> +++ ./003_mariadb-10.2.0-vp/sql/sql_base.cc    2016-05-05
> 21:25:04.319731713 +0900
> @@ -1475,6 +1475,10 @@ unique_table(THD *thd, TABLE_LIST *table
>
>    table= table->find_table_for_update();
>
> -  if (table->table && table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM)
> -  {
> -    TABLE_LIST *child;
> +  if (table->table &&
> +    (
> +      table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM ||
> +      (table->table->file->ha_table_flags() & HA_CAN_MULTISTEP_MERGE)
> +    )
> +  )
> +  continue
>
> I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which
> allwoed me to make the above tests and the following identical ones
> much
> simpler.
>
> This is now pushed into 10.2-spider
>
> Regards,
> Monty
>
> _______________________________________________
> Mailing list: https://launchpad.net/~maria-developers
> Post to     : maria-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~maria-developers
> More help   : https://help.launchpad.net/ListHelp