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
