legacyTagKey does not maintain the legacy schema layout

[sebastian-alfers]

This contribution introduced a new column persistence_id in the event_tag table. Also, a new flag legacyTagKey got introduced which was intended default to the backward compatibility code path that works without the new column.

However, looking at this section shows that regardless of the value of legacyTagKey a persistenceId gets written. In the default case (legacyTagKey=trure) there is no such column.

[sebastian-alfers]

Looking through the implementation, I think that the migration docs were just not right. Looking at this comment, it gives the impression that the schema is actually intended to be changed (two columns added) without taking advantage of the improvements (which is legacyTagKey=true). So one immediate solution would be to run the first part of the migration script, e.g. for postgres:

ALTER TABLE public.event_tag
    ADD persistence_id  VARCHAR(255),
    ADD sequence_number BIGINT;

I assume this works since the query part does truly maintain the legacy schema layout by only looking at the event_id column.

Waiting for additional feedback if this issue just ends up needing to fix the migration docs, or if we need a fix in the implementation to not insert into the two additional columns in the legacy-tag-key mode.

[johanandren]

From talking to @patriknw and @octonato the intent was to not require the schema change.

Could still be that it is to tricky to get working to be worth it, but we should at least try to get it working with the old schema.

[sebastian-alfers]

I did not find a good way to have a runtime-migration between the old and new schema which would allow a non-downtime migration to the non-legacy tag approach.

I think we should revert the default code path and schema to what is was pre 5.4.0, and then add explicit docs to explain how to go to the non-legacy tag mode - which requires a downtime.

Alternative: add to the docs that adding the two columns is required - which feels bad in a non-major release.

[patriknw]

and then add explicit docs to explain how to go to the non-legacy tag mode - which requires a downtime

I don't think it will require downtime. That was the whole intention of the effort in adding the config flags. I think it's already possible to do a multi-phase rolling update, but it requires that the two columns are added as the first thing while it's running the old version. I agree that it's unfortunate to require adding those columns.

[octonato]

Done in #832