On Tue, Aug 25, 2009 at 11:55 PM, Eric Day<[email protected]> wrote:
> Hi Ross!
> Thanks! Just to clarify, lp:libdrizzle is the new libdrizzle, and was
> written from scratch starting last fall. The "oldlibdrizzle" we refer
> to is actually the old library based on libmysql that is only inside of
> a plugin inside of drizzled for the server side protocol (lp:drizzle,
> plugin/oldlibdrizzle). The old libdrizzle plugin is being replaced with
> a plugin that will use lp:libdrizzle for all server-side communication.
k, so there was definitely some confusion there on my part. the main thing that
lead me to believe that libdrizzle was only temporary was that everything goes
across the wire as char *, which is less than optimal for numeric types and that
i'd heard about a new protocol, which i guess i assumed was part of a
(new)libdrizzle.
> So, lp:libdrizzle (the new libdrizzle) is what we've been building
> all the new tools and APIs on, such as the php-ext, python, and others.
k.
> You only need the query string around until it has been sent to the
> client (for obvious reasons). Once it is sent, you can free or reuse
> it. I do have a query object in there already, it's just only used for
> the concurrent query interface right now. I'm planning to re-factor
> things a bit internally to always use it and make it optional even
> for single queries (you can now if you really want to).
that, along with a couple associated changes would be a big step forward in my
mind.
> ...
> This is always an option, but the interface supports both. For
> applications that want to control all memory they can do so easily by
> passing valid pointers in. Having an interface that will automatically
> malloc things for you can be very useful though. We decided providing
> both would be the most robust (and this is fairly consistent for
> all objects).
this one can definitely be chocked up as a difference of opinion (that i don't
feel _that_ strongly about.) mainly i don't like the inconsistency of half of
the time returning ret-codes and the other half pointers and taking the
ret-codes as pass by pointer params.
having the library do the malloc for you doesn't really seem to buy much, at
least what it does buy you (removes a couple lines, malloc and error check) i
don't think is worth the loss of consistency in the api.
>> - naming needs quite a bit of cleanup, things should be made clean
>> OO-ish (in naming, encapsulation, parameter order) even though it's C
>
> Agreed. I saw you mention a few, like drizzle_con_wait, which I agree
> should be moved to drizzle_wait since it works on a drizzle objects. I
> wouldn't say needs that much cleanup, but there are a couple functions
> bugging me too.
it's not just the things that aren't named for the object the really operate on.
there's a lot of calls/paths that just 'feel' a little funky. the results
objects in general fall in to this category. to me it really seems like they're
almost a query object, but not quite. i'm not sure in what situation you need a
result object that using a query object wouldn't be cleaner.
other situations require calling a set of functions before an 'object' is
usable or at least allow you to create situations are that are invalid/errors.
creating connections is one of those (db should be a param to con_create since
you'll always have to set_db before you connect)
all of the multiple query stuff, which i've only glanced at, seems to have a lot
of potential for mistake and just generally (to me again) feel funky.
DRIZZLE_API
drizzle_query_st *drizzle_query_add(drizzle_st *drizzle,
drizzle_query_st *query,
drizzle_con_st *con,
drizzle_result_st *result,
const char *query_string, size_t size,
drizzle_query_options_t options,
const void *data);
looking at that i have no clue what this function does. it takes a lot of things
and returns a query object. is that object a single query? is it a set of
queries/what's the query added to? there's both a drizzle object and a con
object going in, what if a drizzle object that isn't associated with the con
object is passed? ... why is the drizzle object the first parameter, seems like
if this is a query function and i'm passing in a query object it should be the
first param, essentially self/this.
to me, multiple queries on a single connection would look something like:
drizzle_connection_initialize (&con)l
drizzle_query_initialize (&query1, &con, "select...");
drizzle_query_initialize (&query2, &con, "select...");
drizzle_query_initialize (&query3, &con, "select...");
// time passes
drizzle_column_initialize (&column, &query2);
// at which point we wait on query2 to return us data.
these aren't un-grok-able problems or anything, i'm sure if you explained it to
me i would be able to make sense of and use it all.
my biggest complaint about libdrizzle is that none of it has made sense to me
looking for the first time/coming to it clean. i've had to spend a lot of time
in trial-and-error and/or asking questions to the people who built it.
this is what i do for a living and i'm pretty good at it. in the past couple
months i've come (in a similar fresh manner) to sqlite3, libcurl, and libexpat
none of which gave me any problems.
i don't think libdrizzle is wrong/broken, but do feel like it's
non-obvious/sub-optimal/sometimes just doesn't feel right. i can't sit down and
list all of the things i think can be improved and frankly wouldn't have the
energy to spell-out/fight for all of them. i'd love to work towards addressing
them, and it is with that in mind that i started working on ldrizzle-proto. that
said others may not see problems and/or it may be too late to make those kinds
of changes to the libdrizzle api and if that's the case i'll let it go.
>> - everything should have a initialize and destroy that should be
>> called even if it's effectively a no-op.
>
> Agreed, and we have those for every object type. What do you see
> is missing?
there aren't any that are missing, that i know of, but calling them causes
problems. if you run test in ldrizzle-proto as-is with valgrind
--leak-check=full --show-reachable=yes --leak-resolution=high you'll see 1
access error and a couple things being leaked.
i'm calling my destroy for everything i call init on, but the destroy methods
are all no-ops. whenever i try to use the libdrizzle free/destroy methods i get
loads of access violations and leaks usually ending with seg-faults. i spent a
decent bit of time trying to track them down and made no real progress. the only
guess i had was that top level objects had references to lower level things
(drizzle has refs to connection, connection to results/rows/columns.)
for example if you uncomment the drizzle_free call at the end of drizzle.c
(in my drizzle_destroy(Drizzle * drizzle) call) you'll get segfaults that
seem to come from drizzle_free calling drizzle_con_free, which calls
drizzle_result_free which blows up b/c all of my objects are stack
allocated. i thought the DRIZZLE_ALLOCATED/DRIZZLE_AUTO_ALLOCATED options might
have something to do with this, but setting/unsetting them didn't seem to change
anything.
> So, blocking vs non-blocking has nothing to do with buffered
> vs unbuffered. You can use any combo of the above, one does not
> determine the other. The different APIs are really about buffered
> and non-buffered, and each has different semantics. Trying to shove
> this behind a single API will lead to functions that just don't make
> sense in a certain context. Separating the two out seemed like the
> most logical thing to do (they really are two APIs). I played with
> this initially and the API was not intuitive.
definitely two separate issues, i'm not confusing them, but perhaps
misunderstood exactly what NON_BLOCKING entailed
> As far as non-blocking goes, this really is determined by how the
> application is driven (event vs procedural). Also, it seems the
> term 'non-blocking' means different things to different people. In
> libdrizzle, non-blocking is used to set all socket communication into
> non-blocking mode so a read() or write() *never* blocks, and this is
> trickled up to the application using the IO_WAIT return code. It is up
> to the calling application to notify libdrizzle when sockets are ready
> using either the con_wait() function or providing a set of callbacks
> to trigger events on the file descriptors (using libevent for example).
so this seems to be the biggest problem/misunderstanding i've had. if
drizzle_con_query isn't blocking (even without NON_BLOCKING) then that gets
the behavior i'd imagine most cases want/need.
> If you want to be able to send a query and not immediately block
> waiting for the result, you can do this without using non-blocking
> I/O. You simply send the query and tell it not to wait for results
> immediately, the application can do something else, and then at some
> point come back and block on results. This is actually all blocking
> I/O from the caller perspective, it's just a matter of when to call
> those blocking functions.
>
> The biggest thing missing right now is documentation and examples of
> how to do just this and other common uses, and possibly making this
> more apparent in the API.
definitely true, but i think the api itself (in the details) causes/caused a
lot of the confusion.
>> - all data values are transmitted as strings (not sure why this is, maybe
>> there's a reason for it)
>
> This is due to the current protocol, all data is sent in string form
> rather than native types. In the new protocol there will be an option
> to get string or native returns to avoid the overhead you mention.
cool. i'd assumed as much. i think part of my confusion about
oldlibdrizzle/newlibdrizzle might of stemmed from hearing talk about the new
protocol and assuming that was part of a new client library.
>> memset() in init functions and assert(object) in all functions.
>
> It is actually inefficient and problematic with valgrind to use
> memset() in init functions. You end up setting memory you really don't
> need to clear, and when doing this, valgrind looses it's ability to
> check for reads of uninitialized memory. It is actually best practice
> now to NOT be memset()ing memory unless there are no other options.
the memset isn't necessary, just was easier than listing out/initializing all of
the fields individually. if the desired initial state is NULLs for all pointers
and 0's for ints etc. then memset effectively is the same as listing out all of
the structs fields and setting them to NULL/0/etc. which shouldn't confuse
valgrind. i never heard anything about memset being less effecient than setting
each of the fields individually. i would of guessed that memset would be a
highly optimzied operation. regardless wasn't an important point...
> As far as the assert() checks go, they only are good for NULLs, and
> never for bad pointers or uninitialized memory. If you are programming
> mainly with preallocated objects (which is most common), this is
> generally a useless check that just adds a few extra instructions to
> each call. Problems, whether NULL, bad pointers, or bad objects, will
> be fairly obvious (and will cause a crash the same time an assert()
> would get hit).
assert only happens in code compiled without NDEBUG, which admittedly is a
problem in shared library code. assert has its uses, but this probably isn't one
of them.
>> drizzle_select_db valgrind warnings
>
> Yeah, this is a bug and I need to look into it further. Patches
> welcome! :)
i'll take a look next time i'm sitting in front of the code.
> Thanks for all the valuable feedback, libdrizzle still has a bit to go
> and this will certainly help in refining the internals and interface!
you're welcome.
thanks for taking the time to look my stuff over and respond. i look forward to
continuing the discussion.
best,
--
-rm
_______________________________________________
Mailing list: https://launchpad.net/~drizzle-discuss
Post to : [email protected]
Unsubscribe : https://launchpad.net/~drizzle-discuss
More help : https://help.launchpad.net/ListHelp
