[PATCH] OCI8 module - proposed patches (PART 1 of 2)

Hi, I had to split the original message in two e-mails because it was longer than 30000 bytes. In this first message you'll find the commentary and the first attachment, the second message contains the second attachment. Massimo ------------------------------------------------------------------------ ------------------------------------------------ Hi all, since posting bug #22674 resulted in no feedback to date and I believe OCI8 persistent session management should quickly be fixed, I studied the sources and came up with a solution to that particular bug and more. The solutions have been working for a week now on two PHP 4.2.2 and two PHP 4.3.1 servers, so this weekend I downloaded the current CVS snapshot and applied them there. Here follows a description of the problems and the proposed patches: 1) [Solves bug #22674, part 1] The sequence OCIPLOGON('user1', 'pwd1', 'db1') + OCILOGON('user2', 'pwd2', 'db1') creates two persistent session handles instead of one, because the OCILOGON session wrongly "inherits" the persistency attribute of the OCIPLOGON session; BOTH sessions can be reused. I fixed one assignment statement in oci_do_connect. 2) [Solves bug #22674, part 2] The sequence OCIPLOGON + OCILOGON + OCINLOGON (same dbname for all, same userid and password for the latter 2 calls) still creates two persistent session handles instead of one, but the session structure set up by OCILOGON is incorrectly freed when the module cleans up the OCINLOGON structures, so that while Oracle still sees the surplus session PHP cannot reuse it in subsequent scripts. This leads to a session build-up with Oracle eventually giving up. The problem lies in the way hashed_details is built: it is the same for OCILOGON and OCINLOGON, so when zend_hash_del is called during cleanup it deletes BOTH structures. I re-defined the hashed_details for OCINLOGON using gettimeofday(), so each OCINLOGON structure is now unique and different from any OCILOGON structure. These first two patches are in the attached twobugs.txt file; if they are accepted bug #22674 may be closed. I also attach a fourpatches.txt file, which contains the above solutions and two more patches I'd like to submit to your attention, since I believe they could help make the module more flexible: 3) More often than not Oracle installations do NOT provide for a user-defined "password verification function", so that userid, password and dbname are treated case-insensitively by the RDBMS, while the OCI8 module generates hashed_details that are case-sensitive. If the programmers mix case during development this leads to an excessive number of persistent sessions to Oracle. I introduced a php_ini boolean variable credentials_toupper which, when true (defaults to false), triggers uppercase conversion of userid, password and dbname before their use in oci_do_connect. 4) When the web servers are behind a firewall in a DMZ, the firewall truncates sessions when they reach a configurable but unavoidable inactivity timeout. This invalidates persistent sessions and in my experience (if the firewall doesn't specifically support SQL*NET) can't always be circumvented by a decrease in the tcp_keepalive_time; also, I'd rather leave such parameters at their default and let the firewall do its work which is dictated by security concerns. Idle persistent sessions are useless by themselves and should anyway be closed as soon as feasible to minimize load on the RDBMS. In past client-server (OS/2) applications I came up with a thread-based solution which automatically closed the client's Oracle connection after a configurable inactivity timeout, but since I don't know if this should be done in the current PHP implementation I simply decided to add a timestamp field in the oci_server structure and check in the PHP_RINIT_FUNCTION if a php_ini configurable connection_timeout (defaults to -1, or no timeout) has expired. In the affirmative, the expired server handle and all associated session handles are correcly closed. Since this happens at request init I am sure the check triggers for all PHP scripts, even though they don't access the database, as this helps to keep the number of connections down. On the other hand a heavily accessed server will never timeout persistent connections. My tests indicate that a 300s timeout is high enough to guarantee peak Oracle performance while being low enough to never create firewall problems. Especially when coupled with the solutions to bug #22674, the overhead induced by the up-front check is negligible. As stated above, two new php_ini entries are proposed as follows: [OCI8] ;if true, internally convert userid, password and dbname to uppercase ;oci8.credentials_toupper = false ;inactivity timeout for persistent connections (seconds). -1 means no timeout. ;oci8.connection_timeout = -1 Hope this helps Massimo Squillace

? twobugs.patch Index: oci8.c =================================================================== RCS file: /repository/php4/ext/oci8/oci8.c,v retrieving revision 1.205 diff -u -r1.205 oci8.c --- oci8.c 18 Mar 2003 12:06:00 -0000 1.205 +++ oci8.c 6 Apr 2003 16:52:13 -0000 @@ -2153,30 +2153,32 @@ oci_session *session = 0, *psession = 0; OCISvcCtx *svchp = 0; char *hashed_details; + struct timeval tv; + int sec, usec; #ifdef HAVE_OCI9 ub2 charsetid; #endif TSRMLS_FETCH(); - /* + /* check if we already have this user authenticated - we will reuse authenticated users within a request no matter if the user requested a persistent + we will reuse authenticated users within a request no matter if the user requested a persistent connections or not! - - but only as pesistent requested connections will be kept between requests! - */ - hashed_details = (char *) malloc(strlen(SAFE_STRING(username))+ - strlen(SAFE_STRING(password))+ - strlen(SAFE_STRING(server->dbname))+1); - - sprintf(hashed_details,"%s%s%s", - SAFE_STRING(username), - SAFE_STRING(password), - SAFE_STRING(server->dbname)); + but only as persistent requested connections will be kept between requests! + */ if (! exclusive) { + hashed_details = (char *) malloc(strlen(SAFE_STRING(username))+ + strlen(SAFE_STRING(password))+ + strlen(SAFE_STRING(server->dbname))+1); + + sprintf(hashed_details,"%s%s%s", + SAFE_STRING(username), + SAFE_STRING(password), + SAFE_STRING(server->dbname)); + zend_hash_find(OCI(user), hashed_details, strlen(hashed_details)+1, (void **) &session); if (session) { @@ -2191,6 +2193,14 @@ /* breakthru to open */ } } + } else { + gettimeofday((struct timeval *) &tv, (struct timezone *) NULL); + sec = (int) tv.tv_sec; + usec = (int) (tv.tv_usec % 1000000); + /* The max value usec can have is 0xF423F, so we use only five hex + digits for usec and eigth hex digits for sec. */ + hashed_details = (char *) malloc(8+5+1); /* always enough */ + sprintf(hashed_details, "%08x%05x", tv.tv_sec, tv.tv_usec); } session = calloc(1,sizeof(oci_session)); @@ -2217,16 +2227,16 @@ CALL_OCI_RETURN(charsetid, OCINlsCharSetNameToId( OCI(pEnv), charset)); - + session->charsetId = charsetid; oci_debug("oci_do_connect: using charset id=%d",charsetid); } - + /* create an environment using the character set id, Oracle 9i+ ONLY */ CALL_OCI(OCIEnvNlsCreate( &session->pEnv, - OCI_DEFAULT, - 0, + OCI_DEFAULT, + 0, NULL, NULL, NULL, @@ -2245,12 +2255,12 @@ /* allocate temporary Service Context */ CALL_OCI_RETURN(OCI(error), OCIHandleAlloc( - session->pEnv, - (dvoid **)&svchp, - OCI_HTYPE_SVCCTX, - 0, + session->pEnv, + (dvoid **)&svchp, + OCI_HTYPE_SVCCTX, + 0, NULL)); - + if (OCI(error) != OCI_SUCCESS) { oci_error(OCI(pError), "_oci_open_session: OCIHandleAlloc OCI_HTYPE_SVCCTX", OCI(error)); goto CLEANUP; @@ -2258,10 +2268,10 @@ /* allocate private session-handle */ CALL_OCI_RETURN(OCI(error), OCIHandleAlloc( - session->pEnv, - (dvoid **)&session->pSession, - OCI_HTYPE_SESSION, - 0, + session->pEnv, + (dvoid **)&session->pSession, + OCI_HTYPE_SESSION, + 0, NULL)); if (OCI(error) != OCI_SUCCESS) { @@ -2269,11 +2279,11 @@ goto CLEANUP; } - /* Set the server handle in service handle */ + /* Set the server handle in service handle */ CALL_OCI_RETURN(OCI(error), OCIAttrSet( - svchp, - OCI_HTYPE_SVCCTX, - server->pServer, + svchp, + OCI_HTYPE_SVCCTX, + server->pServer, 0, OCI_ATTR_SERVER, OCI(pError))); @@ -2285,11 +2295,11 @@ /* set the username in user handle */ CALL_OCI_RETURN(OCI(error), OCIAttrSet( - (dvoid *) session->pSession, - (ub4) OCI_HTYPE_SESSION, - (dvoid *) username, - (ub4) strlen(username), - (ub4) OCI_ATTR_USERNAME, + (dvoid *) session->pSession, + (ub4) OCI_HTYPE_SESSION, + (dvoid *) username, + (ub4) strlen(username), + (ub4) OCI_ATTR_USERNAME, OCI(pError))); if (OCI(error) != OCI_SUCCESS) { @@ -2299,11 +2309,11 @@ /* set the password in user handle */ CALL_OCI_RETURN(OCI(error), OCIAttrSet( - (dvoid *) session->pSession, - (ub4) OCI_HTYPE_SESSION, - (dvoid *) password, - (ub4) strlen(password), - (ub4) OCI_ATTR_PASSWORD, + (dvoid *) session->pSession, + (ub4) OCI_HTYPE_SESSION, + (dvoid *) password, + (ub4) strlen(password), + (ub4) OCI_ATTR_PASSWORD, OCI(pError))); if (OCI(error) != OCI_SUCCESS) { @@ -2312,10 +2322,10 @@ } CALL_OCI_RETURN(OCI(error), OCISessionBegin( - svchp, - OCI(pError), - session->pSession, - (ub4) OCI_CRED_RDBMS, + svchp, + OCI(pError), + session->pSession, + (ub4) OCI_CRED_RDBMS, (ub4) OCI_DEFAULT)); if (OCI(error) != OCI_SUCCESS) { @@ -2325,7 +2335,7 @@ /* Free Temporary Service Context */ CALL_OCI(OCIHandleFree( - (dvoid *) svchp, + (dvoid *) svchp, (ub4) OCI_HTYPE_SVCCTX)); if (exclusive) { @@ -2333,7 +2343,7 @@ } else { zend_hash_update(OCI(user), session->hashed_details, - strlen(session->hashed_details)+1, + strlen(session->hashed_details)+1, (void *)session, sizeof(oci_session), (void**)&psession); @@ -2676,7 +2686,7 @@ persistent = 0; } else { /* if our server-context is not persistent we can't */ - persistent = server->persistent; + persistent = (server->persistent) ? persistent : 0; } session = _oci_open_session(server,username,password,persistent,exclusive,charset); @@ -2693,7 +2703,7 @@ connection->session->pEnv, (dvoid **)&connection->pError, OCI_HTYPE_ERROR, - 0, + 0, NULL)); if (OCI(error) != OCI_SUCCESS) {