You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Alex Dubov <oa...@yahoo.com> on 2006/05/20 07:03:03 UTC
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c - correct version
Even though some people incorrectly assumed that my
version of apr_dbd_mysql is mostly about error
reporting, the feature list is as following:
1. Works correctly with 5.0.xx
2. Supports most data types for parameters and results
(through printf style format - assumes that result
types are known in advance or set through CAST AS in
query)
3. Correctly deals with corner cases (large unsinged
bigints anybody?)
4. Support for binary types and blobs (returns pointer
to length/data struct)
5. Error codes that mean stuff (as of now, and not in
some remote future)
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c -
correct version
Posted by Bojan Smojver <bo...@rexursive.com>.
Error reporting should now be in, with some style tweaks. I also made
sure it compiles under 4.1. See r40 of the driver.
--
Bojan
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c -
correct version
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Bojan Smojver <bo...@rexursive.com>:
> I reported that the row changed.
I meant to say: "It reported..."
--
Bojan
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c -
correct version
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sun, 2006-05-21 at 23:49 -0700, Alex Dubov wrote:
> won't it be better to add "max_field_size=" key to the
> dbd parameter string (with some sane default and 0
> defined as unlimited)?
Yep, we can do that.
--
Bojan
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c - correct version
Posted by Alex Dubov <oa...@yahoo.com>.
> MAX_FIELD_SIZE_UNLIMITED?
won't it be better to add "max_field_size=" key to the
dbd parameter string (with some sane default and 0
defined as unlimited)?
As to apr_palloc: my mistake. apr_palloc does not
zeroes out memory after all. I looked in the wrong
place.
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c -
correct version
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Alex Dubov <oa...@yahoo.com>:
> What the oracle does if BEFORE UPDATE trigger prevents
> updating of matched rows? I believe that matched rows
> will only differ from "updated" ones in the presence
> of suitable trigger.
No idea. I just tried a simple update with identical data. I reported
that the row changed.
>> MAX_FIELD_SIZE_24MB
> I just thought that no such trickery is needed with
> linux. Linux (and curiously WinCE, but not Win32) use
> speculative allocation, so one can actually malloc 2GB
> of space without much problems and with only 1 page of
> wasted RAM (given a 64b addresses or proper kernel).
> Of course, this is not so portable behaviour.
Yeah, I noticed that on FC5 there are no complaints about large
allocations (i.e. my 700MB box happily allocated multiple 4 GB chunks
- OUCH!). I'd much rather see this kind of thing explicitly enabled
for security reasons. Maybe MAX_FIELD_SIZE_UNLIMITED?
> And for
> some reason (I just noticed it), apr_palloc is
> synonymous to apr_pcalloc and does memset on allocated
> region - this is very bad in light of the above. Given
> this, can we have a apr_xxalloc function that does not
> touches allocated block? This will make a great
> improvement in physical memory usage (at least on
> linux).
I'll leave this one to folks that understand portablility of memory
allocation a bit better than myself.
--
Bojan
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c - correct version
Posted by Alex Dubov <oa...@yahoo.com>.
> > flags = CLIENT_FOUND_ROWS,
What the oracle does if BEFORE UPDATE trigger prevents
updating of matched rows? I believe that matched rows
will only differ from "updated" ones in the presence
of suitable trigger.
> MAX_FIELD_SIZE_24MB
I just thought that no such trickery is needed with
linux. Linux (and curiously WinCE, but not Win32) use
speculative allocation, so one can actually malloc 2GB
of space without much problems and with only 1 page of
wasted RAM (given a 64b addresses or proper kernel).
Of course, this is not so portable behaviour. And for
some reason (I just noticed it), apr_palloc is
synonymous to apr_pcalloc and does memset on allocated
region - this is very bad in light of the above. Given
this, can we have a apr_xxalloc function that does not
touches allocated block? This will make a great
improvement in physical memory usage (at least on
linux).
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c -
correct version
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Bojan Smojver <bo...@rexursive.com>:
> flags = CLIENT_FOUND_ROWS,MAX_FIELD_SIZE_24MB
Or probably even:
flags = CLIENT_FOUND_ROWS/MAX_FIELD_SIZE_24MB
In order to avoid complications with valid keyword delimiters (which
comma belongs to).
--
Bojan
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c -
correct version
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Alex Dubov <oa...@yahoo.com>:
> However, with CLIENT_FOUND_ROWS I would prefer the
> "changed rows" behavior, not the "matched rows" one.
> May be additional parameter to connection string will
> do.
OK. MySQL seems to be the odd one out here - Oracle seems to do what
PostgreSQL/SQLite3 do. Never mind, I'll work on the flags...
> After all, it's not unreasonable to
> have a MEDIUM TEXT/BLOB (24MB) fetched in one chunk on
> modern machine. To conclude - do you have any thoughts
> on how to implement this in apr way?
Maybe we can also use the flags to _open for that? In other words, we
can say in the connect string:
flags = CLIENT_FOUND_ROWS,MAX_FIELD_SIZE_24MB
Then we parse those things (as strings, of course) and convert them to
relevant flags, options and what not. Manual, but safe.
> Mysql likes to crash if parameter/result arrays are
> not zeroed fully before bind calls. I'm using pcalloc
> for everything because of that.
Right. I noticed that in the patch.
--
Bojan
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c - correct version
Posted by Alex Dubov <oa...@yahoo.com>.
In general, my patch may remove a line or two without
a reason because I developed it using older version of
the apr_dbd_mysql, so may be I just missed something.
However, with CLIENT_FOUND_ROWS I would prefer the
"changed rows" behavior, not the "matched rows" one.
May be additional parameter to connection string will
do.
> I'm a bit worried about things like this:
>
> ------------------------------------
> (*res)->bind[i].buffer_length =
> (*res)->res->fields[i].length;
>
> [..snip..]
>
> data[i] = apr_palloc(pool, sizeof(struct blob_t) +
> (*res)->bind[i].buffer_length);
> ------------------------------------
I'm also well aware of the unbounded field length
problem. The proper thing to do is, of course, to
allocate some extent, fetch, then check for truncation
and re-allocate/report error. However, current api has
no way to do this cleanly. Frankly, I prefer to have
an out-of-memory error than silent data truncation (I
do not try to suggest that this is a way to go - only
a side comment). After all, it's not unreasonable to
have a MEDIUM TEXT/BLOB (24MB) fetched in one chunk on
modern machine. To conclude - do you have any thoughts
on how to implement this in apr way?
> OK. As long as segfaults are avoided (which used to
> be the case until
> recently with driver from the head and MySQL 5.0),
> it should be OK.
Mysql likes to crash if parameter/result arrays are
not zeroed fully before bind calls. I'm using pcalloc
for everything because of that.
With your permission, I would like to return to mysqld
timeout problem. mod_dbd always does
apr_dbd_check_conn, which, in turn, happily fails with
"server has gone away" error. This leaves two
possibilities: add some code to mod_dbd to do a
reconnect or make apr_dbd_check_conn do it. The later
is nicer, but it requires a support for "reopen" -
whether additional driver hook or argument to
xxx_open.
Luckily, mysql_real_connect can use existing
reference.
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c -
correct version
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Alex Dubov <oa...@yahoo.com>:
> Ok. Here is a patch against webthing's rev 34 (I
> assume it's a last one).
Yep, that's right.
> There's a brief changelog in
> the beginning of the file.
>
> Two warnings, however:
> 1. I only use mysql-5
I think Nick wanted to see the driver working with 4.1 as well. I have
machines with this version around - it will be trivial to see what
doesn't compile.
> Also: my patch does not uses is_null and error arrays
> (these are set to zero) - instead is_null_value and
> error_value are checked in result's BIND structure.
OK. As long as segfaults are avoided (which used to be the case until
recently with driver from the head and MySQL 5.0), it should be OK.
> I also don't have any pre-set bounds on maximum field
> length and I don't know what happens if somebody tries
> to pass LONG BLOB (the only type which can have
> unreasonable size). It's easy to work around long
> blobs using SUBSTRING sql statement with offset/size
> as parameters, but in general some sort of
> truncation/continuation interface is probably needed
> (and some interface for mysql_stmt_send_long_data, to
> send long data back to db). On the other hand, the
> need for such long data in databases is rather limited.
I'm a bit worried about things like this:
------------------------------------
(*res)->bind[i].buffer_length = (*res)->res->fields[i].length;
[..snip..]
data[i] = apr_palloc(pool, sizeof(struct blob_t) +
(*res)->bind[i].buffer_length);
------------------------------------
For LONG TEXT, length gets returned as 4294967295 (at least it does
with 5.0.21 on my FC5 box), although the actual data is much smaller
(there is only about 50 bytes in the field). I don't think we should
let the allocator come up with solutions for that one :-)
I have to inspect/test the patch in detail, as it also removes some of
the segfault fixes that were put in recently (for transactions and
prepared statement cleanups). I'll keep you posted...
I also wanted to ask about CLIENT_FOUND_ROWS flag (which your patch
removes). I used that flag in one of the recent updates to the driver
in order to bring MySQL behaviour in line with other DBD drivers. In
particular, PostgreSQL/Sqlite3 databases will report that rows have
been affected if UPDATE finds a match, regardless of whether the data
was the same or not. By default, MySQL will say that 0 rows were
affected if data was the same. When the flag is used, this brings
MySQL in line with others. Is that something that MySQL users may find
problematic? Should we have another option to _open() instead (e.g.
flags)?
--
Bojan
Re: LONGs and LOBs [Was: improved error reporting for apr_dbd_mysql.c]
Posted by Nick Kew <ni...@webthing.com>.
On Wednesday 31 May 2006 18:00, Chris Darroch wrote:
> Nick Kew wrote:
> > The Oracle driver uses uppercase; %C for CLOB and %B for BLOB,
> > %L for LOB (hey, I can't even remember which of these are working
> > and which are placeholders!). Is there any reason not to follow that
> > convention for other drivers implementing LOBs?
>
> The Oracle driver is in an interim state at the moment with respect
> to CLOBs and BLOBs. I use it in production with CLOBs, but it's not
> as efficient as it could be. I will get back to it someday!
Aha, right. I thought it had to be CLOBs we have working properly,
but just wasn't sure:-)
> While we're on the subject, I believe there's also no easy way to
> distinguish between an error return from apr_dbd_get_entry() and
> the return of a valid NULL column value. If we're changing the
> get_entry() function at some point in the future, could we add
> something like an int *null_flag argument that's set to 0 or 1?
I tried changing apr_dbd_get_entry once, but didn't follow through
on it. If we're going to support LOBs (and that seems to be a
popular direction to be going), then I'd suggest returning the
data as a bucket brigade - with the option to call get_entry
multiple times if the backend supports streamed LOBs.
something like
typedef struct {
enum { various types, error, null } type;
union { string, integer, real, bucket brigade } data;
} apr_dbd_entry_t;
> So, a quick summary:
>
> - get_entry() would need a buffer length return value to properly
> support BLOBs
> - get_entry() could also use an argument to flag NULLs vs. errors
> - get_entry() could do better with memory allocation and charset issues
> for LONGs/LOBs
>
> - p[v]query() supports %L now for use with LONGs and LOBs
> - p[v]query() could implement a lot of logic for "better" LOB support,
> in which case they'll use %C[:colname] and %B[:colname] options
> - p[v]query() will also need a buffer length argument to support BLOBs
> fully - p[v]select() could support %L, %C, and %B too, but I'm unsure if
> there's real utility in that
Indeed. The issue now is the API. get_entry wants improving, but
query/select can work as-is with the above format params, and
underlying datatypes the driver understands.
Unless someone has strong views to the contrary?
--
Nick Kew
Re: LONGs and LOBs [Was: improved error reporting for
apr_dbd_mysql.c]
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2006-06-02 at 20:12 +1000, Bojan Smojver wrote:
> length:column:table:payload
> ------ ------ ----- -------
> ^ ^ ^ ^
> | | | +-----------> binary data, length in size
> +------+------+-----------------> ascii
>
> Obviously, for most databases column and table could be omitted,
> reducing the thing to:
>
> length:::payload
If we wanted to be even more pedantic, we could call the above %B and
the following %b:
> length:payload
Just a thought...
--
Bojan
Re: LONGs and LOBs [Was: improved error reporting for
apr_dbd_mysql.c]
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2006-06-02 at 13:18 -0400, Garrett Rooney wrote:
> If it has never been present on a release branch we have zero need to
> worry about backwards compatibility. If someone is depending on the
> code in trunk clearly they have enough clue to deal with the breakages
> that can result.
Henceforth, longs shall be known as %l and %L ;-)
Recap of the proposal after the change:
- %s - varchar
- %S - text
- %c - char
- %C - unsigned char
- %h - short
- %H - unsigned short
- %d - integer
- %D - unsigned integer
- %l - long
- %L - unsigned long
- %q - quad (i.e. long long)
- %Q - unsigned quad
- %f - float
- %F - double
- %i - time
- %a - date
- %A - datetime
- %t - timestamp
- %T - timestamp with zone
- %b - blob (encoded on C side as per previous e-mail)
--
Bojan
Re: LONGs and LOBs [Was: improved error reporting for apr_dbd_mysql.c]
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 6/2/06, Chris Darroch <ch...@pearsoncmg.com> wrote:
> Bojan Smojver wrote:
>
> > length:column:table:payload
> > ------ ------ ----- -------
> > ^ ^ ^ ^
> > | | | +-----------> binary data, length in size
> > +------+------+-----------------> ascii
>
> That could work for me, but if others feel it's too much of a hack,
> fair enough.
>
> > - %O - unsigned long (big L has been taken by Oracle driver already!)
>
> A question re APR policy, really -- do we need to care about this,
> since the Oracle driver only exists in trunk, IIRC? I'm all for
> backwards compatibility, but thought I should clarify if it's required
> in this case.
If it has never been present on a release branch we have zero need to
worry about backwards compatibility. If someone is depending on the
code in trunk clearly they have enough clue to deal with the breakages
that can result.
-garrett
Re: LONGs and LOBs [Was: improved error reporting for apr_dbd_mysql.c]
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Bojan Smojver wrote:
> length:column:table:payload
> ------ ------ ----- -------
> ^ ^ ^ ^
> | | | +-----------> binary data, length in size
> +------+------+-----------------> ascii
That could work for me, but if others feel it's too much of a hack,
fair enough.
> - %O - unsigned long (big L has been taken by Oracle driver already!)
A question re APR policy, really -- do we need to care about this,
since the Oracle driver only exists in trunk, IIRC? I'm all for
backwards compatibility, but thought I should clarify if it's required
in this case.
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: LONGs and LOBs [Was: improved error reporting for
apr_dbd_mysql.c]
Posted by Bojan Smojver <bo...@rexursive.com>.
On Fri, 2006-06-02 at 07:58 +1000, Bojan Smojver wrote:
> One more note here: if we go with this approach, this is almost
> certainly something for APU 2.0 due to existing implementation
> incompatibility. For instance, PostgreSQL driver uses %d to cast to
> integer in SQL, but fetches a char* as an argument nonetheless. If we
> were to implement a change in which an int* is expected with %d,
> current code would break.
Now this got me thinking (I know, I shouldn't be doing this :-), because
waiting for APR 2.0 probably means waiting for Apache 2.4 as well...
Instead of introducing various types of pointers on C side and then
using that in p[v]query/select, maybe we can stick to const char *
instead, but encode other information into the string. For instance,
something referred to as %b (blob, binary data) could be passed into
p[v]query/select and fetched fromo apr_dbd_binary_get() as:
length:column:table:payload
------ ------ ----- -------
^ ^ ^ ^
| | | +-----------> binary data, length in size
+------+------+-----------------> ascii
Obviously, for most databases column and table could be omitted,
reducing the thing to:
length:::payload
or even:
length:payload
With this approach we can:
- keep binary compatibility
- cater for Oracle "sing and dance" routine
- keep passing strings around for everything, but most importantly the
simple stuff (strings, integers, floats etc.), as we are doing now
- get stuff back in this format from apr_dbd_binary_get()
I know it's going to a bit of hassle to "pack" and "unpack" data, but we
can have handy little functions (or even macros) for that, making the
job much easier.
In such a case, the "format identifiers" would have to be adjusted
slightly, in order to preserve current meanings. The types would mostly
have meaning on the SQL side and less on C side (i.e. everything would
be picked up as const char *). Something like this:
- %s - varchar
- %S - text
- %c - char
- %C - unsigned char
- %h - short
- %H - unsigned short
- %d - integer
- %D - unsigned integer
- %o - long
- %O - unsigned long (big L has been taken by Oracle driver already!)
- %q - quad (i.e. long long)
- %Q - unsigned quad
- %f - float
- %F - double
- %i - time
- %a - date
- %A - datetime
- %t - timestamp
- %T - timestamp with zone
- %b - blob (only this one is encoded on C side as per above!)
The above would be the only things we'd advertise in APU 1.3.x, so that
people know they can rely on this. All other letters that are currently
implemented differently (e.g. %L in Oracle driver), would be kept
silently to avoid breaking things.
Then in APR 2.0, we can do the fancy stuff with buckets, brigades and
other goodies.
<fire_proof_suit_on/> ;-)
--
Bojan
Re: LONGs and LOBs [Was: improved error reporting for
apr_dbd_mysql.c]
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Bojan Smojver <bo...@rexursive.com>:
> I'm sure there are many more combos that we can consider, but these
> should be a decent start.
One more note here: if we go with this approach, this is almost
certainly something for APU 2.0 due to existing implementation
incompatibility. For instance, PostgreSQL driver uses %d to cast to
integer in SQL, but fetches a char* as an argument nonetheless. If we
were to implement a change in which an int* is expected with %d,
current code would break.
--
Bojan
Re: LONGs and LOBs [Was: improved error reporting for apr_dbd_mysql.c]
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Alex Dubov wrote:
> int apr_dbd_entry_is_null(const apr_dbd_row_t *row, int n)
That strikes me as a good solution for the current API; thanks!
I suppose it might have to be:
int apr_dbd_entry_is_null(const apr_dbd_driver_t *driver,
const apr_dbd_row_t *row, int n);
but I haven't thought about it enough yet to be sure either way.
Bojan Smojver wrote:
> However, we can have a new function instead, so that we remain
> backward compatible. Someting like apr_dbd_entry_get(). Or, if that's
> no good because people may expect intuitively that this is the same as
> apr_dbd_get_entry(), then maybe apr_dbd_binary_get() or something like
> that.
Yeah, I'd suggest that any renaming of functions that we do
to match APR/APU (driver_get, dbname_set, row_get, entry_get, name_get?)
should not also add or change the functionality from the old versions.
Otherwise it'll just add confusion for everyone.
Bojan Smojver wrote:
> Quoting Alex Dubov <oa...@yahoo.com>:
>> from here everything works just like with
>> printf - if you push pointer incompatible to
>> specifier, it's most likely a crash.
[snip]
> I think we should stick with a single letter convention, for
> simplicity of parsing. So, how about:
>
> - %S - string (i.e. char *, text in SQL)
> - %D - unsigned integer
> - %c - char
> - %C - unsigned char
> - %h - short
> - %H - unsigned short
> - %l - long
> - %L - unsigned long
> - %q - quad (i.e. long long)
> - %Q - unsigned quad
> - %F - double
> - %b - blob (brigade, backend specific blob type 1 in SQL*)
> - %B - blob (brigade, backend specific blob type 2 in SQL*)
> - %o - blob (brigade, backend specific blob type 3 in SQL*)
> - %O - blob (brigade, backend specific blob type 4 in SQL*)
> - %i - time (string, time in SQL)
> - %a - date (string, date in SQL)
> - %A - datetime (string, datetime in SQL)
> - %t - timestamp (string, timestamp in SQL)
> - %T - timestamp with zone (string, timestamp with zone in SQL)
I confess I'm alarmed and frightened by all this, although I am
still willing to be convinced of its necessity. At the very least,
I'd hope that most of the processing can occur in set of routines
common to all drivers, otherwise it'll just be terribly difficult to
maintain going forward if each driver implements the parsing for
all this on its own.
Dealing just with LOB types, I'm also concerned about whether
using just a brigade as input data is possible. It may also
prove to be difficult to provide a portable interface; from what
little I know about MySQL, it handles LOBs like any other data
type, so a simple brigade might be OK. Oracle, as I noted, requires
a whole song and dance if you want to insert or update LOB data
into a row as an atomic transaction. If you want to make Oracle
do that, you really need to pass in the following:
- LOB data
- LOB data length
- table name
- LOB column name
(Although it's sometimes possible to determine the last two from
the SQL query, it's not always so.)
Anyway, I'll think on the various suggestions for a bit before
making other comments. And maybe someday I'll actually get back
to writing some code. :-)
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: LONGs and LOBs [Was: improved error reporting for
apr_dbd_mysql.c]
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Alex Dubov <oa...@yahoo.com>:
> I'm actually ok with this. But why do you need 4 blob
> types? I say - give backend a bucket and it should
> figure by itself what to do with it (use smallest blob
> type available that fits). I would also propose a
> plain bucket type, to simplify things:
>
> %b - bucket
> %B - bucket brigade
The above was actually my first thought, but then I had a look at the
Oracle driver and noticed that column types are being explicitly set.
Like this:
------------------------------------------
sql->status = OCIAttrSet(statement->args[i].value.lobval,
OCI_DTYPE_LOB, &null, 0,
OCI_ATTR_LOBEMPTY, sql->err);
------------------------------------------
I don't know Oracle API at all (nor did I actually check any
documentation), but I left the possiblity for more than one of those
types, just in case.
But, if we can simplify that to %b (bucket) and %B (brigade) as per
above, I would be more than happy to go with it. I'm sure Oracle folks
will let us know if that's good or not.
> By the way, what are the plans for apu-2 (when it
> should be ready)?
Dunno. I think some long time APR/APU folks will have to answer this.
--
Bojan
Re: LONGs and LOBs [Was: improved error reporting for apr_dbd_mysql.c]
Posted by Alex Dubov <oa...@yahoo.com>.
> We already have:
>
> - %s - string (i.e. char *, varchar in SQL)
> - %d - integer
> - %f - float
>
> I think we should stick with a single letter
> convention, for
> simplicity of parsing. So, how about:
>
> - %S - string (i.e. char *, text in SQL)
> - %D - unsigned integer
> - %c - char
> - %C - unsigned char
> - %h - short
> - %H - unsigned short
> - %l - long
> - %L - unsigned long
> - %q - quad (i.e. long long)
> - %Q - unsigned quad
> - %F - double
> - %b - blob (brigade, backend specific blob type 1
> in SQL*)
> - %B - blob (brigade, backend specific blob type 2
> in SQL*)
> - %o - blob (brigade, backend specific blob type 3
> in SQL*)
> - %O - blob (brigade, backend specific blob type 4
> in SQL*)
> - %i - time (string, time in SQL)
> - %a - date (string, date in SQL)
> - %A - datetime (string, datetime in SQL)
> - %t - timestamp (string, timestamp in SQL)
> - %T - timestamp with zone (string, timestamp with
> zone in SQL)
>
> * Blob types vary between backends a lot (e.g. MySQL
> has four types,
> Oracle at least 3 and so on). We should pick a few
> we'd like to
> implement in each.
I'm actually ok with this. But why do you need 4 blob
types? I say - give backend a bucket and it should
figure by itself what to do with it (use smallest blob
type available that fits). I would also propose a
plain bucket type, to simplify things:
%b - bucket
%B - bucket brigade
By the way, what are the plans for apu-2 (when it
should be ready)?
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: LONGs and LOBs [Was: improved error reporting for
apr_dbd_mysql.c]
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Alex Dubov <oa...@yahoo.com>:
> from here everything works just like with
> printf - if you push pointer incompatible to
> specifier, it's most likely a crash.
Ah, right. I think I'm starting to understand this now - I thought we
were going to end up with a bunch of backend specific stuff (which I
don't like much :-), but it doesn't have to be that way.
So, it seems that we need to define, on DBD level, what identifiers
mean. They have to convey both the meaning in C and SQL, because
drivers needs hints on how to map C types to SQL types in _prepare.
Getting the stuff once the select is done is easier, but we should
probably stick with the same letters, to make it easier for callers.
We already have:
- %s - string (i.e. char *, varchar in SQL)
- %d - integer
- %f - float
I think we should stick with a single letter convention, for
simplicity of parsing. So, how about:
- %S - string (i.e. char *, text in SQL)
- %D - unsigned integer
- %c - char
- %C - unsigned char
- %h - short
- %H - unsigned short
- %l - long
- %L - unsigned long
- %q - quad (i.e. long long)
- %Q - unsigned quad
- %F - double
- %b - blob (brigade, backend specific blob type 1 in SQL*)
- %B - blob (brigade, backend specific blob type 2 in SQL*)
- %o - blob (brigade, backend specific blob type 3 in SQL*)
- %O - blob (brigade, backend specific blob type 4 in SQL*)
- %i - time (string, time in SQL)
- %a - date (string, date in SQL)
- %A - datetime (string, datetime in SQL)
- %t - timestamp (string, timestamp in SQL)
- %T - timestamp with zone (string, timestamp with zone in SQL)
I'm sure there are many more combos that we can consider, but these
should be a decent start.
Let the flame wars begin... ;-)
* Blob types vary between backends a lot (e.g. MySQL has four types,
Oracle at least 3 and so on). We should pick a few we'd like to
implement in each.
--
Bojan
Re: LONGs and LOBs [Was: improved error reporting for apr_dbd_mysql.c]
Posted by Alex Dubov <oa...@yahoo.com>.
> The other problem is of input arguments for prepared
> statements, where
> again we don't have support for anything but zero
> terminated strings
> at the moment. With a generic structure that Nick
> suggested, we could
> solve that problem too.
I think we need to straighten this out. Nick basically
proposes to use structure similar to MYSQL_BIND one -
you grab the structure, fill it with data (type,
length) and pass it to the dbd (I saw that Nick
currently suggests to append only type tag). I'm
thinking about different approach - you have your data
(including bucket or bb for binaries); you have format
specifier; from here everything works just like with
printf - if you push pointer incompatible to
specifier, it's most likely a crash.
There's nothing in p(v)select that prevents it from
using generic pointers. People are doing this for
years with printf and it never was a problem. Another
way to view this: specifier is a type tag, but there's
no need to carry it all over as there's no easy way
for user to act upon it. It's not like type of any
specific field is going to change suddenly and apr_dbd
is not implemented in Scheme (may be it should be).
I'm using this approach for a while now and it works
rather well.
Of course, back-ends should not be obligated to
support anything beside strings, but it will be nice
for them to actively reject specifiers they don't
support.
The symmetric case, with get_entry implemented in a
scanf style is not difficult to implement, at least
with mysql. libmysqlclient does all the needed
conversions itself (I don't know about other
databases, but they can honour only strings and reject
everything else, if they want to). Additional
advantage here is movement of numbers - you can skip
allocation and simply fetch into user supplied
pointers. Even better: this approach makes get_row
unneeded, simplifying typical db access.
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: LONGs and LOBs [Was: improved error reporting for
apr_dbd_mysql.c]
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Alex Dubov <oa...@yahoo.com>:
>> Second, instead of paying attention to various
>> letters like %B, %L, %s
>> and so on, which come from when the statement was
>> prepared, we ignore
>> them in p[v]bquery/bselect, but we pass
>> apr_dbd_entry_t ** instead of
>> const char ** into the function. Then we have both
>> the type/length
>> figured out explicitly and can pass binary data in
>> safely, without the
>> need to do dubious strlen()'s.
I was actually referring there to the input arguments of prepared
statements, not values pulled out of select.
> void* apr_dbd_get_entry_as(apr_dbd_type_t type)
>
> or may be even
>
> int apr_dbd_get_entries(char *format, ...)
Right. But we're still no better off - we still need to be able to
store all kinds of types in something we understand with just APU DBD.
The underlying problem is that we don't have a way of returning binary
data (i.e. buffer + length or something even smarter, like a brigade)
from get_entry(). Nick's little structure solves that problem for
getting the entries out of select, because it tells us all we need to
know. Your solution goes a step further and attempts to cast things as
well for the caller, but will still need something (or many of those)
known to the caller on APU DBD level to store all kinds of stuff.
The other problem is of input arguments for prepared statements, where
again we don't have support for anything but zero terminated strings
at the moment. With a generic structure that Nick suggested, we could
solve that problem too.
--
Bojan
Re: LONGs and LOBs [Was: improved error reporting for apr_dbd_mysql.c]
Posted by Alex Dubov <oa...@yahoo.com>.
>
> Second, instead of paying attention to various
> letters like %B, %L, %s
> and so on, which come from when the statement was
> prepared, we ignore
> them in p[v]bquery/bselect, but we pass
> apr_dbd_entry_t ** instead of
> const char ** into the function. Then we have both
> the type/length
> figured out explicitly and can pass binary data in
> safely, without the
> need to do dubious strlen()'s.
>
I'm afraid that this will unnecessary complicate an
already ugly code. That is, data types are most often
known in advance and any above typical database code
is not a pretty picture even without the need to
assemble additional data structures. I would rather
prefer something like:
void* apr_dbd_get_entry_as(apr_dbd_type_t type)
or may be even
int apr_dbd_get_entries(char *format, ...)
(akin to scanf) which returns data cast as desired
type if possible, or null.
NULLs are moderately rare type to fetch - I think it's
always better to filter them out on SQL level.
Therefore:
int apr_dbd_entry_is_null(const apr_dbd_row_t *row,
int n)
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: LONGs and LOBs [Was: improved error reporting for
apr_dbd_mysql.c]
Posted by Bojan Smojver <bo...@rexursive.com>.
Quoting Chris Darroch <ch...@pearsoncmg.com>:
> You have to do the insert with an empty CLOB "placeholder", then
> retrieve the ID for the newly inserted row, perform a "SELECT ...
> FOR UPDATE" statement on that row ID, retrieve the OCILobLocator* for
> the CLOB from the output parameters, and finally perform OCILobWrite()
> on that locator. Oh, and of course you need to wrap all this in
> a transaction in the case where the user hasn't already started one,
> so that you don't wind up with just the newly inserted row but
> no CLOB data in there if any error occurs along the way.
You should have warned us to take Dramamine ;-)
> So, a quick summary:
>
> - get_entry() would need a buffer length return value to properly
> support BLOBs
> - get_entry() could also use an argument to flag NULLs vs. errors
> - get_entry() could do better with memory allocation and charset issues
> for LONGs/LOBs
I like Nick's idea with a little structure and enum/union inside.
However, we can have a new function instead, so that we remain
backward compatible. Someting like apr_dbd_entry_get(). Or, if that's
no good because people may expect intuitively that this is the same as
apr_dbd_get_entry(), then maybe apr_dbd_binary_get() or something like
that.
> - p[v]query() supports %L now for use with LONGs and LOBs
> - p[v]query() could implement a lot of logic for "better" LOB support,
> in which case they'll use %C[:colname] and %B[:colname] options
> - p[v]query() will also need a buffer length argument to support BLOBs fully
> - p[v]select() could support %L, %C, and %B too, but I'm unsure if
> there's real utility in that
Why don't we use the above tricks for p[v]query/p[v]select.
First, we implement new function names to be backward compatible
(something like p[v]bquery/bselect - "b" for binary). Some people may
prefer to continue using the string based stuff (because it's simple)
- no need to ditch it completely.
Second, instead of paying attention to various letters like %B, %L, %s
and so on, which come from when the statement was prepared, we ignore
them in p[v]bquery/bselect, but we pass apr_dbd_entry_t ** instead of
const char ** into the function. Then we have both the type/length
figured out explicitly and can pass binary data in safely, without the
need to do dubious strlen()'s.
--
Bojan
LONGs and LOBs [Was: improved error reporting for apr_dbd_mysql.c]
Posted by Chris Darroch <ch...@pearsoncmg.com>.
Nick Kew wrote:
> The Oracle driver uses uppercase; %C for CLOB and %B for BLOB,
> %L for LOB (hey, I can't even remember which of these are working
> and which are placeholders!). Is there any reason not to follow that
> convention for other drivers implementing LOBs?
The Oracle driver is in an interim state at the moment with respect
to CLOBs and BLOBs. I use it in production with CLOBs, but it's not
as efficient as it could be. I will get back to it someday!
As output parameters, CLOBs work OK. There's undoubtedly some
excess memory allocation, though. LONGs should work too.
BLOBs I haven't tested in a while; the principal problem is that
apr_dbd_get_entry() has no way to return a buffer length; since BLOBs
may contain byte values of zero, they shouldn't really be returned
as a null-terminated string. That suggests to me the need for an API
change down the road. Similar issues exist for passing BLOBs as
input parameters to p[v]query() too, of course.
While we're on the subject, I believe there's also no easy way to
distinguish between an error return from apr_dbd_get_entry() and
the return of a valid NULL column value. If we're changing the
get_entry() function at some point in the future, could we add
something like an int *null_flag argument that's set to 0 or 1?
As for LONGs and LOBs as input parameters, things are a little
more sticky. At the moment, %L is the thing that works. When
p[v]query() runs, it uses OCIBindByPos() to bind the input string
to the prepared statement. This call requires a length argument,
so strlen() is used, which isn't ideal for LONG/LOB values, which
one might reasonably expect to be large.
IIRC, the "right" way to handle LOBs here is really painful.
You can't pass a CLOB value to an INSERT statement directly with
OCIBindByPos(), for instance, the way you can most other simple
input value types.
You have to do the insert with an empty CLOB "placeholder", then
retrieve the ID for the newly inserted row, perform a "SELECT ...
FOR UPDATE" statement on that row ID, retrieve the OCILobLocator* for
the CLOB from the output parameters, and finally perform OCILobWrite()
on that locator. Oh, and of course you need to wrap all this in
a transaction in the case where the user hasn't already started one,
so that you don't wind up with just the newly inserted row but
no CLOB data in there if any error occurs along the way.
Worse, there's no way to query a prepared OCI statement handle to
find out, say, what the table name in question is. Perl's DBD::Oracle
performs a post_execute_lobs() function that runs after the main
OCIStmtExecute() call in dbd_st_execute() if the initial pass
over the input parameters determined that any were LOBs. The
post_execute_lobs() function then calls init_lob_refetch() to
do a lot of magic -- basically, it does its own parsing of your
SQL statement to find the table name, then uses OCIDescribeAny()
to get the schema for that table and tries to match up your
original input parameters against the fields in the table.
Of course, there are situations where it can't do this; the
DBD::Oracle manual says:
> One further wrinkle: for inserts and updates of LOBs, DBD::Oracle has
> to be able to tell which parameters relate to which table fields. In
> all cases where it can possibly work it out for itself, it does, how-
> ever, if there are multiple LOB fields of the same type in the table
> then you need to tell it which field each LOB param relates to:
>
> $sth->bind_param($idx, $value, { ora_type=>ORA_CLOB, ora_field=>'foo' }
Our %C and %B options correspond to Perl's ora_type in that they let
us know in advance if there are any LOBs among the input parameters.
However, even if we borrowed Perl's init_lob_refetch() magic parsing,
we'd still need some further kind of hint in the query string; something
like %C:colname to correspond to the ora_field hint.
I should add: IANAL, so I don't know what's required in terms of
licenses and legal stuff if logic is re-implemented from DBD::Oracle
in APR.
Once the %C and %B options are functional, they could be supported
in p[v]select() as well, but the utility of that is somewhat uncertain
to me; you can't usually use a LONG/LOB value in a WHERE clause.
It's possible some DBMS_LOB-type PL/SQL functions might allow it,
though; I'm not expert on those.
So, a quick summary:
- get_entry() would need a buffer length return value to properly
support BLOBs
- get_entry() could also use an argument to flag NULLs vs. errors
- get_entry() could do better with memory allocation and charset issues
for LONGs/LOBs
- p[v]query() supports %L now for use with LONGs and LOBs
- p[v]query() could implement a lot of logic for "better" LOB support,
in which case they'll use %C[:colname] and %B[:colname] options
- p[v]query() will also need a buffer length argument to support BLOBs fully
- p[v]select() could support %L, %C, and %B too, but I'm unsure if
there's real utility in that
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c - correct version
Posted by Alex Dubov <oa...@yahoo.com>.
> Ah, yeah. The good old bucket to the rescue.
> Completely missed that...
May be even brigade of them - this will enable the use
of mysql_stmt_send_long_data to send a lot of data
without flattening everything into single buffer.
Then it came to me that instead of allocating large
buffer in pselect, it's possible to allocate some
small buffer, check for truncation and then
re-allocate and re-fetch problematic columns in
get_entry, if needed (and to think that I removed
*_fetch_column as not needed, because it actually did
nothing, but it should have been a hint).
Unfortunately, there's no incremental fetch in mysql.
Anyway, I'll have some time to implement and test this
toward next week.
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c -
correct version
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2006-05-29 at 11:24 +0100, Nick Kew wrote:
> The Oracle driver uses uppercase; %C for CLOB and %B for BLOB,
> %L for LOB (hey, I can't even remember which of these are working
> and which are placeholders!). Is there any reason not to follow that
> convention for other drivers implementing LOBs?
No, not really. Apart from me being unaware of it being in the Oracle
driver :-)
We should stick to "the same" as much as we can in DBD. Otherwise,
people will have to code their "DBD SQL" for each driver separately,
which kinda defeats the purpose of DBD.
> The bucket would seem more appropriate for LOB types. Especially
> if the underlying database wants to stream them, use indirection,
> or do anything else 'clever'.
Ah, yeah. The good old bucket to the rescue. Completely missed that...
Thanks Nick!
--
Bojan
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c - correct version
Posted by Nick Kew <ni...@webthing.com>.
On Monday 29 May 2006 03:25, Bojan Smojver wrote:
> On Sun, 2006-05-28 at 18:55 -0700, Alex Dubov wrote:
> > Yes, it was a reason why I started to play with
> > apr_dbd_mysql. I'm using a lot of binary data and I
> > must use 'pascal' style strings to do this (so there
> > is distinction between '%s' and '%c').
>
> I understand. However, this API is primarily a C API, so most people
> will associate %s with a zero terminated string. Maybe having a %b, for
> blob, would help?
The Oracle driver uses uppercase; %C for CLOB and %B for BLOB,
%L for LOB (hey, I can't even remember which of these are working
and which are placeholders!). Is there any reason not to follow that
convention for other drivers implementing LOBs?
> > I asked Nick if we can make this "struct blob_t"
> > common to apr_dbd. It's a handy little struct after
> > all.
And I replied that such discussions belong on-list:-)
The convention throughout apr_dbd is that datatypes belong
to the drivers, and are opaque in the public API. If we depart
from this, we may be imposing unnecessary burdens on the
implementation of new drivers.
> I think this would actually be the correct approach. Something like:
>
> typedef struct apr_dbd_blob apr_dbd_blob_t;
> struct apr_dbd_blob {
> apr_size_t length;
> char *data[];
> };
>
> Maybe there is such a structure in APR already that we're unaware of...
The bucket would seem more appropriate for LOB types. Especially
if the underlying database wants to stream them, use indirection,
or do anything else 'clever'.
--
Nick Kew
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c -
correct version
Posted by Bojan Smojver <bo...@rexursive.com>.
On Mon, 2006-05-29 at 00:21 -0700, Alex Dubov wrote:
> I don't like it
> because it requires two allocations (one for struct
> and other for data).
The *data[] was a typo, actually... I just wanted the size to be APR
size, the rest is good.
--
Bojan
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c - correct version
Posted by Alex Dubov <oa...@yahoo.com>.
> I understand. However, this API is primarily a C
> API, so most people
> will associate %s with a zero terminated string.
> Maybe having a %b, for
> blob, would help?
Of course, letter is not important at all. But the
data type is.
>
> I think this would actually be the correct approach.
> Something like:
>
> typedef struct apr_dbd_blob apr_dbd_blob_t;
> struct apr_dbd_blob {
> apr_size_t length;
> char *data[];
> };
>
I tried to find something similar in apr, but failed.
Basically, you are proposing to use libc's "struct
iovec", may be in apr branded fashion. I don't like it
because it requires two allocations (one for struct
and other for data). On the other hand, "inline"
arrays are quite popular in the linux kernel and
people seem to like them, though linux often uses
definitions like:
struct { int len; char data[1]; }
(because of C90 compliance). C99 allows flexible array
members, which I used.
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c -
correct version
Posted by Bojan Smojver <bo...@rexursive.com>.
On Sun, 2006-05-28 at 18:55 -0700, Alex Dubov wrote:
> Yes, it was a reason why I started to play with
> apr_dbd_mysql. I'm using a lot of binary data and I
> must use 'pascal' style strings to do this (so there
> is distinction between '%s' and '%c').
I understand. However, this API is primarily a C API, so most people
will associate %s with a zero terminated string. Maybe having a %b, for
blob, would help?
> I asked Nick if we can make this "struct blob_t"
> common to apr_dbd. It's a handy little struct after
> all.
I think this would actually be the correct approach. Something like:
typedef struct apr_dbd_blob apr_dbd_blob_t;
struct apr_dbd_blob {
apr_size_t length;
char *data[];
};
Maybe there is such a structure in APR already that we're unaware of...
--
Bojan
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c - correct version
Posted by Alex Dubov <oa...@yahoo.com>.
Yes, it was a reason why I started to play with
apr_dbd_mysql. I'm using a lot of binary data and I
must use 'pascal' style strings to do this (so there
is distinction between '%s' and '%c').
I asked Nick if we can make this "struct blob_t"
common to apr_dbd. It's a handy little struct after
all.
--- Bojan Smojver <bo...@rexursive.com> wrote:
> Just going through the "bind arguments" portion of
> the patch. In the new
> code, %s expects BLOB - is this intentional? I would
> think more people
> would intuitively think of %s as a zero terminated
> string...
>
> --
> Bojan
>
>
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c -
correct version
Posted by Bojan Smojver <bo...@rexursive.com>.
Just going through the "bind arguments" portion of the patch. In the new
code, %s expects BLOB - is this intentional? I would think more people
would intuitively think of %s as a zero terminated string...
--
Bojan
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c - correct version
Posted by Alex Dubov <oa...@yahoo.com>.
Ok. Here is a patch against webthing's rev 34 (I
assume it's a last one). There's a brief changelog in
the beginning of the file.
Two warnings, however:
1. I only use mysql-5
2. I only work with prepared statements
(I don't know if there are any errors in other modes
of operation; I also believe that there can be serious
differences between mysql-5 and mysql-4 in handling of
prepared statements).
Also: my patch does not uses is_null and error arrays
(these are set to zero) - instead is_null_value and
error_value are checked in result's BIND structure.
I also don't have any pre-set bounds on maximum field
length and I don't know what happens if somebody tries
to pass LONG BLOB (the only type which can have
unreasonable size). It's easy to work around long
blobs using SUBSTRING sql statement with offset/size
as parameters, but in general some sort of
truncation/continuation interface is probably needed
(and some interface for mysql_stmt_send_long_data, to
send long data back to db). On the other hand, the
need for such long data in databases is rather limited.
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: [PATCH]: improved error reporting for apr_dbd_mysql.c - correct version
Posted by Nick Kew <ni...@webthing.com>.
On Saturday 20 May 2006 06:03, Alex Dubov wrote:
> Even though some people incorrectly assumed that my
> version of apr_dbd_mysql is mostly about error
> reporting, the feature list is as following:
>
> 1. Works correctly with 5.0.xx
> 2. Supports most data types for parameters and results
> (through printf style format - assumes that result
> types are known in advance or set through CAST AS in
> query)
Aha. I remember that one, but don't recollect why it got into
the "postponed" intray. Worth raising again. Is your patch
somewhere I can find it on the mail archives, in case I'm unsure
about versions searching my own mailbox?
> 3. Correctly deals with corner cases (large unsinged
> bigints anybody?)
> 4. Support for binary types and blobs (returns pointer
> to length/data struct)
> 5. Error codes that mean stuff (as of now, and not in
> some remote future)
IIRC it was a worthwhile patch:-)
--
Nick Kew