Persons table schema migration

[yakkomajuri]

Changes

Still WIP as I need to get feedback on this.

This is part of the work to ensure CH and Postgres are always in sync.

The approach here is to always increment a version column in Postgres when we make an update (see #6628). This version is then passed to ClickHouse and we'll collapse rows based on this value.

The interesting part comes with is_deleted. Initially, I added is_deleted to the sort key (ORDER BY clause). This would ensure that we couldn't possibly collapse all rows with is_deleted (as has happened before), leaving us with a person that is deleted in Postgres but exists in ClickHouse.

The problem here is storage. We'd have to keep around 2x rows for each deleted person. As we'd only ever collapse down to 2 rows: one with is_deleted=0 and one with is_deleted=1.

The solution here was inspired by what we did with distinct IDs. When reading from Kafka, we can simply assign version to the max possible UInt64 value, ensuring we always keep the is_deleted row around without having to add it to the sort key. The way this was done also ensures backwards compatibility.

Other points/questions

1. I opted for not changing the queries. They currently use argMax with _timestamp but this should be fine. The case where this could be incorrect is if we have equal timestamps before the collapsing happens. The alternative is to change the queries to use version, but we'd run into problems when we first migrate. Happy to get some thoughts here.
2. I also added _partition to the table. We currently have _offset, which is somewhat helpful, but for it to be truly helpful we need the partition number too.

How did you test this code?

Added tests for direct insertion and "kafka insertion" into the new table (removed due to flakiness). Confirmed collapsing works as expected.

Also tested manually

[yakkomajuri]

Note to self: consider https://clickhouse.com/docs/en/engines/table-engines/mergetree-family/versionedcollapsingmergetree/

EDIT: Looked into it, not what we need. I'd have loved cancel rows to take priority over the version, but that's not the case.

[yakkomajuri]

@macobo would appreciate a second pass after our chat.

@fuziontech can I get your 👁️ 👁️ in here too?

[yakkomajuri]

Getting a paths test failure. Looked at it and it seems unrelated. Any context @neilkakkar ?

[yakkomajuri]

also friendly ping @fuziontech

[yakkomajuri]

Converted to draft as a safety measure given we first need to merge the changes that send us this version value via Kafka

[fuziontech]

This will work, but will probably need to be tested for helm chart deploys. This might time out the migration task. This will also require a custom migration for cloud. These select * into migrations are super slow on clickhouse. Migration will probably take 30-60 minutes on cloud.

[tiina303]

re chart

For the migration task timeout we can just increase it in the command line via --timeout 60m or to whatever as needed (if it's longer than 20m we should potentially make it a major upgrade with a note about it just being a big migration and needing more time).
However is that going to be the case for all new installs, n00b q: do we run all migrations from the beginning of time always?

[fuziontech]

I've tested this pretty extensively now and am comfortable to land it. Landing now and monitoring 👀