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

Fix ambiguous column name that would prevent use of MSC2716 History Import when using Postgres as a database. #12843

Merged
merged 3 commits into from
May 26, 2022
Merged

Fix ambiguous column name that would prevent use of MSC2716 History Import when using Postgres as a database. #12843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dab6190
Disambiguate `event_id` column name in query as Postgres doesn't acce…
reivilibre May 23, 2022
e69a9ae
Newsfile
reivilibre May 23, 2022
3a67e4e
Update Newsfile
reivilibre May 26, 2022
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/12843.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix bug where servers using a Postgres database would fail to backfill from an insertion event when MSC2716 is enabled (`experimental_features.msc2716_enabled`).
2 changes: 1 addition & 1 deletion synapse/storage/databases/main/event_federation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1057,7 +1057,7 @@ def _get_connected_batch_event_backfill_results_txn(
INNER JOIN batch_events AS c
ON i.next_batch_id = c.batch_id
/* Get the depth of the batch start event from the events table */
INNER JOIN events AS e USING (event_id)
INNER JOIN events AS e ON c.event_id = e.event_id
Copy link
Contributor

@MadLittleMods MadLittleMods May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

c.event_id (where batch_events AS c) makes sense 👍

We want to return connected batch events and the info about the batch event (depth, stream_ordering).

Thanks for fixing this up!

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we configure SQLite to yell about ambiguous columns as well?

Copy link
Contributor Author

@reivilibre reivilibre May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't found a way. I've opened a thread on their forum about this (since I didn't find anyone mentioning this behaviour anywhere, so I may as well be the one to do so).
If any SQLite pro comes back to me with a 'oh you forgot to enable the PRAGMA no_silly_stuff=1 flag' then I'll come back and enable it :-).

Copy link
Member

Choose a reason for hiding this comment

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

(Cross link please? 🙏 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's intentional behaviour and there are test cases ensuring it picks the first one: https://www.sqlite.org/forum/forumpost/941664fc450a4f56
Mostly as a legacy compatibility thing, but RIGHT and FULL joins will cause ambiguity checking (because old SQLites didn't support them and hence no applications can be relying on this).

Copy link
Contributor

@MadLittleMods MadLittleMods May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Complement tests pass with either c or i so weren't much use in discerning the two :/.

For reference, backfilling from the insertion or batch event will work. But we already have the insertion event so there isn't a need to backfill it again. And the mistake here would only make it hint to backfill from the batch event earlier anyway.

They are both in the historical chain (diagram)

/* Find an insertion event which matches the given event_id */
WHERE i.event_id = ?
LIMIT ?
Expand Down