There is a workaround in place in the sqlite database driver which is supposed to fix an issue for sqlite reporting matched rows instead of affected rows for an update-query (also discussed in #805858: Affected rows inconsistent across database engines). However this workaround introduces problems in the situation when only the key (e.g. machine_name, tid, nid) of a row should be updated but all the other values remain the same. In this situation an update to the record will be suppressed.
It is easy to reproduce this problem by editing the machine_name of a content_type. If only the machine_name is changed, the change will not be written to the db. If however another field like the human readable name is changed during the same edit, the record gets written to the db properly.
The relevant code resides in includes/database/sqlite/query.inc:52 and is introduced by the following comment:
/**
* SQLite specific implementation of UpdateQuery.
*
* SQLite counts all the rows that match the conditions as modified, even if they
* will not be affected by the query. We workaround this by ensuring that
* we don't select those rows.
*
* A query like this one:
* UPDATE test SET name = 'newname' WHERE tid = 1
* will become:
* UPDATE test SET name = 'newname' WHERE tid = 1 AND name <> 'newname'
*/
class UpdateQuery_sqlite extends UpdateQuery {
[...]
Would it be sufficient to rewrite the code such that all fields get a name<>'newvalue' clause, even those which also appear in the condition? Imagine we'd like to update tid from 1 to 2 but leave name unchanched, in my opinion the query would have to translate to:
UPDATE test SET tid = 2, name = 'oldvalue' WHERE tid = 1 AND tid <> 2 AND name <> 'oldvalue'
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedHi there, and thanks for your report.
The current logic will only add additional conditions to the query if the updated fields are not already in the condition. I don't remember why I did this in the first place, and it indeed looks very wrong.
I'm currently run the test suite the fix sandbox/damz/1185354.git@31c6e2f.
Comment #2
znerol CreditAttribution: znerol commentedAttached is a patch which tests for the misbehaviour. The test succeeds with your fix from the sandbox and fails without it.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis causes a lot of breakage everywhere, I'm not completely sure why. See the results.
Comment #5
znerol CreditAttribution: znerol commentedThat's strange, I was not able to reproduce those test results on my local machine. Neither with your branch nor with vanilla 7.x. Also I was not able to locate changeset 412ef3d4e6687c712c470d53f4be175a42d03d3d which apparently is checked out on the Jenkins server. Where does the code come from on the continous integration platform?
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedIndeed, it seems that it was just an instability due to the test server being overloaded. This patch passed since then.
Comment #7
znerol CreditAttribution: znerol commentedIMHO this patch should be committed soon because it fixes a problem which can cause inconsistencies in the database. As an example consider the case I've mentioned in the original post. When the machine_name of a content type is changed, attached field instances will be updated to point to the new bundle. However because the type column in the node_type table is not updated due to the problem I've pointed out above, the content type will loose all attached fields.
Comment #8
znerol CreditAttribution: znerol commentedUpdate patch and test case for drupal 8.x.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's get this in.
Comment #10
znerol CreditAttribution: znerol commentedStraight backport for d7.
Comment #11
catchOK the fix looks good but please remove the link back to this issue, git blame works fine.
Comment #12
znerol CreditAttribution: znerol commentedThe patch does not apply anymore due to #1542186: PHP 5.4 "Illegal string offset" warning when install. Over there a fix for
removeFieldsInCondition
was introduced while I propose to drop this method entirely. What is the right procedure to continue here. IMHO it would be cleaner to revert the changes introduced by the other fix. But I also may just reroll my patch on top of them.Comment #13
droplet CreditAttribution: droplet commentedNovice: reroll patch
Comment #14
znerol CreditAttribution: znerol commentedFrom #1542186-29: PHP 5.4 "Illegal string offset" warning when install
Before anyone fires up his editor I'd like some comments from one or two core developrs/commiters whether the solution proposed in #8 (d8) and #10 (d7) is acceptible.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis method is completely unnecessary, it should just be removed. Let's move forward with this.
Comment #17
znerol CreditAttribution: znerol commentedRerolled patch on top of current 8.x. Also removed the issue url from the comment in the test-case as suggested by catch (#11)
Comment #18
znerol CreditAttribution: znerol commentedSame for 7.x
Comment #19
dcrocks CreditAttribution: dcrocks commentedI've been getting the following message for the last 2 weeks running recent 7.x dev versions, including today's latest:
Warning: Illegal offset type in unset in UpdateQuery_sqlite->removeFieldsInCondition() (line 80 of /Users/rocks/Sites/drudev/includes/database/sqlite/query.inc).
I haven't tried this fix yet as it isn't quite convenient for me but my question is 'is my database borked'?
Comment #20
znerol CreditAttribution: znerol commentedRerolled after #1542186: PHP 5.4 "Illegal string offset" warning when install has been rolled back.
Comment #21
fenstratI can confirm that the D7 version in #20 fixes errors with PHP 5.4 and drush quick-drupal #1547526: drush qd doesn't work with PHP 5.4.
Comment #22
dcrocks CreditAttribution: dcrocks commentedI can't install drupal 7 dev on a system with php 5.4 without getting this error. Drupal 8 dev installs ok.
Comment #23
czigor CreditAttribution: czigor commentedI could not install drupal7 with php 5.4 and a tmpfs+sqlite3 db. It was trying to write to the watchdog table when it did not exist yet.
Applying the patch in #20 solved it for me.
Comment #24
dcrocks CreditAttribution: dcrocks commentedBumping this because I cannot run drupal v7 on a php 5.4 system. The aforementioned patch #1542186: PHP 5.4 "Illegal string offset" warning when install was apparently only rolled back on drupal 7. On my rhel 6.3 system running php 5.4 drupal 8 works but drupal 7 doesn't. Drupal 7 works after applying the patch in #20.
As far as I can tell there is no published note prohibiting running drupal 7 with php 5.4 and I don't want to work on a patched system. This issue has been open for more that a year. If a better patch than # 20 is needed, please someone provide one soon. Therefore I think it appropriate to bump this. Fill free to correct me.
Comment #25
fenstrat#20: 1266572-20-workaround-in-sqlite-some-update-missed.patch queued for re-testing.
Comment #26
fenstrat#20 is still green. Given recent feedback I'm going to tentatively mark this as RTBC.
Comment #27
webchickBreaking SQLite isn't a critical, but it does qualify as major.
Committed and pushed #20 to 8.x, since the only thing catch seemed to find in #11 was fixed.
Moving down to 7.x. We'll need the patch in #20 re-uploaded without the do-not-test.patch suffix to make sure that testbot still passes (I would just do that but don't want to get false commit credit).
Comment #28
znerol CreditAttribution: znerol commentedThanks webchick. Attached patch from #20 with do-not-test suffix removed.
Comment #29
fenstratLooks good, tests are passing, back to RTBC for D7.
Comment #30
Heine CreditAttribution: Heine commented> Breaking SQLite isn't a critical, but it does qualify as major.
If SQLite isn't guaranteed to work from one minor to another lets just drag the driver out to the fields to shoot it in the back of the head (aka "move to contrib") and be done with it.
Comment #31
dcrocks CreditAttribution: dcrocks commentedI would just comment that from my viewpoint it is Drupal that is broken, whatever sub-component actually fails. Can we please get this in?
Comment #32
joshf CreditAttribution: joshf commentedComment #33
znerol CreditAttribution: znerol commentedI guess this was an accident.
Comment #34
joshf CreditAttribution: joshf commentedBackported to D7; patch fixed the issue for me and I was able to install without problems.
Comment #35
joshf CreditAttribution: joshf commentedComment #36
znerol CreditAttribution: znerol commented@joshf: Diffing #34 against #28 does not show any code-changes. What is the reason for reuploading the patch and demoting the issue from RTBC to needs review?
Comment #37
dcrocks CreditAttribution: dcrocks commentedWhen is anyone going to process the the 7.x RTBC queue? Issues have been waiting more than a month.
Comment #38
joshf CreditAttribution: joshf commented@znerol Nothing, I guess. Sorry, I was following the instructions at http://drupal.org/node/1538380 for the first time when I encountered this issue. I didn't realize that rdbc meant it didn't need to be backported. I thought it needed a separate patch based on 7.x and that I was helping it along. I'll be more careful next time.
Comment #39
znerol CreditAttribution: znerol commented@joshf: No problem. As far as I understand the status-docs you need to look for issues where status is "patch (to be ported)". Thanks for your contribution anyway. I think it is a very good sign that both of us came up with the exact same patch :)
Comment #40
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/bfaa7b4. I also removed t() from the test assertion messages on commit (since that's recently been fixed throughout the database tests). Someone can forward-port that cleanup to D8 if they want to.
@dcrocks, Drupal 7 point releases only happen on a defined schedule, so why does it matter if the RTBC queue builds up a bit in the interim?
Comment #41
dcrocks CreditAttribution: dcrocks commentedI was under the impression things were committed in between point releases. I don't like working with patched code, but I'm ok with dev. Thanks for putting this in.
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, I do usually try not to bunch them all at the end, but sometimes it happens that way :) The reason I don't think it matters much is that the point releases are the only things that are "officially" supported, so we just need to make sure those are as solid as possible when they come out.
(And for what it's worth, I actually believe it's preferable to run a point release with a couple well-tested patches applied, rather than a dev version. In a dev version you are running a much larger amount of unsupported code. Often it won't matter - and hopefully it never does - but if a security issue or upgrade path problem comes up that only affects dev versions, you could find yourself out of luck without a fix.)
Comment #43
dcrocks CreditAttribution: dcrocks commentedI appreciate all your comments. Of course I only use dev for dev and have shot myself in the foot a couple of times. I realize that in those cases the burden is on me but I just feel that I want to encounter something that might break what I'm doing before a point release and its possible production implementation.
Again, thanx for your work.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commentedFixing tags since Drupal 7.21 only contained a fix to deal with fallout from the Drupal 7.20 security release. Hopefully this is really the last time I do this and this will be released in Drupal 7.22 for real :)
Comment #45.0
(not verified) CreditAttribution: commentedChange