Posted by andypost on December 20, 2011 at 1:25am
3 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | database system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | API clean-up |
Issue Summary
There's a whole bunch of bugs caused by 'duplicate key' errors http://goo.gl/f9YdV
Coming from #505812: PDO uses inconsistant error codes between DBs
There's a 2 approaches in core introduced in #364348: OpenID throws an error when two accounts try to use the same OpenID
1) check existence by key in form validation level - most of core uses this
2) try...catch with exception handling in API level (_save)
First one could lead to some drift in multi-db config (master-slave replication)
Second has no consistency in constraint violation detection
I think we should use second approach and unify this all over core
Comments
#1
True. Even in a single-db setup it would not be bullet-proof unless access to the table was restricted by semaphores.
I agree. I suggest we create a separate exception class for all 23xxx Integrity Constraint Violation errors.
http://www.postgresql.org/docs/8.1/static/errcodes-appendix.html
#2
How about this?
#3
The last submitted patch, integrity-constraint-violation-1.patch, failed testing.
#4
I couldn't reproduce all the test errors reported by the test bot (it complained about fatal errors in line numbers that were containts the initial "function testFoo()" method declaration). But I fixed one bug in lock.inc that might have caused problems elsewhere.
#5
Looks good, are you sure that all places are fixed?
#6
I looked through the code (especially places where we catch PDOExceptions) but couldn't find any other places where we should catch the new exception (I wonder why our own database exceptions don't inherit from PDOException? Then changes like this would not break existing code - though of course we should still use the more specific exception class when appropriate).
This updated patch contains an update to the pgsql driver also. This has been tested on Postgres 8.4 (I ran all database tests — two tests were failing with and without the patch). I had to patch core in two places in order to even install Drupal on Postgres?! This is not included in the patch (I assume this problem is being tracked elsewhere, though I couldn't find the ticket).
The patch was also tested successfully with the sqlite driver (I ran all database tests — one test were failing with and without the patch). I fixed a test failure (database_test line 3186, “The whole transaction is rolled back when a duplicate key insert occurs.”) by adding a missing "use Exception" to core/lib/Drupal/Core/Database/Query/Insert.php.
I'm a bit surprised about the problems with the pgsql and sqlite drivers. Don't we have test bots running tests on with engines?
#7
I created a separate issue for the Postgres install troubles: #1491526: Cannot install Drupal on PostgreSQL
#8
Reroll (needed because of #1477446: Move lock backend to PSR-0 code).