maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi Sergei,

On Sat, Apr 10, 2021 at 11:12 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> Hi, Aleksey!
>
> On Apr 09, Aleksey Midenkov wrote:
> > > >
> > > > --echo # Increment from 3 to 5
> > > > --echo # Increment from 3 to 6, manual names, LOCK TABLES
> > > > --echo # Multiple increments in single command
> > > >
> > > > Besides "increment" is correct because PARTITIONS number is incremented.
> > >
> > > Sure.
> > > "Increment the number of partitions", this is fine.
> > > "Auto create partitions" is also fine.
> > > "Increment partitions" is meaningless.
> >
> > It is obvious from the context that we are talking about the number,
> > not partitions themselves. Treat "partitions" as PARTITIONS keyword
> > and the increment is attached to a number right next to it. That's
> > quite a sense, isn't it?
>
> No, I think it's a meaningless combination of words.
> But let's ask native speakers, shall we?

The ability to imply things and make special terms is the freedom of
any language I suppose. Slang (or "special language") helps us to
describe technical entities more concisely.

>
> > > > > > +--echo # Here t2 is incremented too, but not updated
> > > > > > +set timestamp= unix_timestamp('2000-01-01 03:00:00');
> > > > > > +update t1, t2 set t1.x= 0 where t1.x< t2.y;
> > > > > > +--replace_result $default_engine DEFAULT_ENGINE
> > > > > > +show create table t1;
> > > > > > +# Multiupdate_prelocking_strategy::handle_end() is processed after table open.
> > > > > > +# For PS it is possible to skip unneeded auto-creation because the above happens at
> > > > > > +# prepare stage and auto-creation is done at execute stage.
> > > > > > +--replace_result $default_engine DEFAULT_ENGINE 'PARTITIONS 4' 'PARTITIONS ok' 'PARTITIONS 5' 'PARTITIONS ok'
> > > > >
> > > > > eh... I don't think this is really "ok".
> > > > > As far as I remember, Multiupdate_prelocking_strategy knows what
> > > > > tables should be opened for writing and what for reading. Why
> > > > > would a new partition be created for t2?
> > > >
> > > > It knows this after tables are opened. Look at handle_end(),
> > > > specifically mysql_handle_derived(), handle_derived(),
> > > > setup_fields_with_no_wrap() and check_fields(). I believe all these
> > > > calls are required to get proper get_table_map().
> > > >
> > > > To get this working properly there must be 2-staged open tables,
> > > > something like PS does.
> > >
> > > You mean, a new partition is created for t2 in normal mode and not
> > > created in --ps?
> >
> > Yes.
> >
> > > What if you'll also add `&& table_list->updating` to your condition in
> > > TABLE::vers_need_hist_part() ?
> >
> > If `Multiupdate_prelocking_strategy::handle_end()` was not yet
> > executed how `updating` can help? No, it doesn't help.
>
> Okay. I think I know how to fix it, but let's do it after the main thing
> is pushed.
>
> Users are rather sensitive to cases when a non-updatable table in
> multi-update gets treated like updatable, we've had a bunch of bug
> reports about it over the years. Wrong privileges (UPDATE privilege
> should not be checked), wrong locks (the table should never be
> write-locked, a concurrent write lock should not block multi-update),
> triggers (should not be opened, so any problems with triggers, like
> privileges or missing tables, should not affect multi-update), and so on.
>
> So I suppose we'll have to fix this one too. This is how it could be
> done - vers_need_hist_part should check for table_list->updating. It's
> always FALSE in multi-update, so it won't be auto-creating new
> partitions at all for multi-update. But! after handle_end() (or, better,
> in handle_end()) we'll have another pass over tables and will implement
> a fallback-and-retry from handle_end().
>
> Let's do it as a separate commit after the feature is pushed.
>
> > > > > > @@ -2032,6 +2083,20 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx)
> > > > > >      tc_add_table(thd, table);
> > > > > >    }
> > > > > >
> > > > > > +  if (!ot_ctx->vers_create_count &&
> > > > >
> > > > > what does this condition mean? When is ot_ctx->vers_create_count > 0 ?
> > > >
> > > > When we already requested backoff action.
>
> I'd suggest to rewrite as
>
>    if (ot_ctx->vers_create_count)
>      /*
>        already tried to add a partition to this table and failed
>        (because of e.g. lock conflict). Don't try again
>      */
>    else if (table->vers_need_hist_part(thd, table_list))
>     ... and here your code ...
>

The comment is wrong. Added correct comment.

> > -void partition_info::vers_set_hist_part(THD *thd)
> > +uint partition_info::vers_set_hist_part(THD *thd, bool auto_hist)
> >  {
> > +  DBUG_ASSERT(!thd->lex->last_table() ||
> > +              !thd->lex->last_table()->vers_conditions.delete_history);
>
> is that right?
> Conceptually you need to test vers_conditions.delete_history for the table that
> owns this partition_info. Is it always last_table() ?
>
> I'd say it'd be more logical to do, like
>
>     part_field_array[0]->table->pos_in_table_list
>

Sorry, I had to revert that back. The original form was selected not
by coincidence. This data in TABLE is not yet initialized.

I also have to revert DBUG_ASSERT() you suggested which fails
drop_table_force on winx64-debug and add error code condition instead:

@@ -3268,12 +3271,12 @@ Open_table_context::recover_from_failed_open()
    case OT_DISCOVER:
    case OT_REPAIR:
    case OT_ADD_HISTORY_PARTITION:
-      DBUG_ASSERT(!m_thd->get_stmt_da()->is_set());
      result= lock_table_names(m_thd, m_thd->lex->create_info, m_failed_table,
                               NULL, get_timeout(), 0);
      if (result)
      {
-        if (m_action == OT_ADD_HISTORY_PARTITION)
+        if (m_action == OT_ADD_HISTORY_PARTITION &&
+            m_thd->get_stmt_da()->sql_errno() == ER_LOCK_WAIT_TIMEOUT)
        {
          // MDEV-23642 Locking timeout caused by auto-creation
affects original DML
          m_thd->clear_error();

> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx



-- 
All the best,

Aleksey Midenkov
@midenok