Description: ------------ Regarding: $Id: pgsql.php,v 1.83 2004/10/04 17:53:09 danielc Exp $ The function simpleQuery in DB::pgsql is flawed. It tests to see whether a SQL call is a "manipulation" query using DB::isManip. Only if isManip returns true does it begin a transaction. (When autoComit is false). This is flawed because in PgSQL SELECTS can alter data. Particularly when you want to call a stored procedure. With PostgreSQL you call stored procedures using SELECT. These procedures can alter data which you may wish to be part of a transaction. Also, sometimes you don't want the first query in a transaction to be an update/delete query. Sometimes you want to do this: begin; select record; update record; commit; With the way that DB::pgsql works it will instead be: select record; begin; update record; commit; Which may cause errant behaviour. I have removed the "&& $ismanip" portion of pgsql.php line 175 in my copy of DB::pgsql and I'm seeing the behaviour I expect and I believe it to be the correct behaviour. Thought it does have an impact of how the DB API can be used when autocomit is defaulted to false. Personally I think the whole "autocommit" idea (stolen from Perl's DBI) is flawed. I think there should be explicit beginWork(), commit(), rollback() calls. Calls made outside of a beginWork/commit/rollback block should always be commited imediately. Reproduce code: --------------- // Not a valid test case. Just an example. $dbh = DB::connect(...); $dbh->autoCommit(false); $res = $dbh->simpleQuery("SELECT fn_some_stored_procedure(1, 2, 3)"); if(DB::isError($res)){ $errors = true; } $res = $dbh->simpleQuery("SELECT fn_some_other_stored_procedure(4, 5, 6)"); if(DB::isError($res)){ $errors = true; } if($errors){ $dbh->rollback(); } else { $dbh->commit(); } $dbh->autoCommit(true); Expected result: ---------------- That the store procedure calls are is contained within a single transaction. Actual result: -------------- They are not.

I absolutely agree with oliver at deeper dot co dot nz. You should remove the "&& $ismanip" check, 'cos my volatile function was commited after execution, thanks to DB::isManip(), which gives wrong answer checking whether query is a manipulation one or not. And why it is not possible to make SELECTs inside a transaction?

I am also not sure what benefit this $this->transaction_opcount hackery brings. However the use of isManip is founded in the fact that ext/pgsql simply does not give a clear indication of what type of result it produced. This in turn makes it impossible to determine if the code needs to handle a result set or fetch the number of affected rows. This is a fundamental issue I dont know how to work around.

This bug is related, but not a duplicate, to bug #4856

There is no reason to completely disable isManip. The debate on auto-commit aside, the real issue is in the regular expression that detects whether or not a query is a data manipulation query. Take the following stored proceedure in Postgres for example: SELECT * FROM add_update_value('the system may be rotated and locked into place'); The above is in fact a query that updates data, but the stored proceedure returns a value (in this case, the id for the record that it added), hence causing the following error: Fatal error: Call to a member function on a non-object in /usr/local/share/pear/DB/common.php on line 1242 This query however, is not an update query, and wouldn't cause an error since it doesn't return anything: SELECT * FROM fuzzy_search('the system may be rotated and locked into place'); So, in summary- due to stored proceedures, you can't assume whether or not a block of SQL will return a result based on whether or not it contains SELECT .* INTO. I think the best solution to the issue (auto-commit discussion aside) is to not check for SELECT .* INTO. The only other solution would be hinting for isManip, or allow it to be configured (regex string?). My vote would be an interm fix to remove SELECT .* INTO with a further discussion if necessary on configuration vs. hints

Well I think the only viable thing to do this late in the game is to add a method to explicitly hint if the next query is a manip query or not. If a value is set this way it will be used in the next query and it is then reset. We cannot really mess with the API now or with the isManip regexp.

Other related bugs #6131, #4467.

Changing the bug summary from "SELECT statements should be contained within a transaction."

Another related bug #6414

Another related bug: isManip doesn't include PREPARE or EXECUTE as a mainp statement. In postgres you can go: PREPARE stmt AS INSERT INTO table (field) VALUES ($1); EXECUTE stmt(3); EXECUTE stmt(6); DEALLOCATE stmt; I added PREPARE and EXECUTE to the manip regex bug I also feel that autoCommit(false) and commit/rollback are perhaps not the best way to do this :). Of course, you can't change it now, but perhaps there will be a PEAR::DB2 one day...

$query = "/* teste */ insert /* TESTE */ into table VALUES ('value')"\")"; $query = preg_replace(array('%/\*.*\*/%s', '%(?<=\s|^)--.*%'), '', $query); echo $query; /* * * into table VALUES ('value')" * */ $query = preg_replace(array('%/\*.*?\*/%s', '%(?<=\s|^)--.*%'), '', $query); echo $query; /* * * insert into table VALUES ('value')" * */

Another related bug: http://pear.php.net/bugs/bug.php?id=8561

OK, the plan of attack at this stage is to implement a nextQueryIsManip() method, per Lukas's suggestion.

This bug has been fixed in CVS. If this was a documentation problem, the fix will appear on pear.php.net by the end of next Sunday (CET). If this was a problem with the pear.php.net website, the change should be live shortly. Otherwise, the fix will appear in the package's next release. Thank you for the report and for helping us make PEAR better.