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

[Roiocam]

References #710

[patriknw]

looking good, but a compatibility approach is needed

[patriknw]

looking good

[Roiocam]

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:

• The ordering used to sequence events in a specific stream.
• The foreign key used to load event payloads lazily.

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]

During the verify step I commented these line here, and using the original approach to insert the same rows (at this time, the table didn't have any new column of journal table PK). By doing this, the outcome aligns more closely with real-world scenarios.

However, in order to avoid redundancy with the old schema, I'm use the new approach here to simulate the insertion.

I've validated this with databases other than H2, as I believe that when using H2, it implies that migration is not necessary.

[Roiocam]

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)

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).

[patriknw]

LGTM, thanks for great work on this

[octonato]

Looking good.

I think we can improve the migration instructions. We also need a page about it.

For example, we need to ask users to create new columns and deploy a new version using redundant-write.

Then users still need to run an SQL script to backfill the PID and SeqNr. Something like:

UPDATE event_tag
  SET
    persistence_id = event_journal.persistence_id,
    sequence_number = event_journal.sequence_number
  FROM event_journal
  WHERE event_journal.ordering = event_tag.event_id;

After that, deploy once more with redundant-read plus changes in the schema.

About redundant reads and writes, I find the name ambiguous. I made a comment in my comments below.

I think we can even use one single config. For example, legacy-tag-key. By default set to true.

Users should add the new columns and update the plugin.
The application will start to write in all three columns.

After backfilling the two new columns (pid and seq_nr) with data from event_journal, the user deploys once again with legacy-tag-key=false.
Now, the application won't write the event_id anymore and will always make the join using pid and seq_nr.

[octonato]

It's not clear to me what do you mean by projection catch-up? Are you referring to the migration tool? The tool is intended for migration from previous versions.

With the changes in this PR, we need to run another kind of migration.

Luckily, we can do this migration with plain SQL. For example, for Postgres, we can run the following:

UPDATE event_tag
  SET
    persistence_id = event_journal.persistence_id,
    sequence_number = event_journal.sequence_number
  FROM event_journal
  WHERE event_journal.ordering = event_tag.event_id;

After that, we can create the new foreign key and even drop the event_id.

[Roiocam]

"after projection catch-up" refers to the process of waiting for Akka Projection to update the offset to the earliest new column write.

I hadn't considered the option of migrating data using SQL, which is great but I overlooked it. Including this SQL in the migration steps allows us to proceed without waiting for the Projection read "catch up".

I will update PR after fix integration test.

[octonato]

For clarification, there is no projection involved here.

The tags are written when the events are persisted, not when consumed by a Projection.

So basically, the journal stays what it is. Old tags will be written with ordering filled and pid and seq_nr will stay empty. New tags will have pid and seq_nr filled. Without explicit migration, nothing will happen.

Btw, Akka Projection is a separate project and may not be in use at all. You can use Akka Persistence with this plugin without even using Akka Projection.

[patriknw]

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]

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

[Roiocam]

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]

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

[Roiocam]

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

[Roiocam]

using the SQL migrate old rows, then we could rolling updates.

[octonato]

LGTM

I left a few comments but more about things for later we should consider after we cut a release with this change.

[octonato]

This is fine, but we could also read the config key and then decide if we want to fill this column or not.

Ultimately, it will be nicer if users can even not have this column in their table.

[octonato]

It would be better if new users just don't have this column.

I know, legacy mode is enabled by default and if you just start today with the plugin you will be using it in legacy mode without even noticing it.

I don't have a solution. I'm just mentioning it so we can think together about alternatives.

In the worst case scenario, we leave it as is and people will have a column that is never used.

[octonato]

Probably, we need to go with that and on a next release we simply drop this column and we remove the legacy mode flag.

Then users will first need to move to the previous version, run the migrate and then move to the next version. After all that they will be able to drop the event_id column.

[octonato]

@Roiocam, thanks for all the efforts and patience. I have one more comment that I think we need to address before merging.

We are almost there.

[octonato]

I just realised that the script needs to be run in two parts. First add the columns, redeploy with new version, then run the rest of the script and redeploy once more with legacy-mode set to false. See my comment on the migration guide PR: #781 (review)