Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
During a call to a hook_update_N() function that is walking over a large number of nodes and updating each with a final call to node_save() we experienced the following error;
sample_config module :
7094 - Enable colorbox load to allow links to be opened in a colorbox.
7095 - Update the year with new format
Do you wish to run all pending updates? (y/n): y
Processing sample_config_update_7094 [success]
Processing sample_config_update_7095 [success]
Reverting all features. [success]
Gathering customer_pricing_phase nodes. [success]
Processing 1 customer_pricing_phase nodes. [success]
Processed 1 of 166: Unison Generators - Confirm assets ASD2015/16 [success]
Processed 2 of 166: Wessampleer - Confirm assets ASD2015/16 [success]
WD node: PDOException: SQLSTATE[25P02]: In failed sql transaction: 7 ERROR: current transaction is aborted, commands ignored until end of [error]
transaction block: UPDATE url_alias SET source=:db_update_placeholder_0, alias=:db_update_placeholder_1, language=:db_update_placeholder_2
WHERE (pid = :db_condition_placeholder_0) ; Array
(
[target] => default
[return] => 2
[already_prepared] => 1
)
in drupal_write_record() (line 7261 of /path/to/drupal/includes/common.inc).
SQLSTATE[25P02]: In failed sql transaction: 7 ERROR: current transaction is aborted, commands ignored until end of transaction block [error]
The node that the error occurs on is random and occasionally we would also have a successful run in our development environments. However, the staging and production environments always had the error at some point.
This was tracked down to path_save() and, more specifically, the use of drupal_write_record() within it. The first thing that drupal_write_record() says is;
Do not use drupal_write_record() within hook_update_N() functions, since the database schema cannot be relied upon when a user is running a series of updates. Instead, use db_insert() or db_update() to save the record.
Proposed resolution
The proposed resolution is to follow the advice in the docs for drupal_write_record() and use db_insert() and db_update() instead.
Since doing this I've not been able to replicate this issue at all.
Remaining tasks
- Code review
- Community testing
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | drupal-path-save-sans-dwr-2459719-13.patch | 1.25 KB | Gold |
| #6 | drupal-path-save-sans-dwr-2459719-6.patch | 1.14 KB | Gold |
| #3 | drupal-path-save-sans-dwr-2459719-3.patch | 1.45 KB | Gold |
| #1 | drupal-path-save-sans-dwr-2459719-1.patch | 1.12 KB | Gold |
Comments
Comment #1
GoldI may have jumped the gun on this one. Second test run on staging still failed. I'm thinking this wasn't the actual source of the issue.
However, the reason for using this patch still stands. Based on what drupal_write_record() recommends and with path_save being used a lot in update hooks this change should probably still stand. I'm going to dig into db_update and db_insert a little and see if they experience the same issues as dwr. If so, there may be a second patch shortly using db_query and raw sql.
Comment #2
wiifmQuick review:
Make this into a hyperlink so it renders correctly in api.drupal.org
Do not remove this line
Do not worry about aligning the '=>', this is not required (and creates hard to maintain code)
Again, this should not be removed
The third argument to condition() defaults to '=', I would remove
Comment #3
GoldIt's been a long painful day.
Comment #4
GoldComment #5
wiifmHey, that is looking good, small nitpicks:
Unsure of whether this is needed in the parent docblock, as this function is no longer used inside it correct?
I think a single space is what is needed here. Also s/recommend/recommends/
Comment #6
GoldI was wondering about the @see thing.
Removed that and fixed the comment too.
Comment #7
GoldComment #8
wiifmLooks good to me, keen to have more eyes from someone in core to look at this. Also keen to see what the test bot says here.
Comment #9
Gold@wiifm, thanks for the critique.
Comment #13
Goldrunning the tests on SimplyTest.me suggests it's something in the update hooks. @wiifm pointed out that drupal_write_record would populate the pid on an insert.
Let's try that.
Comment #14
GoldReally need to get into the habit up updating this on patch submission...
Comment #15
wiifmHey patch passes all the test, seems to fix an important bug. Marking RTBC to get a core maintainer to review.
Comment #16
David_Rothstein CreditAttribution: David_Rothstein commentedI don't think we should change this. path_save(), like most API functions, is too high-level to recommend calling it from update functions at all. In addition to the drupal_write_record(), it also invokes hooks, etc.
I think if you're running into issues with it you should use low-level functions only, like direct db_insert() or db_update() queries... right?
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedWell I see the code comments refer to this being called indirectly via node_save(). I would assume calling node_save() from a generic update function has all sorts of other problems too...
It's fine in a site-specific update function, I think, but in generic code (e.g. contrib modules) it's probably not the best idea.
That said, "not recommended" is not the same as "causes PDOExceptions". I would really expect that if you're seeing PDOExceptions here, the bug is caused by something else.