Skip to content
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 individual transactions in subscription engine and use session lock via GET_LOCK #5383

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Nov 28, 2024

Adjustments for #5321

1.) Introduce dedicated locking and use small transactions

using many small transactions has two advantages:

  • on a replay with many events, we dont build up a huge transaction that the database has to possible extract into files at some point. Otherwise, we would need to introduce batching and need to require the for update lock
  • to ensure integrity - that the projection got exactly that many events how it is in the subscription store - each applied event per projection has its own transaction. If something goes really wrong (php crashes) the transaction will not be committed. This is important for the case of using a different connection or a different database for a projection, as otherwise a crash of another projection would lead to a external projection being up-to-date while the subscription update is fully rolled back.

later we can introduce a concept of batching possibly - even though it must be patched per subscriber and not mixed to ensure integrity in fatal errors.

2.) Ensure onAfterCatchUp is always executed after the projection is persisted. Even in error case.

3.) Deactivate ParallelWritingInWorkspacesTest test for now

we will implement a proper wait mechanism later -> see #5376
Previously that test worked in the subscription branch as we used a FOR UPDATE lock which waits by default instead of using NOWAIT.

To not put the load on the database in GET_LOCK we call it with no wait (0 timeout)

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mhsdesign mhsdesign changed the title WIP Subscription with GET_LOCK Nov 29, 2024
using many small transactions has two advantages:
- on a replay with many events, we dont build up a huge transaction that the database has to possible extract into files at some point. Otherwise, we would need to introduce batching and need to require the for update lock
- to ensure integrity - that the projection got exactly that many events how it is in the subscription store - each applied event per projection has its own transaction. If something goes really wrong (php crashes) the transaction will not be committed. This is important for the case of using a different connection or a different database for a projection, as otherwise a crash of another projection would lead to a external projection being up-to-date while the subscription update is fully rolled back.

later we can introduce a concept of batching possibly - even though it must be patched per subscriber and not mixed to ensure integrity in fatal errors.
we will implement a proper wait mechanism later.
Previously that test worked in the subscription branch as we used a FOR UPDATE lock which waits by default instead of using NOWAIT.

To not put the load on the database in `GET_LOCK` we call it with no wait (0 timeout)
@mhsdesign mhsdesign force-pushed the task/subscription-engine-get-and-release-lock branch from 7da6d1f to 22c3d3e Compare December 2, 2024 10:47
@mhsdesign mhsdesign changed the title Subscription with GET_LOCK TASK: Use individual transactions in subscription engine and use session lock via GET_LOCK Dec 2, 2024
@github-actions github-actions bot added the Task label Dec 2, 2024
@mhsdesign mhsdesign marked this pull request as ready for review December 2, 2024 10:50
@mhsdesign
Copy link
Member Author

Initially christian and me wanted to reimplement the current 9.0 behaviour just with subscriptions: #5321 (comment)

The idea was to do not do any crazy behaviour changes as that would allow us to get the change in faster and also measure performance later if we want to change anything.

That turned out more complex (again :D). So the idea was get - without lock - the lowest position. Fetch the event stream from there, use a lot of for update transactions for each projection and each event like we do currently.

Problems with that approach:

  • the onBeforeCatchUp / onAfterCatchUp hooks will be more difficult and less obvious to invoke
  • we cannot detach old subscriptions as we loop over the registered subscribers and not the db entries
  • we always have to fetch the subscriber and then skip its position if its ahead (as there is no central lock)
  • we cannot abort easily if there are no matching subscribers, for example in BOOTING state, as we cannot filter in advance

Thus this pr now is a compromise. It uses central locking via GET_LOCK (which is session based and released on fatal errors or CTRL+C) which allows us to fetch entries and work with them without having to worry about the lock. We do not utilise FOR UPDATE anymore now because of that.

@mhsdesign mhsdesign requested a review from kitsunet December 2, 2024 11:00
@mhsdesign mhsdesign mentioned this pull request Dec 2, 2024
37 tasks
@mhsdesign mhsdesign requested a review from bwaidelich December 2, 2024 20:42
@mhsdesign mhsdesign marked this pull request as draft December 3, 2024 13:46
@mhsdesign
Copy link
Member Author

After discussing with Bastian again we opted against this change.

Instead it will be replaced by #5392

Main motivation - as will be further outlined in the main pr - is that the FOR UPDATE has the future simplicity of being able to catchup multiple subscribers in parallel. Also the default wait is perfect and we dont need exactly once delivery for external projections and crazy rollbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant