You are viewing kostja_osipov

Fish Magic - How on earth is it possible to accept this?

> Recent Entries
> Archive
> Friends
> Profile
> My photos at flickr

May 2nd, 2010


Previous Entry Add to Memories Share Next Entry
03:55 am - How on earth is it possible to accept this?
Somebody should teach me how to accept community contributions.
On Friday I looked at the second version of a community contributed patch. I.e. it was since then re-written. It's Bug#20837, and it again is an infrastructure issue that is identical in all our forks.

While looking at the patch, I encountered 7 issues, which I had to report:
Bug#53340, Bug#53341, Bug#53342, Bug#53343, Bug#53344, Bug#53345, Bug#53346.

(I won't go into the details how the bad piece of code got into 5.0 in the first place, people who were in charge back in 2004 are now all gone.)

My experience suggests that quality is an issue with almost every piece of code that's thrown at us (MariaDB team contributions are an exception, since they are made by ex-MySQLers).
Should a semi-working, semi-documented code be accepted, expecting that there will be more patches?
In that environment it would be impossible to plan for stable versions (if you mean it).

Perhaps the reason for low contributed code quality is that MySQL ecosystem is mostly made by DBAs, not C/C++ engineers. Perhaps through more documentation, standardization, re-engineering we can get better results.

On the other hand, MySQL has a fairly large community of engines. Indeed, we have more than a dozen of fairly significant storage engines. There were no contributions from this group of any significance.

(7 comments | Leave a comment)

Comments:


From:jimw
Date:May 2nd, 2010 05:53 am (UTC)

that's funny

(Link)
i vaguely remember dealing with 'SET TRANSACTION LEVEL' and friends a long, long time ago. actually, it looks like bug #53340 comes directly from my fix for bug #7955 (which also, amusingly enough, implemented it's test case using the bdb engine - i guess we've lost the test for that behavior).
[User Picture]
From:kostja_osipov
Date:May 2nd, 2010 09:52 pm (UTC)

Re: that's funny

(Link)
I updated the report, it's the one about using SET TRANSACTION in the middle of a running transaction.
From:muzazzi
Date:May 2nd, 2010 07:54 am (UTC)

Quality

(Link)
Is an issue with almost every piece of code in MySQL period. How can you expect good contributions when you have spaghetti code to start with?
[User Picture]
From:kostja_osipov
Date:May 2nd, 2010 08:52 am (UTC)

Re: Quality

(Link)
With all due respect, have you actually read our code?
There are lots of claims that MySQL has spaghetti code, but our code quality varies significantly.

This is a part that has been notoriously hard to understand, but if you look at the changes in it between 5.0 and 5.5 you'll be surprised, I think.

I agree that's one of the keys to contributing.
From:markcallaghan
Date:May 3rd, 2010 04:14 am (UTC)

Re: Quality

(Link)
I have read the code. Things are getting better over time. There are several metrics for quality. The code is extremely stable when running in production as long as you skip a few features. But I don't think quality is high from the perspective of code maintenance.

I just looked at the code for handler.cc from 5.5 and there are still lousy things in there. Check out http://bazaar.launchpad.net/~mysql/mysql-server/mysql-trunk/annotate/head%3A/sql/handler.cc. MySQL has a pattern of using integer values directly rather than an enum and that is done in handler.cc -- check out ha_commit_trans where 0 == OK, 1 == rollback, 2 == error during commit, eventually 3 will mean something. How does this get past a review? Please, use an enum.

Even better, there are no comments for ha_commit_trans in handler.h. Most things don't have comments in header files, even in 5.5. Try to figure out the interface for dynamic arrays via my_sys.h -- http://bazaar.launchpad.net/~mysql/mysql-server/mysql-trunk/annotate/head%3A/include/my_sys.h -- grep for my_init_dynamic_array. Of course people in the community will do the wrong thing. I don't blame them.

Try to figure out the interface for hash tables via the header file -- http://bazaar.launchpad.net/~mysql/mysql-server/mysql-trunk/annotate/head%3A/include/hash.h, and then try to figure out what the code actually does -- http://bazaar.launchpad.net/~mysql/mysql-server/mysql-trunk/annotate/head%3A/mysys/hash.c

I am surprised that anyone writes code for MySQL without getting paid and even when they get paid I expect many of them to find something else to work on. This problem isn't isolated to MySQL. Many closed-development projects look the same.
[User Picture]
From:kostja_osipov
Date:May 3rd, 2010 10:25 am (UTC)

Re: Quality

(Link)
Mark,
bad code should be rewritten, not documented.

In every comment (or complaint) on your list, I can pinpoint the author and the time of writing. And, as we discussed before, MySQL coding standards got more strict only gradually. Back in 2001 or 2002 they were very, very lax, so all the code from this epoch does not satisfy even the basic criteria.
Besides, dynamic array and hash is an example of orphaned stuff. Will I hurry up documenting these after this blog post? Nope, not my area, and I personally believe we need a completely different container API.

This is not just my view -- as far as I can see, in the server development group few people think that the containers or mysys coding issues are the worst or most important. Lack of modularity has been on the first place so far. And it's what got most attention recently.

My experience in coaching of new server engineers also suggests it's not the most serious issue. Yes, when people find it, they curse, they waste a few hours debugging and asking questions. But soon the problem is overcome and no one bothers to write a patch. Speaking of the patch: I'm looking forward to a patch that provides a template C++ interface to mysys HASH and DYNAMIC ARRAY structures.
No patch in 7 years I've been with the project!

Take the bug in question. The solution does not involve any of mysys. But to write a correct patch, one needs to know semantics of @@session.completion_type, COMMIT AND CHAIN, XA, MySQL two-phase commit algorithm, the notion of MySQL "statement" commit, understand the difference between a transaction started excplicitly, with BEGIN or COMMIT AND CHAIN, or implicitly, with a statement that uses transactional tables, between auto-commit and implicit commit, server flags that indicate one or another.

From:jorela
Date:May 3rd, 2010 11:24 am (UTC)

Re: Quality

(Link)
FYI: we(magnus) added a c++-wrapper on top of mysys HASH in 7.0, storage/ndb/include/HashMap.hpp

> Go to Top
LiveJournal.com