akka / akka-persistence-jdbc

Asynchronously writes journal and snapshot entries to configured JDBC databases so that Akka Actors can recover state
https://doc.akka.io/docs/akka-persistence-jdbc/
Other
307 stars 139 forks source link

Replace event_tag FK to get rid of insert and return #710 #731

Closed Roiocam closed 10 months ago

Roiocam commented 1 year ago

References #710

Roiocam commented 1 year ago

proposal

After several months, I finally have the time to complete this merge. I've thoroughly reviewed the migration's changes, and here are my considerations.

Regarding the event_tag table, there are two essential components:

lazy event load means join table and query.

In the previous approach, we used the event_id as the foreign key. However, this incurred some costs since we had to wait for all events to be inserted before retrieving their IDs. Moreover, the 'slick' plugin had performance issues with 'InsertAndReturn', which led to individual inserts instead of batch inserts.

On the other hand, we can employ the primary key of the event_journal table instead of the 'ordering' column for the foreign key. This modification removes the need for InsertAndReturn.

To ensure a rolling updates when shifting to the "new way," we propose a phased rollout with steps controlled by a configuration property:

  1. Continue with the old approach.
  2. Modify the table structure to allow for a redundant foreign key column, although only one will be used.
  3. Write both old and new foreign keys, but only read the old foreign key for event lazy-loading (configurable through the configuration property).
  4. Once the projection catches up with the newly written foreign key's position, migrate the table to the new primary and foreign keys. Alter the table structure to make the old foreign key column nullable.
  5. Finally, the migration has completed, switch the projection to read the new foreign key and stop writing in the old manner (controlled by configuration properties).

An real world example using MySQL.

1. add new column before migration

ALTER TABLE event_tag
    ADD PERSISTENCE_ID  VARCHAR(255),
    ADD SEQUENCE_NUMBER BIGINT;

2. change to redundant write and read via config

jdbc-journal.tables.event_tag.redundant-write = true
jdbc-read-journal.tables.event_tag.redundant-read = true

3. waitting for projection catech, and then

-- drop old fk column
DELETE
FROM event_tag
WHERE PERSISTENCE_ID IS NULL
  AND SEQUENCE_NUMBER IS NULL;
-- drop old FK constraint
SELECT CONSTRAINT_NAME
INTO @fk_constraint_name
FROM INFORMATION_SCHEMA.REFERENTIAL_CONSTRAINTS
WHERE TABLE_NAME = 'event_tag';
SET @alter_query = CONCAT('ALTER TABLE event_tag DROP FOREIGN KEY ', @fk_constraint_name);
PREPARE stmt FROM @alter_query;
EXECUTE stmt;
DEALLOCATE PREPARE stmt;
-- drop old PK  constraint
ALTER TABLE event_tag
DROP PRIMARY KEY;
-- create new PK constraint for PK column.
ALTER TABLE event_tag
    ADD CONSTRAINT
        PRIMARY KEY (PERSISTENCE_ID, SEQUENCE_NUMBER, TAG);
-- create new FK constraint for PK column.
ALTER TABLE event_tag
    ADD CONSTRAINT fk_event_journal_on_pk
        FOREIGN KEY (PERSISTENCE_ID, SEQUENCE_NUMBER)
            REFERENCES event_journal (PERSISTENCE_ID, SEQUENCE_NUMBER)
            ON DELETE CASCADE;
-- alter the event_id to nullable, so we can skip the InsertAndReturn.
ALTER TABLE event_tag
    MODIFY COLUMN EVENT_ID BIGINT UNSIGNED NULL;

4. finally, rollback the redundant config

jdbc-journal.tables.event_tag.redundant-write = false
jdbc-read-journal.tables.event_tag.redundant-read = false
Roiocam commented 12 months ago

I did some tests and profiles to verify the improvement of performance.

before PR

A flame graph provides two pieces of information: the original approach incurs overhead not only during the commit (insert), but also has cost on during the result coverter.

Furthermore, just 6 samples account for 0.46% + 0.3% of CPU time.(Although this is not strict proof)

截屏2023-08-31 15 05 26

after PR

After this PR, the most obvious change is the elimination of the overhead in result convert. Additionally, there is an improvement in execution efficiency at the database side (though this cannot be demonstrated by this flame graph).

截屏2023-08-31 15 05 43

patriknw commented 11 months ago

Could you add the following to a file: akka-persistence-jdbc/core/src/main/mima-filters/5.2.1.backwards.excludes/issue-710-tag-fk.excludes

ProblemFilters.exclude[IncompatibleSignatureProblem]("akka.persistence.jdbc.journal.dao.JournalTables#EventTags.eventId")
ProblemFilters.exclude[IncompatibleResultTypeProblem]("akka.persistence.jdbc.journal.dao.JournalTables#TagRow.eventId")
ProblemFilters.exclude[DirectMissingMethodProblem]("akka.persistence.jdbc.journal.dao.JournalTables#TagRow.copy")
ProblemFilters.exclude[IncompatibleResultTypeProblem]("akka.persistence.jdbc.journal.dao.JournalTables#TagRow.copy$default$1")
ProblemFilters.exclude[IncompatibleResultTypeProblem]("akka.persistence.jdbc.journal.dao.JournalTables#TagRow.copy$default$2")
ProblemFilters.exclude[DirectMissingMethodProblem]("akka.persistence.jdbc.journal.dao.JournalTables#TagRow.this")
ProblemFilters.exclude[MissingTypesProblem]("akka.persistence.jdbc.journal.dao.JournalTables$TagRow$")
ProblemFilters.exclude[DirectMissingMethodProblem]("akka.persistence.jdbc.journal.dao.JournalTables#TagRow.apply")
ProblemFilters.exclude[IncompatibleSignatureProblem]("akka.persistence.jdbc.journal.dao.JournalTables#TagRow.unapply")
patriknw commented 11 months ago

There are also some test failures from CI if you can take a look at those?

Roiocam commented 11 months ago

Could you add the following to a file: akka-persistence-jdbc/core/src/main/mima-filters/5.2.1.backwards.excludes/issue-710-tag-fk.excludes

of course.

There are also some test failures from CI if you can take a look at those?

Renato has some suggestions on this PR that sparked some thoughts in me: Perhaps we can simplify the rolling update steps by migrating SQL.

I have some thoughts, but I am currently trapped by other issues and don't have the time for it at the moment. I will fix and verify them over the weekend.

Roiocam commented 10 months ago

The migration guide will be add it on new pull request.

Roiocam commented 10 months ago

@octonato could you please take a look agin? Thanks.