maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi, Sergei!

On Mon, Apr 12, 2021 at 8:46 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> 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 believe you are right in your motivation. But the emphasis in the
test is on increment: we are checking the number in PARTITIONS clause.
I removed all the comments regarding "increment" from the test.

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

Yes, I should remember there are no other debug buildbot nodes for a
developer. Though there should be.

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

The new branch is ready.

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


-- 
All the best,

Aleksey Midenkov
@midenok