From dev-return-17172-apmail-apr-dev-archive=apr.apache.org@apr.apache.org Wed Sep 06 20:35:28 2006 Return-Path: Delivered-To: apmail-apr-dev-archive@www.apache.org Received: (qmail 64260 invoked from network); 6 Sep 2006 20:35:20 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (209.237.227.199) by minotaur.apache.org with SMTP; 6 Sep 2006 20:35:20 -0000 Received: (qmail 66658 invoked by uid 500); 6 Sep 2006 20:35:20 -0000 Delivered-To: apmail-apr-dev-archive@apr.apache.org Received: (qmail 66305 invoked by uid 500); 6 Sep 2006 20:35:19 -0000 Mailing-List: contact dev-help@apr.apache.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Id: Delivered-To: mailing list dev@apr.apache.org Received: (qmail 66289 invoked by uid 99); 6 Sep 2006 20:35:19 -0000 Received: from asf.osuosl.org (HELO asf.osuosl.org) (140.211.166.49) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 06 Sep 2006 13:35:19 -0700 X-ASF-Spam-Status: No, hits=0.4 required=10.0 tests=SPF_HELO_PASS,SPF_PASS,WHY_WAIT X-Spam-Check-By: apache.org Received-SPF: pass (asf.osuosl.org: domain of bojan@rexursive.com designates 203.171.74.242 as permitted sender) Received: from [203.171.74.242] (HELO beauty.rexursive.com) (203.171.74.242) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 06 Sep 2006 13:35:18 -0700 Received: from [172.27.0.24] (shrek.rexursive.com [172.27.0.24]) by beauty.rexursive.com (Postfix) with ESMTP id AD451254A48 for ; Thu, 7 Sep 2006 06:34:55 +1000 (EST) Subject: Re: DBD: Prepared statements, BLOBs etc. From: Bojan Smojver To: APR Development List In-Reply-To: <44FE6D0B.90004@pearsoncmg.com> References: <1155589108.3289.26.camel@shrek.rexursive.com> <44EF3BEF.7060901@pearsoncmg.com> <1156537930.2596.1.camel@shrek.rexursive.com> <20060831092218.nh0wn8ls00cg0c88@www.rexursive.com> <20060904084356.7prsyaa30g4kok08@www.rexursive.com> <44FE6D0B.90004@pearsoncmg.com> Content-Type: text/plain Date: Thu, 07 Sep 2006 06:34:55 +1000 Message-Id: <1157574895.22901.43.camel@shrek.rexursive.com> Mime-Version: 1.0 X-Mailer: Evolution 2.8.0 (2.8.0-1.fc6) Content-Transfer-Encoding: 7bit X-Virus-Checked: Checked by ClamAV on apache.org X-Spam-Rating: minotaur.apache.org 1.6.2 0/1000/N 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, 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