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

Fix background schema updates failing over a large upgrade gap #15887

Merged
merged 2 commits into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/15887.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix background schema updates failing over a large upgrade gap.
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.


from synapse.storage.database import LoggingTransaction
from synapse.storage.engines import BaseDatabaseEngine, PostgresEngine


def run_create(
cur: LoggingTransaction,
database_engine: BaseDatabaseEngine,
) -> None:
"""
An attempt to mitigate a painful race between foreground and background updates
touching the `stream_ordering` column of the events table. More info can be found
at https://github.com/matrix-org/synapse/issues/15677.
"""

# technically the bg update we're concerned with below should only have been added in
# postgres but it doesn't hurt to be extra careful
if isinstance(database_engine, PostgresEngine):
select_sql = """
SELECT 1 FROM background_updates
WHERE update_name = 'replace_stream_ordering_column'
"""
cur.execute(select_sql)
res = cur.fetchone()

# if the background update `replace_stream_ordering_column` is still pending, we need
# to drop the indexes added in 7403, and re-add them to the column `stream_ordering2`
# with the idea that they will be preserved when the column is renamed `stream_ordering`
# after the background update has finished
if res:
drop_cse_sql = """
ALTER TABLE current_state_events DROP CONSTRAINT event_stream_ordering_fkey
"""
cur.execute(drop_cse_sql)

drop_lcm_sql = """
ALTER TABLE local_current_membership DROP CONSTRAINT event_stream_ordering_fkey
"""
cur.execute(drop_lcm_sql)

drop_rm_sql = """
ALTER TABLE room_memberships DROP CONSTRAINT event_stream_ordering_fkey
"""
cur.execute(drop_rm_sql)

add_cse_sql = """
ALTER TABLE current_state_events ADD CONSTRAINT event_stream_ordering_fkey
FOREIGN KEY (event_stream_ordering) REFERENCES events(stream_ordering2) NOT VALID;
"""
cur.execute(add_cse_sql)

add_lcm_sql = """
ALTER TABLE local_current_membership ADD CONSTRAINT event_stream_ordering_fkey
FOREIGN KEY (event_stream_ordering) REFERENCES events(stream_ordering2) NOT VALID;
"""
cur.execute(add_lcm_sql)

add_rm_sql = """
ALTER TABLE room_memberships ADD CONSTRAINT event_stream_ordering_fkey
FOREIGN KEY (event_stream_ordering) REFERENCES events(stream_ordering2) NOT VALID;
"""
cur.execute(add_rm_sql)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm not seeing anywhere in the code where we actually validate these constraits?

Copy link
Contributor Author

@H-Shay H-Shay Jul 17, 2023

Choose a reason for hiding this comment

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

I don't think that has happened yet. These columns/constraints were introduced in #15356. On the discussion of that PR there is mention of validating them in the future, after a background job migrates historical data from the stream_ordering column of events to the new stream ordering columns added, but I don't see anywhere in the deltas after this (for reference these were added in 74) that the background job was added and run or that the constraints were validated.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, fair!