Chris Darroch
Mon, 19 May 2008 18:46:47 -0700
William A. Rowe, Jr. wrote:
Then my thought is to remove the code entirely from apr branch 1.3.x and encourage it's development and refinement on trunk.
Personally, I'd suggest just removing it from trunk too ... I'm
not sure exactly how a global cache could be made to work across
multiple databases, since they might be of different versions, etc.
It's not likely to be trivial, I would think, and I'm not sure what
the upside is.
So, anyway, here's a patch against trunk ... it should apply cleanly
against 1.3 as well, I believe. If you're not applying this to trunk
as well, please apply the previous patch, which at least moves the
apr_dbd_mutex_unlock() calls into matching #ifdefs. Thanks,
Chris.
================================================
--- dbd/apr_dbd_oracle.c.orig 2008-05-19 18:37:11.000000000 -0700
+++ dbd/apr_dbd_oracle.c 2008-05-19 18:41:07.000000000 -0700
@@ -42,22 +42,6 @@
* as a sysop using oracle would be a good start.
*/
-/*******************************************************************/
-
-/* GLOBAL_PREPARED_STATEMENTS
- *
- * This probably needs bindings taken away from prepare and into
- * execute, or else we'll get race conditions on the parameters
- * Might be able to do it with some pool refactoring.
- *
- * In fact, though the documentation says statements can run with
- * different interps and threads (IIRC), it doesn't say anything
- * about race conditions between bind and execute. So we'd better
- * not even try until and unless someone who knows oracle from the
- * inside fixes it.
-#define GLOBAL_PREPARED_STATEMENTS
- */
-
/* shut compiler up */
#ifdef DEBUG
#define int_errorcode int errorcode
@@ -205,9 +189,6 @@
* the right way to do it (if such a thing exists).
*/
static OCIEnv *dbd_oracle_env = NULL;
-#ifdef GLOBAL_PREPARED_STATEMENTS
-static apr_hash_t *oracle_statements = NULL;
-#endif
/* Oracle specific bucket for BLOB/CLOB types */
typedef struct apr_bucket_lob apr_bucket_lob;
@@ -374,46 +355,6 @@
}
}
-#ifdef GLOBAL_PREPARED_STATEMENTS
-static apr_status_t freeStatements(void *ptr)
-{
- apr_status_t rv = APR_SUCCESS;
- OCIStmt *stmt;
- apr_hash_index_t *index;
- apr_pool_t *cachepool = apr_hash_pool_get(oracle_statements);
-
-#ifdef PREPARE2
- OCIError *err;
-
- if (OCIHandleAlloc(dbd_oracle_env, (dvoid**)&err, OCI_HTYPE_ERROR, 0, NULL)
- != OCI_SUCCESS) {
- return APR_EGENERAL;
- }
-#endif
-
- for (index = apr_hash_first(cachepool, oracle_statements);
- index != NULL;
- index = apr_hash_next(index)) {
- apr_hash_this(index, NULL, NULL, (void**)&stmt);
-#ifdef PREPARE2
- if (OCIStmtRelease(stmt, err, NULL, 0, OCI_DEFAULT) != OCI_SUCCESS) {
- rv = APR_EGENERAL;
- }
-#else
- if (OCIHandleFree(stmt, OCI_HTYPE_STMT) != OCI_SUCCESS) {
- rv = APR_EGENERAL;
- }
-#endif
- }
-#ifdef PREPARE2
- if (OCIHandleFree(err, OCI_HTYPE_ERROR) != OCI_SUCCESS) {
- rv = APR_EGENERAL;
- }
-#endif
- return rv;
-}
-#endif
-
static void dbd_oracle_init(apr_pool_t *pool)
{
if (dbd_oracle_env == NULL) {
@@ -429,14 +370,6 @@
NULL, NULL, NULL, NULL, 0, NULL);
#endif
}
-#ifdef GLOBAL_PREPARED_STATEMENTS
- if (oracle_statements == NULL) {
-
- oracle_statements = apr_hash_make(pool);
- apr_pool_cleanup_register(pool, oracle_statements,
- freeStatements, apr_pool_cleanup_null);
- }
-#endif
}
static apr_dbd_t *dbd_oracle_open(apr_pool_t *pool, const char *params,
@@ -926,19 +859,6 @@
int i;
apr_dbd_prepared_t *stmt ;
-/* prepared statements in a global lookup table would be nice,
- * but we can't do that here because our pool may die leaving
- * the cached statement orphaned.
- * OTOH we can do that with Oracle statements, which aren't on
- * the pool, so long as we don't register a cleanup on our pool!
- *
- * FIXME:
- * There's a race condition between cache-lookup and cache-set
- * But the worst outcome is a statement prepared more than once
- * and leaked. Is that worth mutexing for?
- * Hmmm, yes it probably is ... OK, done
- */
-
if (*statement == NULL) {
*statement = apr_pcalloc(pool, sizeof(apr_dbd_prepared_t));
}
@@ -948,29 +868,6 @@
stmt->nargs = nargs;
stmt->nvals = nvals;
- /* If we have a label, we're going to cache it globally.
- * Check first if we already have it. If not, prepare the
- * statement under mutex, so we don't end up leaking
- * concurrent statements
- *
- * FIXME: Oracle docs say a statement can be used even across
- * multiple servers, so I assume this is safe .....
- */
-#ifdef GLOBAL_PREPARED_STATEMENTS
- if (label != NULL) {
- stmt->stmt = apr_hash_get(oracle_statements, label,
APR_HASH_KEY_STRING);
- if (stmt->stmt != NULL) {
- return ret;
- }
- apr_dbd_mutex_lock();
- stmt->stmt = apr_hash_get(oracle_statements, label,
APR_HASH_KEY_STRING);
- if (stmt->stmt != NULL) {
- apr_dbd_mutex_unlock();
- return ret;
- }
- }
-#endif
-
/* populate our own args, if any */
if (nargs > 0) {
stmt->args = apr_pcalloc(pool, nargs*sizeof(bind_arg));
@@ -982,7 +879,6 @@
sql->status = OCIHandleAlloc(dbd_oracle_env, (dvoid**) &stmt->stmt,
OCI_HTYPE_STMT, 0, NULL);
if (sql->status != OCI_SUCCESS) {
- apr_dbd_mutex_unlock();
return 1;
}
@@ -990,7 +886,6 @@
strlen(query), OCI_NTV_SYNTAX, OCI_DEFAULT);
if (sql->status != OCI_SUCCESS) {
OCIHandleFree(stmt->stmt, OCI_HTYPE_STMT);
- apr_dbd_mutex_unlock();
return 1;
}
@@ -1001,7 +896,6 @@
sql->status = OCIAttrGet(stmt->stmt, OCI_HTYPE_STMT, &stmt->type, 0,
OCI_ATTR_STMT_TYPE, sql->err);
if (sql->status != OCI_SUCCESS) {
- apr_dbd_mutex_unlock();
return 1;
}
@@ -1011,7 +905,6 @@
sizeof(prefetch_size), OCI_ATTR_PREFETCH_MEMORY,
sql->err);
if (sql->status != OCI_SUCCESS) {
- apr_dbd_mutex_unlock();
return 1;
}
#endif
@@ -1019,12 +912,6 @@
if (stmt->type == OCI_STMT_SELECT) {
ret = outputParams(sql, stmt);
}
-#ifdef GLOBAL_PREPARED_STATEMENTS
- if (label != NULL) {
- apr_hash_set(oracle_statements, label, APR_HASH_KEY_STRING,
stmt->stmt);
- apr_dbd_mutex_unlock();
- }
-#endif
return ret;
}
================================================