Hi, Zardosht! Thanks for your feedback! I'm looking at the storage engine API from the other side, but my opinion is mostly the same as yours. Current API works, but it could be simplified/clarified in few places. There are part of it that I needed to explain many times. And I was saying, like, "it's historically so, just accept the way it is" or something along these lines. In particular, external_lock/start_stmt - that's, perhaps, the worst part in this regard. There's unnecessary boilerplate code - lines that every engine has to have, and they just copy it from each other. Even if minor, like incrementing status variables, setting auto-increment and timestamp columns, table->status, SHARE's. This is getting fixed though. I don't like the fact that some methods assume that char *buf is table->record[0]. I'd rather fix it (remove either the assumption or the method's argument). It never occurred to me that one can be confuse TABLE::fields and TABLE_SHARE::fields, but now I see that you're right. TABLE_SHARE should have kind of a Field_share object, while TABLE - Field_instance. Currently Field object inclused both, so the shared part is unnecessary duplicated and the "instance" part is unused in the TABLE_SHARE::fields. Come to think of it, almost everything I had to explain about storage engine API needs fixing :) Good parts of the API just work, there's no need to explain them. So, thanks for asking - it highlights problems! Besides, we're trying to add more features too. Like engine-specific attributes in the CREATE TABLE or the new table discovery API. Regards, Sergei On Aug 16, Zardosht Kasheff wrote:
I've worked on the TokuDB storage engine for quite a while now. I have had many experiences over the years, so I guess it's hard to know where to begin. I guess I will start small, and if the conversation evolves, I can contribute more thoughts. I think the current API is really good, as evidenced by the fact that many storage engines have used it to plug into MySQL. The two areas that I see we can really benefit from are the following: - documentation of the API, discussing clearly what the contracts for the functions are. There are still several functions I am not fully clear on, but am glad that they "just work". This includes documentation on the locking mechanisms. For instance, clear documentation on what external_lock, store_lock, and other related functions do would be great. - general API cleanup. In too many places, the functions just assume things, such as the buffer passed into write_row is the same as table->record[0]. In other instances, the well defined way to answer certain questions is not obvious. For instance: - If I want information on the fields, do I use the table object or table_share? - There seem to be several ways to determine if a field is NULL - There is redundant information in the KEY* structure and the fields that make up the KEY. I recall when working on MySQL 5.1, this information was inconsistent and it led to bugs, such as MySQL bug 37292 - I am sure there are plenty of other instances of this that I can find. - Transactions. The API seems to not be designed with transactions taken into account. Transactions are started in some cases of external_lock, or in start_stmt. Transactions are committed either in the handlerton's commit function or in some cases in external_lock. A clear API that takes transactions into consideration would be quite helpful.
Those are my early thoughts. I am happy to share more if people are interested.