| Package home | Report new bug | New search | Development Roadmap | Status: Open | Feedback | All | Closed Since Version 1.12.3 |
| Bug #3895 | DB::isManip issues | ||
|---|---|---|---|
| Submitted: | 2005-03-21 12:01 UTC | ||
| From: | oliver at deeper dot co dot nz | Assigned: | aharvey |
| Status: | Closed | Package: | DB |
| PHP Version: | 4.3.10 | OS: | Linux |
| Roadmaps: | (Not assigned) | ||
[2005-03-21 12:01 UTC] oliver at deeper dot co dot nz
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.
Comments
[2005-10-11 17:34 UTC] daddy_cool at inbox dot lv
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?
[2005-10-11 17:41 UTC] User who submitted this comment has not confirmed identity
If you submitted this note, check your email.If you do not have a message, click here to re-send MANUAL CONFIRMATION IS NOT POSSIBLE. Write a message to pear-dev@lists.php.net to request the confirmation link. All bugs/comments/patches associated with this email address will be deleted within 48 hours if the account request is not confirmed!
[2005-10-13 09:01 UTC] User who submitted this comment has not confirmed identity
If you submitted this note, check your email.If you do not have a message, click here to re-send MANUAL CONFIRMATION IS NOT POSSIBLE. Write a message to pear-dev@lists.php.net to request the confirmation link. All bugs/comments/patches associated with this email address will be deleted within 48 hours if the account request is not confirmed!
[2005-12-06 23:16 UTC] bajordan at hubbell-ltg dot com
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
[2005-12-09 10:39 UTC] User who submitted this comment has not confirmed identity
If you submitted this note, check your email.If you do not have a message, click here to re-send MANUAL CONFIRMATION IS NOT POSSIBLE. Write a message to pear-dev@lists.php.net to request the confirmation link. All bugs/comments/patches associated with this email address will be deleted within 48 hours if the account request is not confirmed!
[2006-03-11 10:10 UTC] User who submitted this comment has not confirmed identity
If you submitted this note, check your email.If you do not have a message, click here to re-send MANUAL CONFIRMATION IS NOT POSSIBLE. Write a message to pear-dev@lists.php.net to request the confirmation link. All bugs/comments/patches associated with this email address will be deleted within 48 hours if the account request is not confirmed!
[2006-03-11 10:12 UTC] User who submitted this comment has not confirmed identity
If you submitted this note, check your email.If you do not have a message, click here to re-send MANUAL CONFIRMATION IS NOT POSSIBLE. Write a message to pear-dev@lists.php.net to request the confirmation link. All bugs/comments/patches associated with this email address will be deleted within 48 hours if the account request is not confirmed!
[2006-03-11 10:13 UTC] User who submitted this comment has not confirmed identity
If you submitted this note, check your email.If you do not have a message, click here to re-send MANUAL CONFIRMATION IS NOT POSSIBLE. Write a message to pear-dev@lists.php.net to request the confirmation link. All bugs/comments/patches associated with this email address will be deleted within 48 hours if the account request is not confirmed!
[2006-04-26 02:44 UTC] nigel at catalyst dot net dot nz
(Nigel McNie)
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...
[2006-05-30 17:34 UTC] gnome_gtk2000 at yahoo dot com dot br
(Evandro Veloso Gomes)
$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')"
*
*/
[2006-08-25 19:15 UTC] User who submitted this comment has not confirmed identity
If you submitted this note, check your email.If you do not have a message, click here to re-send MANUAL CONFIRMATION IS NOT POSSIBLE. Write a message to pear-dev@lists.php.net to request the confirmation link. All bugs/comments/patches associated with this email address will be deleted within 48 hours if the account request is not confirmed!
[2007-01-11 07:47 UTC] User who submitted this comment has not confirmed identity
If you submitted this note, check your email.If you do not have a message, click here to re-send MANUAL CONFIRMATION IS NOT POSSIBLE. Write a message to pear-dev@lists.php.net to request the confirmation link. All bugs/comments/patches associated with this email address will be deleted within 48 hours if the account request is not confirmed!
[2007-01-12 02:41 UTC] User who submitted this comment has not confirmed identity
If you submitted this note, check your email.If you do not have a message, click here to re-send MANUAL CONFIRMATION IS NOT POSSIBLE. Write a message to pear-dev@lists.php.net to request the confirmation link. All bugs/comments/patches associated with this email address will be deleted within 48 hours if the account request is not confirmed!