maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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?

> > > > > +--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 ...

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