apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bojan Smojver <bo...@rexursive.com>
Subject Re: DBD: Prepared statements, BLOBs etc.
Date Wed, 06 Sep 2006 20:34:55 GMT
On Tue, 2006-09-05 at 23:39 -0700, Chris Darroch wrote:

>    Firstly, Bojan, thank you for all the work on these patches!  I read
> through them last week and reviewed quickly your latest updates, and
> really, I don't see much to discuss -- it's all looking very close to
> what I might hope for!

No worries at all. Thanks for reviewing!

>    Here are my few thoughts so far.  They may not be completely cogent.
> 
> 1) The one thing that stood out for me was that apr_dbd_datum_get()
>    seems to take an apr_dbd_type_e parameter.  The drivers seem to
>    then format the result data from the DB into the requested type.
>    This means that you can ask for an APR_DBD_TYPE_NULL, for example,
>    regardless of what the data is.
> 
>    Now, I may very well be missing something here, but this seems
>    backwards to me.  I would have thought that you'd want to pass
>    an apr_dbd_type_e *type parameter instead, and then datum_get()
>    would write into that location the type of the column, and
>    write into the void *data parameter the actual data, formatted
>    as one would expect for the returned type.  So if the column
>    contains integers, you'd get a type of APR_DBD_TYPE_INT and
>    a pointer to a signed int, and so forth.

I was thinking about doing it this way as well and maybe it's the right
way - not sure. Here is why I chose to do it the way I did:

In majority (or all?) of my own database related work, I would know the
column type ahead of getting the data, because I was usually the one
that created the table, had prior knowledge of its structure or could
find out in some SQL way. So, I was thinking, why wait until after the
call to figure out what the column type is (and have another if/switch
construct after datum_get()) - let's specify it upfront.

The next thing that felt good about this approach is that the storage is
entirely managed by the caller. So, I already have my ints, shorts,
longs, brigades and other data types allocated - then I call datum_get()
with pointers to the allocated data, and I get everything populated and
ready to be used (minus the "is it null?" bit - see below).

And, it also gives us the opportunity to convert various data on the fly
with datum_get(), without any extra programming effort on caller's
behalf (to a degree, of course).

>    Further, if the data in that row for that column happens to be a NULL,
>    you'd get an APR_DBD_TYPE_NULL type and a NULL pointer.  That way,
>    you don't need to use the proposed apr_dbd_is_null() function unless
>    you're using the string-only API (i.e., the non-binary-data functions).

In the patches, I return APR_ENOENT from datum_get() if I find a null
value, so no need for apr_dbd_is_null() call. Except that I've forgotten
to do this for SQLite2... Sorry, I'll try to fix that.

> 2) Alas, for Oracle, there should probably also be the types
>    APR_DBD_TYPE_CLOB and APR_DBD_TYPE_BFILE, but I can add those
>    if I ever get around to implementing the Oracle version of this!  :-(

If that's what we need to do, sure, why not. You're the Oracle guy! :-)

> 3) Defining apr_dbd_blob in apr_dbd.h means it's going to wind up
>    fixed across all drivers.  Oracle would need an apr_dbd_clob too,
>    or at least a common apr_dbd_lob (since the type would be specified
>    elsewhere, but the data, length, table, and column need to be known
>    at bind time for all LOBs in Oracle, IIRC).
> 
>    Do you see any way an apr_dbd_lob could be made private to the driver,
>    and the required data fields in it queried from the driver by
>    a kind of reflection method?  At one point, I proposed something
>    like this, but I'll modify it a bit here:
> 
> typedef enum {
>     APR_DBD_FLAGS_TABLE_NAME,
>     APR_DBD_FLAGS_COLUMN_NAME
> } apr_dbd_flags_e;
> 
> APU_DECLARE(apr_dbd_flags_e)
>     apr_dbd_type_flags_get(const apr_dbd_driver_t *driver,
>                            apr_dbd_type_e type);
> 
>    I'm thinking here that if you knew you were passing in a particular
> type of bound argument after an apr_dbd_prepare(), you could query to
> find out what fields you need to pack, either in the old-style string
> argument or in the new-style binary structure.  Or it could be called
> apr_dbd_lob_flags_get() and return an apr_dbd_lob_flags_e value:
> that would clarify its usage at the expense of using the method for
> any future kind of non-LOB data type.

Yeah, I see that this would be a lot more generic (i.e. you'd
dynamically figure out what needs to be put in - similar to figuring out
the data types in datum_get()). I was thinking along the lines of
keeping it relatively simple, by having a cover-all structure (which, of
course, is a recipe for breaking binary compatibility when we need more
fields :-).

In other words, in _prepare, we get in %p<something>, which means CLOB,
LOB, FILE or any other additional types that Oracle (and other DBs) may
support.

Then, in the _p[b]query/select (since we know the type from _prepare),
we get ourselves just a regular apr_dbd_blob_t, but we know to call the
correct bind functions and use the correct parts of it (i.e. length, or
table name, table name etc.). The apr_dbd_blob_t is just a data delivery
mechanism. We could have others - for instance apr_dbd_file_t (for
APR_DBD_TYPE_BFILE), which could look something like this:

struct apr_dbd_file {
    apr_file_t *file;   /**< file to read the data from */
    apr_size_t size;    /**< size to read */
    apr_off_t  offset;  /**< offset to read from */
    const char *table;  /**< table name (used for Oracle) */
    const char *column; /**< column name (used for Oracle) */
};

Again, thanks for reviewing and I hope you find my answer useful.

-- 
Bojan


Mime
View raw message