In includes/database, for both MySQL and SQLite implementations of TruncateQuery, we see overrides of TRUNCATE resembling this:
return $comments . 'DELETE FROM {' . $this->connection->escapeTable($this->table) . '}';
But for the PostgreSQL implementation, there is no override of TruncateQuery, with normal object inheritance from includes/database/query.inc for the PostgreSQL database:
return $comments . 'TRUNCATE {' . $this->connection->escapeTable($this->table) . '} ';
In PostgreSQL, TRUNCATE holds an exclusive access lock on the table until the transaction commits. This can cause problems where db_truncate is called, such as in _menu_router_save, when one clears the cache, for example. DELETE FROM queries don't hold exclusive access locks, and while they don't immediately reclaim the disk space like TRUNCATE would, autovacuum is enabled by default in PostgreSQL, and has been for 5+ years.
Access Exclusive locks are the most Draconian of table locks, thwarting SELECTs and all other queries while in the transaction block. On a high traffic site, this can mean timeouts, white screen, and all sorts of bad times. Is there a method to the madness, or can all TruncateQuerys' be overridden/replaced by DELETE FROM's instead? Drupal is already doing this for the other supported backends, so why not PostgreSQL?
Comment | File | Size | Author |
---|---|---|---|
#15 | 1839998-use-delete-in-transactions-15.patch | 4.33 KB | dcam |
#12 | 1839998-use-delete-in-transactions-12.patch | 2.61 KB | wiifm |
#8 | 1839998-use-delete-in-transactions.patch | 2.48 KB | Josh Waihi |
#6 | 1839998-postgresql-truncate-replace-D8.patch | 1.03 KB | wiifm |
#6 | 1839998-postgresql-truncate-replace-D7.patch | 959 bytes | wiifm |
Comments
Comment #0.0
mcm.guaba CreditAttribution: mcm.guaba commentedtypo
Comment #0.1
mcm.guaba CreditAttribution: mcm.guaba commentedautovacuum link
Comment #0.2
mcm.guaba CreditAttribution: mcm.guaba commentedmore typos
Comment #0.3
mcm.guaba CreditAttribution: mcm.guaba commentedlink for access exclusive locks
Comment #1
webchickThis sounds like a bug, rather than a feature.
Does this problem exist in D8 as well? If so, we should fix it there first, and then backport.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, so we should do the same thing we do for MySQL: use DELETE if we are in a transaction, TRUNCATE otherwise.
Comment #3
mcm.guaba CreditAttribution: mcm.guaba commentedYes, that is how MySQL TRUNCATE in implemented, in D7 and D8.
Comment #4
mcm.guaba CreditAttribution: mcm.guaba commentedle patch (D8)
Comment #5
webchickThanks! Marking needs review.
Comment #6
wiifmHave just experienced this with PostgreSQL and entitycache (with D7), from the logs:
This caused a integrity issue between the node table and the node_reivision table, leaving the content inaccessible to end users. Obviously less than ideal.
Have reviewed the patch in #4 and have made comment only changes so it matches what I wrote for the D7 patch (which in turn was copied from the MySQL truncate command).
Happy to mark this as RTBC for the D8 patch, wondering if someone can also review the D7 patch for me though?
Comment #8
Josh Waihi CreditAttribution: Josh Waihi commentedIt makes sense to me that we use the MySQL implementation as the global default. As SQLite implements its own version of Truncate, all core databases will work fine like this.
Comment #9
acbramley CreditAttribution: acbramley commentedHad the same issue @wiifm explains, marked #1865238: PDO Deadlock causing node table vid inconsistencies as duplicate of this
Comment #10
wiifmHappy with the patch in #8 marking as RTBC
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's update the comment. The problem on PostgreSQL is not that the DDL statement is transaction unsafe, it's that it locks the whole table, strongly reducing the concurrency with other transactions.
Comment #12
wiifmUpdated comment in a new patch attached. Hope this is clear now.
Comment #13
Josh Waihi CreditAttribution: Josh Waihi commentedThat makes sense to me.
Comment #14
catchMe too. Committed/pushed to 8.x.
Looks like this needs 7.x backport.
Comment #15
dcam CreditAttribution: dcam commentedBackported #12 to D7.
Comment #16
wiifmPatch looks good, there is some extra whitespace removals, but I think this is a good thing (and no doubt simply done by your editor automatically when saving the file).
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/2df0b84
Comment #18.0
(not verified) CreditAttribution: commentedbad times