Split from #1998366: [meta] SQLite is broken
according to #84, suggested commit message:
Issue #2057401 by plach, socketwench, kfritsche, Damien Tournoud: Fixed Make the node entity database schema sensible.
Summary
Motto: we are basically taking node data and vomiting it on the hard drive without any consideration as to if the data we are writing is sensible or simply dreamed up by magic pixies. — chx
The database schema introduced by #1498674: Refactor node properties to multilingual has several short-comings, the primary of them being that the concept of "revision" is not materialized as a table in the schema anymore. As a consequence, we don't have no good way to generate the revision identifier (vid
). The current schema works on MySQL (because it is doing some really, really ahem interesting tricks to generate the serial if the primary key is not the serial column by itself), but it would not work on virtually any other database engine.
The following modifications are being proposed:
Re-introduce a {node_revision}
table
It will store fields that are specific to the revision itself and will allocate the vid
.
The following structure:
- Primary key:
(vid)
- Fields:
vid
nid
langcode
log
revision_timestamp
author_uid
Remove {node}.langcode
(as it is moved to {node_revision}
Update the structure of {node_field_data}
table
- Change
{node_field_data}.nid
fromserial
toint
, - Change the primary key from
nid, vid, langcode
tonid, langcode
, - Drop the
{node_field_data}.nid
index that is redundant.
Update the structure of {node_field_revision}
table
- Change
{node_field_revision}.vid
fromserial
toint
, - Change the primary key from
nid, vid, langcode
tovid, langcode
, - Drop the
{node_field_revision}.nid
and{node_field_revision}.vid
indexes.
Remaining tasks
Agree on the planWrite a patch- Benchmarking/profiling
Blocks:
#2015277: Reduce the number of indexes on the node_field_* tables
#2032977: Add views support for the missing columns of node_field_revision
More related issues:
#1498674: Refactor node properties to multilingual
See from #62 to #104 of #1998366: [meta] SQLite is broken for a complete background.
Comment | File | Size | Author |
---|---|---|---|
#83 | et-node-ml2-2057401-83.patch | 73.4 KB | plach |
#78 | et-node-ml2-2057401-78.interdiff.txt | 834 bytes | plach |
#78 | et-node-ml2-2057401-78.patch | 73.4 KB | plach |
#74 | et-node-ml2-2057401-74.interdiff.txt | 1.5 KB | plach |
#74 | et-node-ml2-2057401-74.patch | 73.46 KB | plach |
Comments
Comment #1
chx CreditAttribution: chx commentedtagging.
Comment #1.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #2
chx CreditAttribution: chx commentedWe will need to make this change on an entity storage level, of course. What shall be the name of the table be? As a reminder:
revision_data_table looks logical to me.
Comment #3
plachYep,
revision_data_table
sounds sensible.Comment #3.0
plachUpdated issue summary.
Comment #4
chx CreditAttribution: chx commentedNote that I will not work on this issue.
Comment #5
plachI will as soon as I am done with #2019055: Switch from field-level language fallback to entity-level language fallback.
Comment #6
catchTagging.
Comment #7
plachHere is a first draft.
Comment #9
plachThis one should be better.
Comment #11
plachMore test fixes
Comment #13
plachAnd more
Comment #14
plachComment #16
webchickTagging all critical D8 upgrade path issues as "beta blocker."
Comment #17
plachPlain reroll after #1497374: Switch from Field-based storage to Entity-based storage.
Comment #18
plachAnd now even with patch!
Comment #20
plachSome more test fixes.
Comment #22
plachmoar!
Comment #24
plachThis contains a better fix for some storage controller-related failures. It might intoduce new failures though.
Comment #26
plachRerolled
Comment #28
plachOnly upgrade path stuff should be failing now.
Comment #30
plachAnd this should hopefully fix the upgrade path.
Comment #32
plach#30: et-node-ml2-2057401-30.patch queued for re-testing.
Comment #33
dcrocks CreditAttribution: dcrocks commentedTested patch on a clean install of D8-x.dev. Installed without error. Created a simple article, saved it, then revised it. No errors. Looks good. Can someone review and rtbc?
Comment #34
plachPlain reroll
Comment #34.0
plachUpdated issue summary
Comment #35
andypost+1 here. Overall patch is ready, just small nitpicks.
@plach upgrade path tests passed but I found no creation of new table!?
// Init RevisionData if defined.
$this->revisionDataTable =
!empty($this->entityInfo['revision_data_table']) ? $this->entityInfo['revision_data_table'] : FALSE;
suppose this property is optional
this need to point about usage of the key for enity_info array.
And defaults is 'base_table'
Comment #36
andypostAlso I think that no need to make separate methods that all calls
mapToStorageRecord()
Any reason to all this methods?
Suppose savePropertyData() should care about this "base table" hacks
Comment #37
plachThanks :)
This should address #35-#36.
I think we should do some profiling/benchmarking once we are done with code reviews.
Comment #38
plachRerolled
Comment #39
jibranAdding VDC tag for views related changes and needs profiling as per #37 . Patch looks good to me.
Comment #40
BerdirBecause we don't have enough tags yet.
Comment #41
jthorson CreditAttribution: jthorson commentedNot enough tags? Here's another!
Doesn't apply, thanks to a bunch of updated tests.
Comment #42
socketwench CreditAttribution: socketwench commentedReeeeeeeeeeEEEEROOOOOOOOLLLLLL!
Comment #44
kfritscheNext reroll.
Seems like in the last reroll we had a "merge conflict".
Hopefully this works now.
Comment #45
chx CreditAttribution: chx commentedMoving to Damien for review -- as he is the maintainer of two non-mysql db drivers in core and a third in contrib it would make a lot of sense to get his approval.
Comment #46
BerdirWhy move to separate calls here?
Comment #47
plachRerolled and fixed #46.
Comment #48
plachRerolled again
Comment #49
Damien Tournoud CreditAttribution: Damien Tournoud commentedJust so that this doesn't block on me, +1 on the principal, and on the new schema for the node module.
The patch is really hard to review without actually applying it, so I haven't looked in details, but all the changes seem reasonable from a cursory look.
I'll take a closer look this week as time permit, but please don't block this on me. This really need to get in, if there are details to iron out we can iron them out later.
Comment #50
BerdirOk, went through the patch, didn't review the big picture, but we have Damien's OK now :)
Why the string cast here?
A few inline comments here would really help to understand the flow, e.g. making sure that the entity is still seen as new after adding the id.
Although this is really a bit weird that we have to do it like this...
Wondering if it might be easier to understand the folow if the table_key and table_name were passed directly to the method instead of the magic (and undocumented) $revision property. Then you can more easily see what it's doing when it's called.
And match this one better.
Weird indendation.
This is not a db_update() query why.. ? :)
Should now use \Drupal. Didn't check if there are others, just noticed this one.
We should really have a helper for this. But that's just going to make this patch bigger.
Comment #51
dcrocks CreditAttribution: dcrocks commentedI was able to apply the patch in #48 to a copy of D8 cloned from git without error, and then install it on SQLite, also without error. I created and revised some trivial content without a problem. Looking forward to seeing this go in.
Comment #52
nevergone CreditAttribution: nevergone commentedI install the latest Drupal 8 with #48 path and SQLite 3.7.9 and works well!
Create user, versioned and unversioned nodes without errors.
Thanks! :)
Comment #53
das-peter CreditAttribution: das-peter commentedJust did a visual review and the only addition to #50 I've found is following nitpicky thing. All other things I found were already mentioned by berdir.
Typo:
field date.
I guess that should befield data.
Comment #54
plachAddressed #50 and #53.
@berdir:
I don't remember honestly: it was needed to fix some failing tests. I attached a patch version that reverts that to see which ones.
Comment #55
plachActually executing queries usually helps...
Comment #57
plachAnother try
Comment #58
plachSigh
Comment #60
plachLet's try this
Comment #61
tim.plunkettRemoving tags
Comment #62
BerdirOk, I think this is ready.
We have reports that this installs and works fine on sqlite, the OK from Damien, we had some code reviews. I also did some profiling, where I wasn't able to see a measurable difference.
Also did run this before and after:
Before:
21.323546886444
21.295757055283
After:
21.397483825684
21.107705116272
Comment #63
catchThis looks a lot better. Committed/pushed to 8.x
If we need to make further tweaks (for example #1528028: Add tests for reverting revisions where revision_uid and uid differ) can do those later.
Will need either a new or updated change notice.
Comment #64
BerdirOk, looks like this broke HEAD (PathLanguageTest), possibly because it went in in combination with the translation.module removal?
No idea why yet, investigating. Would be great to get a quick fix and not having to roll back this issue..
Comment #65
BerdirOk, this fails with the following exception:
Sounds like an actual problem and not just a stale test or so.
The test was changed to use entity translation with the removal of translation.module, is it possible that this is now testing a use case that we previously didn't?
Comment #66
tim.plunkettThis was reverted since it broke PathLanguageTest.
Comment #67
plachWeird, the latest rerolls were due to the translation module removal and we had a green patch... I will look into this ASAP
Comment #68
plach#60: et-node-ml2-2057401-60.patch queued for re-testing.
Comment #70
plachI was not able to make tests pass yet, no idea of how the
PathLanguageTest
could be passing before.However I think node forms are buggy wrt revision handling in the current head (a new revision is always created) and this revealed some bugs with multilingual revisions.
Let's see if I am moving in the right direction.
Comment #72
andypostMaybe better to file separate issue for that?
Comment #73
plachSure, I was just suggesting that buggy node forms might be the cause behind the new test failures.
Comment #74
plachThis should fix the last failure.
Comment #75
BerdirNice work.
I'm not sure I fully understand all the changes, can you quickly describe why some of those changes are necessary?
Comment #76
plachThis is needed because untranslatable fields were getting different field objects per language instead of shared ones.
This prevented code following the revision record storage to know we just saved a new revision.
This was broken when
'data_table'
was passed explicitly.Node titles cannot be translated yet, so this test was totally meaningless. Moved it to node bodies which can be translated.
The test is programmatically creating a node but, since field translatability change was not picked up, the body value got the wrong language.
Removed some unneeded cruft.
Comment #77
BerdirOk, thanks for the explanations, makes sense.
Is there a reason we don't just put $table_key = 'data_table' in the function definition now?
Comment #78
plachI should definitely stop working on that method after midnight :D
Comment #79
BerdirThat's a problem then, don't you only work after midnight? ;)
Ok, we fixed the bug that the changed test exposed and improved that test, not sure how it even worked before that. Let's try this again :)
Comment #80
plachComment #82
plach#78: et-node-ml2-2057401-78.patch queued for re-testing.
Comment #83
plachIt seems there's no way to get a result :(
Let's try a reroll...
Comment #84
plachOk, #78 is green and is identical to #83. Back to RTBC.
@committers:
Please credit @Damien as this fix is mainly due to him, I just implemented it.
Comment #84.0
plachUpdated issue summary.
Comment #85
catchCommitted/pushed to 8.x, thanks!
Comment #86
YesCT CreditAttribution: YesCT commentedcritical task according to https://drupal.org/core-gates#documentation (we can change it back to critical bug report after the change notice is done.)
I read the issue summary (nice, btw) and the patch that got committed. I'm confused my some of the namings
data_table, base_table
are they combined, like
base_table.data_table to make something like: revision.data_table?
or node_field_revision?
data == fields ?
What kind of change notice do we want for this? Is this a: previous 8.x to now 8.x kind of change notice, 7 to 8, or should we update a previous change notice.
Maybe updating the change records linked from #1498674: Refactor node properties to multilingual:
https://drupal.org/node/1722906 Added multilingual support to the standard entity schema
or
https://drupal.org/node/2004750 Node properties are made multilingual
Are properties now base fields (revision data)? (I haven't been completely keeping up with the property stuff)
Comment #87
amateescu CreditAttribution: amateescu commentedThose (new) annotation keys could be documented in this issue: #2107317: The 'data_table', 'revision_table' and 'revision_data_table' entity info keys are not documented
Comment #88
catchI've updated https://drupal.org/core-gates to reflect #2083415: [META] Write up all outstanding change notices before release. Sorry for status pong.
Comment #89
plachI updated the change notices cited in #86.
Comment #90
plachI don't understand the question :)
The
base table
,data_table
,revision_table
andrevision_data_table
keys are just the names of the 4 tables implementing the 4 modes we are supporting (basic entity, multilingual entity, revisionable entity, multilingual and revisionable entity). Onlyrevision_data_table
has been introduced here.More or less: data is an abbreviation for field data. The new table layout somewhow mimics field tables, where we have a data table and a revision table.
Again I am not sure I get what you mean: with the introduction of the Entity Field API, we no longer distinguish between fields and properties. Base fields are defined in the entity class, additional fields may be defined by modules, for instance the Field API module. Now all node base fields (including language) have revision support, except for revision metadata, but this was already implemented in #1498674: Refactor node properties to multilingual.
Comment #91
BerdirUpdates look good to me.
Comment #92
Gábor HojtsyTagging.
Comment #93
Gábor HojtsyWhum?
Comment #94
Gábor HojtsyIs there something that should be brought back into https://drupal.org/node/1722906 and the base entity schema suggestions? Since that would also be taken as a best practice, we should consider keeping it up to date to current best practices.
Comment #95
plachI already updated it with all the relevant information :)
https://drupal.org/node/1722906/revisions/view/2710340/2869763
Comment #96
Gábor HojtsyYay, superb, thanks!
Comment #97.0
(not verified) CreditAttribution: commentedadded plach's suggested commit message.