maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi Sergei,

On Mon, Apr 12, 2021 at 3:14 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> Hi, Aleksey!
>
> On Apr 11, Aleksey Midenkov wrote:
> >
> > It was a conscious choice. Quantitative characteristic is implied.
>
> This isn't going anywhere.

Of course. And because of that I prefer to write shorter comments.

> I've asked Ian (KB technical writer) for his opinion and we'll do what
> he'll say.

So he says it is not meaningless. I am and was upset by your
disagreement with my taste. If you are totally against the form I use
I'm going to remove the comments. But when we are talking about public
documents of course I'm going to make it better understandable, and he
is right regarding that particular place.

>
> > > True, I know that my feeling of the language is not perfect, I'm not a
> > > native speaker. But it works both ways, you're not a native speaker
> > > either.
> > >
> > > So, if you don't want to change to correct English, there's no point in
> > > arguing, let's just ask a native speaker.
> > >
> > > > > 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.
> > >
> > > The comment describes what I've seen in gdb, looking how
> > > ot_ctx->vers_create_count could be non-zero. It was your
> > >
> > >   update t1 set x= x + 122 where x = 1
> > >
> > > test case, "Locking timeout caused by auto-creation affects original DML"
> > > You try to auto-create, it fails because of the lock timeout, you
> > > clear_error(), the table is reopened. But because vers_create_count is
> > > not reset, it does not try to auto-create again, thus avoiding an
> > > infinite loop.
> > >
> > > What do you thing was wrong there?
> >
> > Okay, it is partly true. Failing case is the secondary one.
>
> May be, but it's the only case where ot_ctx->vers_create_count>0 there.
> So if there's a "primary" case too, your tests never trigger it.
> Add a test for it, please.

Sorry, my bad, vers_create_count was not passed to open_ltable(),
there is a different ot_ctx. Added your comment as is.

>
> > > > 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();
> > >
> > > What happens on winx64-debug?
> >
> > Please look yourself at
> >
> > http://buildbot.askmonty.org/buildbot/builders/winx64-debug/builds/24345/steps/test/logs/stdio
>
> Thanks. But either I don't know how to read it correctly or it doesn't
> have enough info.
>
> Could you explain what happens on this builder? What error do you see in
> stmt_da in recover_from_failed_open() that triggers the assert?

Why are you asking me to do investigations on your hypotheses out of
the task scope? It was just a hypothesis after all! Though you can
figure out error code (ER_NO_SUCH_TABLE) from the test case:

create table t2(a int not null) engine=archive;
flush tables;
--error 1
--remove_file $DATADIR/test/t2.frm
select * from t2;
flush tables;
--remove_file $DATADIR/test/t2.ARZ
--error ER_NO_SUCH_TABLE
select * from t2;

There are several tests failing though...

>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx
--
All the best,

Aleksey Midenkov
@midenok