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

Properly setup more sequences in portdb #16043

Merged
merged 1 commit into from
Aug 1, 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/16043.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a long-standing bug where the `synapse_port_db` failed to configure sequences for application services and partial stated rooms.
18 changes: 15 additions & 3 deletions synapse/_scripts/synapse_port_db.py
Original file line number Diff line number Diff line change
Expand Up @@ -761,14 +761,18 @@ def alter_table(txn: LoggingTransaction) -> None:

# Step 2. Set up sequences
#
# We do this before porting the tables so that event if we fail half
# We do this before porting the tables so that even if we fail half
# way through the postgres DB always have sequences that are greater
# than their respective tables. If we don't then creating the
# `DataStore` object will fail due to the inconsistency.
self.progress.set_state("Setting up sequence generators")
await self._setup_state_group_id_seq()
await self._setup_user_id_seq()
await self._setup_events_stream_seqs()
await self._setup_sequence(
"un_partial_stated_event_stream_sequence",
("un_partial_stated_event_stream",),
)
await self._setup_sequence(
"device_inbox_sequence", ("device_inbox", "device_federation_outbox")
)
Expand All @@ -779,6 +783,11 @@ def alter_table(txn: LoggingTransaction) -> None:
await self._setup_sequence("receipts_sequence", ("receipts_linearized",))
await self._setup_sequence("presence_stream_sequence", ("presence_stream",))
await self._setup_auth_chain_sequence()
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'm not quite sure why the auth_chain_sequence one has a separate method, it seems almost identical. 🤷

await self._setup_sequence(
"application_services_txn_id_seq",
("application_services_txns",),
"txn_id",
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
)

# Step 3. Get tables.
self.progress.set_state("Fetching tables")
Expand Down Expand Up @@ -1083,7 +1092,10 @@ def _setup_events_stream_seqs_set_pos(txn: LoggingTransaction) -> None:
)

async def _setup_sequence(
self, sequence_name: str, stream_id_tables: Iterable[str]
self,
sequence_name: str,
stream_id_tables: Iterable[str],
column_name: str = "stream_id",
) -> None:
"""Set a sequence to the correct value."""
current_stream_ids = []
Expand All @@ -1093,7 +1105,7 @@ async def _setup_sequence(
await self.sqlite_store.db_pool.simple_select_one_onecol(
table=stream_id_table,
keyvalues={},
retcol="COALESCE(MAX(stream_id), 1)",
retcol=f"COALESCE(MAX({column_name}), 1)",
Comment on lines -1096 to +1108
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine as-is, but I have some thoughts as an aside.

The error message quoted in the logs of #16040 uses SELECT GREATEST(MAX(txn_id), 0). Is there an important difference between 0 and 1 as a fallback here? I don't think so...

I also vaguely remember reading some stream code that suggests streams could count backwards.

step(int): which direction the stream ids grow in. +1 to grow
upwards, -1 to grow downwards.

If negative-step streams are used gets used, I think this logic will be broken. But I suspect we never use it (and would vote to remove it).

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 think we at some point changed these to start at 1 instead of 0 in some situations? I don't think there's much of a difference honestly. I didn't understand the fixes there though, see #13766.

(Note that this coalesces to 1, but then adds 1 below so it is either 0 or 2.)

allow_none=True,
),
)
Expand Down
Loading