Reintroduce DB changes from #14979, using SCHEMA_COMPAT_VERSION mechanism

[DMRobertson]

The changes in #14979

ALTER TABLE current_state_events ADD COLUMN event_stream_ordering BIGINT;
ALTER TABLE local_current_membership ADD COLUMN event_stream_ordering BIGINT;
ALTER TABLE room_memberships ADD COLUMN event_stream_ordering BIGINT;

INSERT INTO background_updates (update_name, progress_json) VALUES
  ('populate_membership_event_stream_ordering', '{}');

Problems:

• the query used by get_ex_outlier_stream_rows_txn becomes ambiguous: there are two columns called event_stream_ordering. Add event_stream_ordering column to membership state tables #14979 disambiguates this, but won't exist in old code.
• the background update doesn't add an ordering, so it might usurp other updates in the queue.

Questions:

• does the new query assume the populate_membership_event_stream_ordering background update has completed? How long do we expect it will take?

For 1.77 I

• reverted the PR: 9cd7610
• but kept in the change which disambiguates the columns in the source code anyway: f10caa7
• and bump SCHEMA_VERSION: Bump schema version #15036

For the future we should try to land the schema changes gradually (see guidance) over the next few releases.

[DMRobertson]

(cc @Fizzadar)

[DMRobertson]

I've updated the description and labels to explain what I did in 1.77.

[Fizzadar]

Thank you for catching the compact & handling this @DMRobertson, I'll resubmit using the SCHEMA_COMPAT_VERSION next week(ish).

[DMRobertson]

No worries @Fizzadar. I think we'd normally hope to land it in stages e.g.:

• disambiguate the query (1.77)
• add the columns, start writing to them, and kick off the background job. (e.g. 1.78)
• require the background job to have completed, and read from the new columns. (e.g. 1.79)

I'm not fully sure when SCHEMA_VERSION and SCHEMA_COMPAT_VERSION should get bumped. The example in the docs is for removing a column, not adding one. There might be some clues in big comment describing SCHEMA_VERSION bumps, see

synapse/synapse/storage/schema/__init__.py

Line 81 in b4ec4f5

Changes in SCHEMA_VERSION = 73;

In fact, judging from

synapse/synapse/storage/schema/__init__.py

Lines 50 to 52 in b4ec4f5

	Changes in SCHEMA_VERSION = 66:
	- Queries on state_key columns are now disambiguated (ie, the codebase can handle
	the `events` table having a `state_key` column).

maybe I should have bumped the version when disambiguating the query?

(cc @richvdh, does the above sound sensible?)

[richvdh]

maybe I should have bumped the version when disambiguating the query?

Oh bah, yes you should. Otherwise we have no way to distinguish between a Synapse that is compatible with the new columns, and one which is not.

Can you do it now, before 1.77final?

[DMRobertson]

Can you do it now, before 1.77final?

Will do.

[richvdh]

Re the other comments on SCHEMA_VERSION, in particular

synapse/synapse/storage/schema/__init__.py

Lines 82 to 88 in b4ec4f5

	- thread_id column is added to event_push_actions, event_push_actions_staging
	event_push_summary, receipts_linearized, and receipts_graph.
	- Add table `event_failed_pull_attempts` to keep track when we fail to pull
	events over federation.
	- Add indexes to various tables (`event_failed_pull_attempts`, `insertion_events`,
	`batch_events`) to make it easy to delete all associated rows when purging a room.
	- `inserted_ts` column is added to `event_push_actions_staging` table.

What we really care about for SCHEMA_VERSION is the expectations made by the python code. Those three points don't really give us any clues about those expectations. Compare with most of the earlier comments which are more explicit about how the Python is changing, rather than the structure of the database.

Not necessarily saying they need fixing retrospectively; my point is more that they aren't great examples to follow.

[DMRobertson]

Can you do it now, before 1.77final?

Will do.

#15036

[Fizzadar]

I have now reintroduced this in #15128 with proper schema bump & ordering on the background job. To answer Q's above:

does the new query assume the populate_membership_event_stream_ordering background update has completed?

No only to use existing columns explicitly.

How long do we expect it will take?

Probably a while :) Requires iterating over all membership events so a pretty hefty job overall.