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

Nice patches, but could you please provide ones without the whitespace changes? Now it's not very clear what you have actually changed.. --Jani On Mon, 7 Apr 2003, SQUILLACE MASSIMO wrote: >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 > -- <- For Sale! ->