maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey!

On Apr 12, Aleksey Midenkov wrote:
> On Mon, Apr 12, 2021 at 3:14 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> >
> > 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.

I perceived it simply as incorrect, broken English. So I asked Ian's
opinion. Also I believe we should avoid using different terminology
internally and externally unless there's a good reason to do it.
That's all, not a question of a taste.

> > > > > 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!

I meant that you do thd->clear_error(). It's non-discriminative, it
clears all errors, no matter what was the error and where it was
created.

This is not always correct - you, probably, remember a bug with virtual
columns when a vcol expression was parsed in the error handling branch
and clear_error() removed the original error.

Generally, one should avoid thd->clear_error() and push an error handler
into the thd.

But my hypothesis was that what you're doing here is safe, because there
cannot be any error at this point. If my hypothesis is wrong, then you
cannot use thd->clear_error(). Or, perhaps, my hypothesis is correct and
there should be no error here, meaning some other code is wrong.

I asked to be able to differentiate between these two possibilities. If
my hypothesis is wrong, you cannot use thd->clear_error(). If my
hypothesis is correct, some other code needs fixing.

> 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;

This is, I suppose, archive discovery. And it's not Win64-specific.

Ok, I see now that my hypothesis was indeed wrong, and this method
indeed expects an error and has a thd->clear_error() on pretty much
every code path. It'd be easier to read if it'd had one single
thd->clear_error() at the beginning, but it's beyond the scope of
MDEV-17554.

Sorry for confusion, this change of yours is fine.

I believe that was my last comment on these commits, please tell me when
the new branch is ready. This converges rather quickly now, good!

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