Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Make thread_id required in event_push tables #14225

Closed
clokep opened this issue Oct 18, 2022 · 5 comments · Fixed by #15350, #15437 or #15597
Closed

Make thread_id required in event_push tables #14225

clokep opened this issue Oct 18, 2022 · 5 comments · Fixed by #15350, #15437 or #15597
Assignees
Labels
A-Threads Threaded messages O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@clokep
Copy link
Member

clokep commented Oct 18, 2022

Update thread_id column to be non-null:

  1. UPDATE null thread IDs to be 'main' after the previous background update and add NOT NULL constraint.
  2. Drop the thread_id IS NULL indexes (since they're no longer needed).

See #14222 for the code which was backed out, most of it can likely be re-used.

This will re-apply part of #13776.

@clokep clokep added S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. A-Threads Threaded messages O-Uncommon Most users are unlikely to come across this or unexpected workflow labels Oct 18, 2022
@clokep clokep changed the title Make thread_id required in event_push code Make thread_id required in event_push tables Oct 18, 2022
@clokep clokep self-assigned this Oct 18, 2022
@clokep
Copy link
Member Author

clokep commented Mar 17, 2023

I confirmed the necessary operations will now be index scans. So I think we can just do this now? This was back in Synapse v1.70.0, I guess we need to consider the schema ramifications of this first though.

@clokep
Copy link
Member Author

clokep commented Mar 20, 2023

Background

Currently there's a nullable thread_Id column on event_push_actions, event_push_actions_staging event_push_summary that we wish to make non-nullable to simplify some code. This column was added in schema 74.

There are a few related background processes related to this which were previously added:

  • event_push_backfill_thread_id to backfill this data.
  • Indices on thread_id IS NULL were added in event_push_actions_thread_id_null or event_push_summary_thread_id_null (these are added to aid in making it non-null).

If the event_push_backfill_thread_id background update is not complete then the code runs it "just-in-time" for any room/user it is operating on (see example).

End goal

  • Make the thread_id column non-nullable.
  • Remove the compatibility code that updates this column just-in-time.

Proposal

Since we need to assume the thread_id column is non-null, I believe that we need to bump the schema compat version.

Note that currently SCHEMA_COMPAT_VERSION = 73:

  • Schema version 73 added a nullable thread_id column to the tables.
  • Schema version 74 was unrelated, but already shipped.

I am proposing that we:

  • Create a schema version 75 which migrates the thread_id column to be non-null in the foreground, essentially:
    • Update any null thread_id to be "main".
    • Configure the column to be non-null.
    • Drops the event_push_backfill_thread_id background update.
    • Drop the event_push_actions_thread_id_null, event_push_summary_thread_id_null background updates (and indices).

This might be a bad time if event_push_actions_thread_id_null or event_push_summary_thread_id_null haven't finshed, but I don't think we have any reasonably way to guard against that.

We let at least two versions of Synapse pass and then we:

  • Bump SCHEMA_COMPAT_VERSION to 75 (or greater).
  • Drop the code which updates the thread_id column on-the-fly.

TODO: Why do we need to wait to drop the code if we've dropped the background update?!?!?!

@clokep
Copy link
Member Author

clokep commented Mar 20, 2023

TODO: Why do we need to wait to drop the code if we've dropped the background update?!?!?!

Thinking of this more, I think the assurances we want to make are:

  • The database schema has a thread_id column to make non-nullable.
  • We cannot downgrade to a version of the code which attempts to insert a null thread_id column.
  • (Hopefully) The background update finished and the indices exist.

@clokep
Copy link
Member Author

clokep commented Mar 23, 2023

So thinking about this more -- I think we must set the SCHEMA_COMPAT_VERSION to 74 when doing this, but we don't need to bump it ourselves. The initial code running on 74 already has all the assurances we need.

@clokep
Copy link
Member Author

clokep commented Apr 3, 2023

The fix for this got reverted, see #15359.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Threads Threaded messages O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Blocks non-critical functionality, workarounds exist. T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
1 participant