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

Update the thread_id right before use (in case the bg update hasn't finished) #14222

Merged
merged 5 commits into from
Oct 18, 2022

Conversation

clokep
Copy link
Member

@clokep clokep commented Oct 18, 2022

As discussed with @erikjohnston this makes some changes:

  1. Reinstate the backwards compat code.
  2. Add a background update to add indexes for thread_id IS NULL.

A future PR would then:

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

Partially backsout #13776.

@clokep clokep changed the title Update event_push_actions and event_push_summary to ensure there are no Update the thread_id right before use (in case the bg update hasn't finished) Oct 18, 2022
Comment on lines +313 to +316
# Check ASAP (and then later, every 1s) to see if we have finished
# background updates the event_push_actions and event_push_summary tables.
self._clock.call_later(0.0, self._check_event_push_backfill_thread_id)
self._event_push_backfill_thread_id_done = False
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe overkill, but it seems good to not constantly do those updates if we don't need to?

Comment on lines +16 to +17
-- Allow there to be multiple summaries per user/room.
DROP INDEX IF EXISTS event_push_summary_unique_index;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping this index is somewhat scary, I don't know how well this will handle rolling back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we can only rollback to v1.68, which I thought we checked was fine?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it will work OK with that, yes.

Comment on lines +19 to +23
INSERT INTO background_updates (ordering, update_name, progress_json, depends_on) VALUES
(7306, 'event_push_actions_thread_id_null', '{}', 'event_push_backfill_thread_id');

INSERT INTO background_updates (ordering, update_name, progress_json, depends_on) VALUES
(7306, 'event_push_summary_thread_id_null', '{}', 'event_push_backfill_thread_id');
Copy link
Member Author

@clokep clokep Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose not to care about event_push_actions_staging here since it is transient and should be handled OK once the rows get into event_push_actions.

(And the table shouldn't be large so I don't think it can really have non-nulls in it? Maybe I should just do the alter table for that one now?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(And the table shouldn't be large so I don't think it can really have non-nulls in it? Maybe I should just do the alter table for that one now?)

Yeah, we also have a cleanup task for it to ensure that stale entries get removed, so it really should be quick. Though at this point it might be worth just doing the UPDATE when we do the others too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though at this point it might be worth just doing the UPDATE when we do the others too

But we don't need to since we don't join to the table; we could maybe do something when we pull rows from event_push_actions_staging and insert into event_push_actions? But the data can only be "incorrect" across a restart of Synapse, I believe, which ends up orphaning the data anyway (since the event wouldn't be persisted).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the data can only be "incorrect" across a restart of Synapse, I believe, which ends up orphaning the data anyway (since the event wouldn't be persisted).

My only mild concern is what happens when we do a rolling restart or something, but I think that's fine?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the data can only be "incorrect" across a restart of Synapse, I believe, which ends up orphaning the data anyway (since the event wouldn't be persisted).

My only mild concern is what happens when we do a rolling restart or something, but I think that's fine?

(Note that Synapse 1.69 fills in that column in the staging table so shouldn't be an issue)

I don't think it will be a problem regardless:

  1. event_push_actions_staging has a null thread_id value.
  2. It gets moved over to event_push_actions (there's now a null in event_push_actions).
  3. Before we use event_push_actions the column gets updated to be "main" and things work.

There might be a slight race in here with whether bg update has ran though. 😟

@clokep clokep marked this pull request as ready for review October 18, 2022 12:46
@clokep clokep requested a review from a team as a code owner October 18, 2022 12:46
@clokep
Copy link
Member Author

clokep commented Oct 18, 2022

Filed #14225 as a follow-up.

@clokep clokep merged commit dbf18f5 into develop Oct 18, 2022
@clokep clokep deleted the clokep/jit-non-nulls branch October 18, 2022 14:55
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Oct 29, 2022
Upstream changes:

Synapse 1.70.1 (2022-10-28)
===========================

(bugfixes)


Synapse 1.70.0 (2022-10-26)
===========================

Features
--------

- Support for
  [MSC3856](matrix-org/matrix-spec-proposals#3856):
  threads list
  API. ([\#13394](matrix-org/synapse#13394),
  [\#14171](matrix-org/synapse#14171),
  [\#14175](matrix-org/synapse#14175))

- Support for thread-specific notifications & receipts
  ([MSC3771](matrix-org/matrix-spec-proposals#3771)
  and
  [MSC3773](matrix-org/matrix-spec-proposals#3773)). ([\#13776](matrix-org/synapse#13776),
  [\#13824](matrix-org/synapse#13824),
  [\#13877](matrix-org/synapse#13877),
  [\#13878](matrix-org/synapse#13878),
  [\#14050](matrix-org/synapse#14050),
  [\#14140](matrix-org/synapse#14140),
  [\#14159](matrix-org/synapse#14159),
  [\#14163](matrix-org/synapse#14163),
  [\#14174](matrix-org/synapse#14174),
  [\#14222](matrix-org/synapse#14222))

- Stop fetching missing `prev_events` after we already know their
  signature is
  invalid. ([\#13816](matrix-org/synapse#13816))

- Send application service access tokens as a header (and query
  parameter). Implements
  [MSC2832](matrix-org/matrix-spec-proposals#2832). ([\#13996](matrix-org/synapse#13996))

- Ignore server ACL changes when generating pushes. Implements
  [MSC3786](matrix-org/matrix-spec-proposals#3786). ([\#13997](matrix-org/synapse#13997))

- Experimental support for redirecting to an implementation of a
  [MSC3886](matrix-org/matrix-spec-proposals#3886)
  HTTP rendezvous
  service. ([\#14018](matrix-org/synapse#14018))

- The `/relations` endpoint can now be used on
  workers. ([\#14028](matrix-org/synapse#14028))

- Advertise support for Matrix 1.3 and 1.4 on
  `/_matrix/client/versions`. ([\#14032](matrix-org/synapse#14032),
  [\#14184](matrix-org/synapse#14184))

- Improve validation of request bodies for the [Device
  Management](https://spec.matrix.org/v1.4/client-server-api/#device-management)
  and [MSC2697 Device
  Dehyrdation](matrix-org/matrix-spec-proposals#2697)
  client-server API
  endpoints. ([\#14054](matrix-org/synapse#14054))

- Experimental support for
  [MSC3874](matrix-org/matrix-spec-proposals#3874):
  Filtering threads from the `/messages`
  endpoint. ([\#14148](matrix-org/synapse#14148))

- Improve the validation of the following PUT endpoints:
  [`/directory/room/{roomAlias}`](https://spec.matrix.org/v1.4/client-server-api/#put_matrixclientv3directoryroomroomalias),
  [`/directory/list/room/{roomId}`](https://spec.matrix.org/v1.4/client-server-api/#put_matrixclientv3directorylistroomroomid)
  and
  [`/directory/list/appservice/{networkId}/{roomId}`](https://spec.matrix.org/v1.4/application-service-api/#put_matrixclientv3directorylistappservicenetworkidroomid). ([\#14179](matrix-org/synapse#14179))


Deprecations and Removals
-------------------------

- Remove the experimental implementation of
  [MSC3772](matrix-org/matrix-spec-proposals#3772). ([\#14094](matrix-org/synapse#14094))

- Remove the unstable identifier for
  [MSC3715](matrix-org/matrix-spec-proposals#3715). ([\#14106](matrix-org/synapse#14106),
  [\#14146](matrix-org/synapse#14146))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants