-
-
Notifications
You must be signed in to change notification settings - Fork 224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
!!! TASK: Use integers as lightweight nodeanchorpoints #4791
Conversation
e609c30
to
3a215d0
Compare
3a215d0
to
1b919ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general.
I just raised a question about the need of NodeRelationAnchorPoint::create
. And I wonder how the migration path looks like? I guess replay? If yes, we should document this in the PR description.
Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Projection/NodeRecord.php
Outdated
Show resolved
Hide resolved
Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Projection/NodeRelationAnchorPoint.php
Outdated
Show resolved
Hide resolved
0cfb2d7
to
7e17eee
Compare
In my testings i had a little trouble while migrating the database. I assumed that those are the steps:
but a first (and any directly following subsequent run of cr:setup) will result in an error: Error
then i tried out of fun the Error
I know that the definition insanity is doing the same thing over and over and expecting different results, but as a programmer i had to try - and indeed. Now both commands pass:
but besides this i can confirm that this change works with the neos ui and demo and that the neos ui e2e tests are working locally! |
Right, mmm the migration, not too much we can do about it, the values obviously do not fit at all in the new schema if you have entries already.... |
yes but i was confused that this order of commands results in a migrated state:
if we are okay with this, as there are way more breaking changes already ahead - like the workspace thing - which absolutely crushes an installation we can just move forward and i +1. |
I guess one just tries and fails to update the schema, the replay clears the tables and errors out on the first write due to wrong schema, then setup works because no data, and then replay works. |
hum im stupid this sound 100% logical. Thanks for letting me use your brain 😂 |
but doesnt this scream for a |
would be neat for sure, but IDK if we shouldn't keep the schema stable once we have 9.0 out the door |
would be need for sure, but IDK if we shouldn't keep the schema stable once we have 9.0 out the door |
...DbalAdapter/Tests/Behavior/Features/Bootstrap/ProjectionIntegrityViolationDetectionTrait.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the upgrade instructions and made this followup for a cr:resetall
#4864 (please review :D)
Either way this looks neat and works so lets go
The nodeanchorpoints are an implementation detail of the DBAL adapter, using integers in the projection saves space and helps improving query performance.
The change is surprisingly simple. Due to the locking mechanism it should be fairly safe to do this with auto increments even under load.
Important
This change requires a CR replay if you already have an installation with data.
Note that
resetall
will only be available via #4864. Previously you can usecr:replayall
and ignore the error ;)