On Sat, Mar 14, 2009 at 4:41 PM, Jay Pipes <[email protected]> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Padraig O'Sullivan wrote: >> Based on advice from Stewart, Jay and Monty on IRC, I added a new flag >> to the new storage engine API for resolving this bug. The behavior is >> still the same as what I outlined in earlier comments in this thread but >> the fix is a little different. I added a new flag to >> drizzler/handlerton.h named HTON_BIT_DATA_INDEX_DIR and only 1 storage >> engine adds this flag to its flags - MyISAM. Then, the check I have in >> drizzled/sql_table.cc is as follows: >> >> /* >> If the DATA_DIRECTORY or INDEX_DIRECTORY options are specified and the >> storage engine is not MyISAM, then we should return an error. >> */ >> if (create_info->data_file_name && >> !ha_check_storage_engine_flag(create_info->db_type, >> HTON_BIT_DATA_INDEX_DIR)) >> { >> my_error(ER_ILLEGAL_HA_CREATE_OPTION, MYF(0), >> ha_resolve_storage_engine_name(create_info->db_type), "DATA >> DIRECTORY"); >> goto err; >> } >> if (create_info->index_file_name && >> !ha_check_storage_engine_flag(create_info->db_type, >> HTON_BIT_DATA_INDEX_DIR)) >> { >> my_error(ER_ILLEGAL_HA_CREATE_OPTION, MYF(0), >> ha_resolve_storage_engine_name(create_info->db_type), "INDEX >> DIRECTORY"); >> goto err; >> } >> >> So I'm utilizing the storage engine API call >> ha_check_storage_engine_flag(). I also modified the symlink test case >> based on the new behavior after implementing this fix. Does the above >> solution look like the right way to go about fixing this? >> >> The branch with the fix is now linked to this bug so if you want to have >> a look at it feel free. If the above solution sounds reasonable, then >> I'll propose the branch for merging. >> >> -Padraig > > I like the ideas so far, but would recommend the following interface > change. Instead of ha_check_storage_engine_flag(), can we instead just > do something like this on the handlerton API: > > /** > * Returns whether or not the storage engine support a specific > * capability. Capability is represented by a hton_flag_bit. > */ > bool supports(enum hton_flag_bits capability); > > That way, the interface would simply be: > > if (! hton->supports(HTON_BIT_DATA_DIR)) {//do something..} > > Isn't that less verbose and more natural for the caller?
Yep, I agree its less verbose. The reason I used the ha_check_storage_engine_flag() call was because that was already present and it is used in a few places. I implemented what you suggested and it looks good. But now it seems like the ha_check_storage_engine_flag() method has become obsolete since we could just call hton->support() to check any of the storage engine flags? Also, I added 2 flags instead of 1 now because the ARCHIVE engine supports the DATA DIRECTORY option as Brian stated but I'm assuming it doesn't support the INDEX DIRECTORY option from what I've read about the ARCHIVE storage engine. Thus, one flag indicates whether an engine supports the DATA DIRECTORY option and the other flag indicates whether an engine supports the INDEX DIRECTORY option. -Padraig > > Cheers, > > Jay > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.9 (GNU/Linux) > Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org > > iEYEARECAAYFAkm8FoQACgkQ2upbWsB4UtGJggCeKZ3HOPJkMf+o2e/vZjNztG53 > vzYAnRXEVb2qxwXDFDAj3gkuRVbOHdSG > =01h8 > -----END PGP SIGNATURE----- > > _______________________________________________ > Mailing list: https://launchpad.net/~drizzle-discuss > Post to : [email protected] > Unsubscribe : https://launchpad.net/~drizzle-discuss > More help : https://help.launchpad.net/ListHelp > _______________________________________________ Mailing list: https://launchpad.net/~drizzle-discuss Post to : [email protected] Unsubscribe : https://launchpad.net/~drizzle-discuss More help : https://help.launchpad.net/ListHelp
