On Wed, Aug 26, 2009 at 5:03 PM, Eric Day<[email protected]> wrote:
>> 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.
>
> Yeah, I can understand. It's not perfect, but it was either this or
> more function calls to provide both APIs. I didn't really want to
> get into the business of passing double pointers around, this tends
> to scare more people.
yeah i'm not a fan of the double pointer thing.
>> 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.
>
> It's not uncommon for libraries to do the memory management for
> you, and like I said, I wanted to make it easy for both types of
> users. Perhaps we should propose a shorter list of design questions
> to the mailing list to see what folks think, this would be one of them.
perhaps a better approach would be to do:
drizzle_con_st *drizzle_con_new (drizzle_return_t * ret, <params>);
drizzle_return_t drizzle_con_initialize (drizzle_con_st * con, <params>);
and drizzle_con_new would just be:
drizzle_con_st *drizzle_con_new (drizzle_return_t * ret, <params>)
{
drizzle_con_st * con = malloc (sizeof (drizzle_con_st));
// error handle
*ret = drizzle_con_initialize (con, <params>);
return con;
}
that lets people who want to manage memory do it and people who want it done for
them have it done for them and neither has to deal with the other's path.
> This gets down to naming of objects and the lifetime of those
> objects. To me a result object was something that could last much
> longer than a query object. Otherwise you have unusable fields in
> a result object until the query is sent, or unusable fields in the
> query object until a result is starting to be read. It seems best to
> separate these objects out.
>
> It may also be the case that a single query can produce more than one
> result set (it can in MySQL, we've not added this back into Drizzle
> yet). SO having a 1-1 binding of query-result would prevent this.
the problem with the result case seems to be that it leaves you with the
situation of having to call drizzle_query_str until a non-io-wait is returned. a
result doesn't know enough to tell you whether or not the response to the query
is back yet. i don't know enough about what you're wanting to solve with the
result to offer an example solution here like the other cases, but i'm sure
there's a solution somewhere in the middle.
>> 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)
>
> I wanted to keep all _create and _destroy functions consistent
> taking only the "this". I didn't want to introduce extra parameters
> here. Also, it is possible to connect without any DB context, you can
> just set it later in the connection or possibly reference tables via
> database names as well (not sure if this is 100% supported in the
> server still, but thats the plan).
that's not the way it would be done in a clean oo situation. the constructor
would take all of the required information so that the object would be valid and
usable immediately after construction. i'm not sure of the benefit of a
constructor not taking any parameters. there are situations where you can't pass
everything in on construction, but those seem to be pretty rare.
>> 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.
>>
>> 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.
>
> Agreed, this is not the most inutitive, and like I said before, the
> biggest thing missing here is documentation. The concurrent query
> interface was something I added as a "prototype" and still needs some
> refining (broader use of query objects for example). I can get into
> more specific answer here, but the idea is that you can add this in the
> context of a drizzle object with con being optional (in which case it
> would choose a con from the pool of available cons). One of my todos
> has been to refine this and clean it up. Also, right now the concurrent
> query interface only supports fully buffered results, which I still
> need to fix. There is a lot to do in query.c, but the basics are there.
given what i know of this function, which is only the paragraph above then my
suggestion would be to replace it with something more like
DRIZZLE_API
drizzle_query_st * drizzle_query_initialize(drizzle_query_st *query,
drizzle_result_st *result,
drizzle_st *drizzle,
const char *query_string,
size_t size,
drizzle_query_options_t options,
const void *data);
DRIZZLE_API
drizzle_query_st * drizzle_query_initialize_con(drizzle_query_st *query,
drizzle_result_st *result,
drizzle_con_st *con,
const char *query_string,
size_t size,
drizzle_query_options_t options,
const void *data);
- one function is broken up in to two, one for each use-case so it's obvious
there's two different modes of operation. this also prevents you from doing
unexpected/unnecessary things like passing in both the drizzle object and
a connection... what's the behavior of the function if you did that?
documentation is great and all, but if the api is good you'll rarely need to
consult it.
- the method operates on a query so the query should be the first param (the
this/self.)
- output params should come next, this doesn't matter in C, but in c++, or any
language that supports default values, it allows you to cleanly use them while
still preserving the C call semantics and thus lets the C api doc roughly apply
for bindings. (also just nice to have this consistency across an api)
> This was a bit of a surprise because you've been the first person
> to mention these things (which is great feedback). Others who have
> used it or wrapped it have not mentioned many issues or difficulties
> getting up to speed with it.
i looked for examples of things using libdrizzle and didn't really come up with
much. i started out playing around with ruby bindings (i realize there are other
projects already going after this) with an eye for the rails use case. it's
pretty straight-forward, essentially the simple example with a non-blocking call
to drizzle_query_str. i quickly bound the objects to ruby, but ran in to no end
of life-time management stuff. (which i'll get in to in the next section.)
things like not freeing connection objects that were added to a drizzle object
or rows of a result are completely non-obvious and should be avoided. if in one
case i have to free the connections and in others i'm not supposed to you've
created more for me to think about and do in order to get things right. if i
start out with stand-alone connections and then decide i need to put them in to
a drizzle object (that i own) then i have to go thought and change all of the
code freeing connections or else double free things.
on the face it seems helpful to recursively free like that, but in reality it
creates more chances for problems. looking at the api and examples i have no
clue that's the designed behavior, what things i have to free myself and what
things will be free'd for me. i have to go in and read some documentation or
watch things blow up in order to figure that out.
these are the sorts of reasons why the "other" libraries that i deal with often
are much easier to use than libdrizzle. i create an object i free it, i don't
have to make a set of calls before an object is valid and can be used... i just
have to know/figure out a lot less to use them.
the freeing behavior especially is a problem in language bindings where you can
foresee how things will be used. you don't really have any control over when
objects are created and destroyed. example:
DrizzleResult r;
{
Drizzle drizzle;
DrizzleConnection con;
...;
r = query()
}
r.get_data();
a somewhat simplified/made-up case, but you have an object r that will be
completely invalidated if the bindings got rid of con and/or drizzle when they
went out of scope, which they'd have to do. this is going to blow up hard and
the DrizzleResult bindings are going to have no way of knowing that the object
they own has been free'd out from under them.
more than that the bindings would have to keep track of whether or not the con
object was created with a parent drizzle so that they'd know whether or not to
free it when the owning object goes away.
> Generally, you should not be touching
> DRIZZLE_ALLOCATED/DRIZZLE_AUTO_ALLOCATED. They are available for
> advanced use cases, but most folks don't need them.
yeah didn't think so, but was just trying to see if they solved the problem i
was seeing. exposing the flag that records whether or not drizzle_create
malloc'd the memory (DRIZZLE_ALLOCATED) doesn't seem like a good idea, what
use-case require it?
> If you are getting access violations, it is probably because
> of proper allocation order like you mention, or possibly double
> freeing of objects. For example, if you free a result object that
> had buffered rows in it, all those rows are automatically freed as
> well, you don't need to do so manually. Same with connection objects
> added to a drizzle object, you can free a con object before you free
> the drizzle object, but never the other way around (drizzle_destroy
> will free all connections added to it). I always check for valgrind
> issues before releases and are not seeing many. For example, run
> your queries through examples/client with valgrind, it should be free
> (if not please let me know).
my best guess is that something is holding and then trying to work with stack
objects that are no longer in scope. going to dig in to this one more. basically
if i call drizzle_free(&drizzle) i get a seg-fault down in drizzle_result_free,
i am making no calls to drizzle_result_free so it can't be a double free.
>> 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.
>
> drizzle_con_query will block, as will all potentially blocking
> functions would, without NON_BLOCKING. Not sure what you mean here.
then i'm totally confused, how do i have a clue what is and isn't blocking? what
case would i ever want drizzle_query_str to block in? i'd much rather it
(non-blocking) return a query object that when i'm ready for the results will
block if need be. that's the 95% use case of libdrizzle, effectively simple.c is
what most clients should look like, but i would like to see the default behavior
start the query non-blocking and block at the point at which
drizzle_result_buffer is called. (which is also why i think drizzle_query_str
should be returning a query object that could cleanly be blocked on.)
> Yes, memset usually has the same effect, but if your objects contain
> fields or buffers that do not need to be zeroed out, it is less
> efficient. For example, take a connection object that has a 8k buffer
> in it. It is more efficient to set the individual fields than to memset
> the entire thing due to the extra memory writes of the buffer. Also,
> when you memset the entire buffer, you loose the possibility of
> valgrind finding invalid read issues for you.
ldrizzle-proto objects don't contain buffers like that, if they did i wouldn't
of been using memset on them.
> Same here! I'm very interested in working out these details to have a
> simple yet powerful interface (as best we can, the two do not always
> go hand-in-hand). I did a majority of this work on my own and only a
> couple people have really read through and critiqued it, so I'm very
> grateful for this type of feedback. I'm going to be digging back into
> this type of design code in libdrizzle soon, and we're going to have
> one more big backwards incompatible release soon with the new protocol,
> so now is the time to refine the API.
cool.
anyway, take a look at main.c (and the rest of the code from there) i think it's
a pretty good example of what 95% of the use-cases would look like. everything
i'm espousing is exemplified there. it's non-blocking and 'optimally' buffered
without the client even having to know what those things mean, e.g. they get the
optimal behavior by default.
it doesn't cover the full-async poll based stuff, but as i said in previous
emails i think that can cleanly be added in to what i've sketched out. so far
(as best as i can tell) we haven't discussed any use-cases that can't be
supported by ldrizzle-proto's api (the impl is another story) and it's very
small and concise. i by no means claim it's perfect and i'm sure if we were to
sit down and work through it we'd find other things that it doesn't address, but
i feel that it's straightforward interface that could be picked up by anyone and
is effectively self-documenting and reasonably mistake-proof.
--
-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
