Errors inside db_lock_table()'s transaction are not logged, following queries are ignored.

I am wondering why do we need in this function a 'LOCK TABLE {%s} IN EXCLUSIVE MODE' when we start the transaction. I can not see an example of a situation where we need to define this.
-------------------------------------------
function db_lock_table($table) {
db_query('BEGIN; LOCK TABLE {%s} IN EXCLUSIVE MODE', $table);
}
-------------------------------------------

From the postgresql documentation:
'...... most PostgreSQL commands automatically acquire locks of appropriate modes to ensure that referenced tables are not dropped or modified in incompatible ways while the command executes. ......".

I think function db_unlock_tables() should be modified. A suggestion would be something like this:

function db_unlock_tables($code) {
  if($code == '0')  
    db_query('COMMIT');
  elseif($code == '1')
    db_query('ROLLBACK');
}

The code you are talking about should also be modified, for example like this:

  db_lock_table('cache');

  if (!db_query("UPDATE {cache} SET dat_a = %b, created = %d, expire = %d, headers = '%s' WHERE cid = '%s'", $data, time(), $expire, $headers, $cid)){
     db_unlock_tables('1');
  }
  else{   
     if (!db_query("INSERT INTO {cache} (cid, data, created, expire, headers) VALUES ('%s', %b, %d, %d, '%s')", $cid, $data, time(), $expire, $headers)){
      db_unlock_tables('1');
     else{
     db_unlock_tables('0');
     }
  }

I have not checked this code. This is a suggestion, but I think you get the idea. The same should be done in variable_set()

More, more, ;-)

I think 'db_lock_table' and 'db_unlock_tables' are not good names in the postgresql version. We do not lock the tables, like in mysql, we start a transaction and this is quite different.

Shouldn't they be called, for example, 'db_start_transaction' and 'db_finnish_transaction' or something like this?

The functions are called lock/unlock() because their main purpose is to lock tables. For mysql they only LOCK. For postgresql, you need to have a transaction. So the transaction is only a byproduct of table locking.

I think it is the other way around, locks are a byproduct of transactions. Transactions are there because postgresql is fully ACID compliance and changes in the database are Atomic, Consistent, Isolated and Durable, not because it is needed by a lock. Transactions in postgresql use different types of locks to make its work. Anyway, this was just a suggestion, because I think the function name was better for the mysql world than postgresql.

I believe that if you use Innodb tables instead of Myisam with mysql, you do not need to lock the table either because Innodb uses also transactions, it uses row-level locking instead of MVCC.

I believe that LOCK is needed to prevent two transactions trying to INSERT the same data. MVCC won't help with this.

MVCC will work out what to do with two transactions that want to modify the same data and running an EXCLUSIVE lock does not help in this situation. An EXCLUSIVE lock in a table will make things work ****much more slower with many users**** because only one process can update the table at the same time. This is one of the big problems with mysql/myisam tables that postgresql does not have with MVCC. If we used an EXCLUSIVE lock on a table with postgresql is like using mysql with myisam tables, many of the benefits of MVCC disappear.

Any UPDATE, DELETE, and INSERT command will adquire automatically a ROW EXCLUSIVE lock on the rows that are going to be changed to avoid problems with transactions trying to modify the same data, but other transactions can modify other data on the table without problems, so reading never blocks writing and writing never blocks reading or writing as long as they do not modify the same row (not table).

Maybe (I do not know without reading all the code) we need to use SELECT FOR UPDATE in some places to ensure the current validity of a row and protect it against concurrent updates but I am sure that we do not need EXCLUSIVE locks in our case.

About proposed changed to cache_set():
db_query() returns FALSE in case of failed (bad) query, and "query result" otherwise. It doesn't say if it modified (UPDATEd) anything.
And it still does not solve problems with failed queries inside of transaction block.

Ok, I misinterpret this, we run the insert only if the update modified anything. I thought that the insert only would be done if the update did not fail.

BTW: I've written before that "we could COMMIT in case of errors". That would not change anything, if query is bad (produces errors) then all transaction is automatically rolled back.

Ok, it sounds good, mea culpa, I can see that it is not necessary to run a rollback because the commit will run an automatic rollback if there are any errors in the transaction.

BTW: Another thing we should start thinking about is to run any insert/update and delete in the code inside a transaction to guarantee ACID compliance.

regards
Ralfm

.... you'll get error in the INSERT2 because INSERT1 already inserted 'foo'. INSERT1 will lock the table (or not, does not matter), INSERT2 will wait or not, but then it'll try to insert the same value again.

This is like it supposes to work when using transactions, if they are trying to insert the same data, the second transaction will get a rollback because it fails but the table is updated with the right data by the first transaction.

I do not see the problem with this, the data is still consistent and many users can update the table at the same time (huge different in speed in busy systems).

chears
Ralfm

Hello again :)

I had time to put together a test case. The code used is this and the results are here: http://folk.uio.no/rafael/drupal_pg_locks.html
$num = rand(1, 2000); and $sql = sprintf("BEGIN; "); are changed in the different tests.

<?php
$conn = pg_pconnect("dbname=test user=postgres host=localhost");

if ($conn) {
  $num = rand(1, 2000);
  
  $sql = sprintf("BEGIN; ");
  $result = pg_query($conn,$sql);
  
  $sql = sprintf("UPDATE test01 SET id2 = '%s' where id = '%s';", $num+1000, $num);
  $result = pg_query($conn,$sql);
  
  if (!pg_affected_rows($result)){
    $sql = sprintf("INSERT INTO test01 VALUES('%s','%s','%s');",$num,$num+100,$num+200);
    $result = pg_query($conn,$sql);
  }
  
  $sql = sprintf("COMMIT;");
  $result = pg_query($conn,$sql);
  
}
?>

test=# \d test01 
         Table "public.test01"
 Column |  Type   |     Modifiers      
--------+---------+--------------------
 id     | integer | not null
 id2    | integer | not null
 id3    | integer | not null
Indexes:
    "test01_pkey" PRIMARY KEY, btree (id)

I tested with 50 and 2000 rows tables and with 10 and 120 concurrent clients.

There would be errors in logs (I'm quite sure, I have to check this). You would have to differentiate somehow between errors caused by duplicated values and other errors. I think it's possibile, not sure how. Would all duplicate-value errors be the result of a concurrency?

In the result page you have the results of how many errors I get in every test. And yes, I think all duplicate-value errors will be the result of a concurrency.

How big would be the difference in speed? Do you have some measurements?

Now we have measurements. We get the best results without using locking when concurrency is high and the number of rows is also high

Would it be possibile to have it work with MySQL without problems?

I do not understand what do you mean with this.

If the speed difference is big it's certainly worthy to look at.

What do you think from the results? ;-)

I will try to put together a test case this week with your suggestions

this issue of when/how to locak tables seems like it should be a priority for those worried about performance.

What about making the cache and variable tables InnoDB for MySQL so the LOCK won't be needed?

Perhaps this would go along with dropping support for MySQL 3.23 as i saw suggested recently?

For Postgres, we should use transactions as much as possible and rollback on failure. For me, data integrity and acid compliant transactions are more important than speed. What good is speed if my database is messed up? It just gets messed up at warp speed. With financial data (ecom), this is important.

So for Postgres, we should not use db_lock_table, but do as ralfm suggested: db_start_transaction & db_end_transaction.

I have (maybe) related problem with this issue. I logged in on my Drupal 5 beta 1 installation as admin. I have tried almost all functions (heavy testing :)) without problems and errors but when i logged out i give many DB warnings which seems related with cache table locking.

I had same problem with Drupal 5 cvs few days ago but if i tried complete new installation from scratch (i have erase database and create new) this problem go on.

Here is site:

http://www.fem.uniag.sk/drupal5/

Through DEVEL module i discover query which make problems -> http://drupal.org/node/92503

moving from x.y.z to 5.x-dev version queue. Looking at latest comments that appears to be the latest version where this is relevant.

This puts postgres-specific code in common.inc:

+      db_query('ROLLBACK'); 

Note that we have the same problem in D6.

The best solution would be to accumulate all the errors in the watchdog() call, then on db_unlock_tables() to save all of those errors. This would require non-existent call backs.

Now, for those questioning the need for the lock, it is necessary in case two transactions (two people accessing the web site simultaneously) occur simultaneously. Only one of them must execute that code. There are many instances in the code that do not use the lock when it should. My question would be opposite: why isn't it checked everywhere?!

I hope that this will be fixed in D7 though.

Thank you.
Alexis

I think it is safe to won't fix this. PostgreSQL is not properly supported by Drupal before Drupal 7, and we will not change a frozen API at this time for such a small user base.