Home » Database » DB » Bug #3895

[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.

[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-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

[2006-04-26 02:44 UTC] nigel at catalyst dot net dot nz

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

$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')"
*
*/