oqgraph-dev team mailing list archive

[Arjen Lentz <arjen@xxxxxxxxxxxxx>]

Hi Andrew

> If the bug is in the new code in passing in a bogus data structure, I
> guess I was thinking, that table.cc seems to be in the core of MariaDB,
> and I was a little surprised something central could be crashed by a
> broken plugin.
> 
> Anyway, would a patch to table.cc should go into the main code base in
> which case, how would I go about that? Or should I just fix in my tree
> now and it would get merged in the future with oqgraph?

There are a few ways to go about this, but it shouldn't be mixed with oqgraph patches/merges.
So, see this as a separate bug, as it could be triggered by other plugins also (like you said).

You could file a bug on MariaDB's Jira bug tracker, and include a patch segment there or refer to a pushed branch that you have on Launchpad that purely fixes that particular issue - you can even set a "merge request" on that branch to its trunk in mainline MariaDB.

I think that's the most proactive approach: "I identified a problem, and here is my fix". Even if the fix is not liked, you've done your homework and made an effort. In that case another approach is likely to get committed. Otherwise, possibly your fix. So all good.


> > Crashing a multi-threaded/multi-user daemon is not nice. So
> > generally when something fails in MySQL, you have a mechanism to
> > return a fail from that function.
> > At the very least you'd have an assert in there (do add it), but
> > obviously you'd want to code to not get into that situation in the
> > first place
> 
> Agreed on both :-)
> 
> init_tmp_table_share() is void however, so I am not sure what the
> usual pattern is for MySQL/mariadb core code in these cases...

Could be blunt and put an assert() in there, as obviously continuing with a NULL ptr will result in a segfault anyway. Better to know and see it identified clearly even if it's not neatly handled.

InnoDB has some places like that in its code.


> This function would appear to be part of the mysql API and perhaps
> should be
> changed to return an error code if something unexpected happens - but
> who/how
> decides these things? For example, should it also check for key==NULL,
> although this condition wont crash that function?

Practicality should decide, and sometimes time.

Changing from void to int is relatively harmless, although a compiler will chuck warnings to calls that don't void or use the return then, right? Well that's ok too. You can suggest it.

I recommend you get connected to the mariadb dev list - in your comment field, say you're "working on oqgraph with Antony and Arjen and through there have also become involved with some of the MariaDB code code and wish to further contribute" or something like that. I think that's excellent!


> Another "odd" thing is this is C++ code with a bunch of classes,
> coexisting up with a bunch of plain root namespace functions; and
> table.cc is 6900 lines long... would it make sense to split this
> cc/h into multiple files one major class per each, etc?

We jokingly refer to the MySQL/MariaDB source code as written in C+.
Code cleanups are awesome, but also very delicate issues even just from a technical perspective - initially you'd have to make absolutely sure that there are no functional changes, even stuff that seems harmless.

Also there's still the consideration with merges from Oracle upstream, although that might become less of an issue.

For this, I'd say awesome you're interested and definitely something to look in to further, but right now focus on the oqgraph codebase as a) we have full control over it, b) there's a time factor there (we have to hurry up to get it into MariaDB 10.0) and c) your activity will show up, so it provides practical credit when working on core MariaDB later. All beneficial.

Cheers,
Arjen.
-- 
Arjen Lentz, Exec.Director @ Open Query (http://openquery.com)
Australian peace of mind for your MySQL/MariaDB infrastructure.

Follow us at http://openquery.com/blog/ & http://twitter.com/openquery