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 2 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 ambiguous column name that would prevent use of MSC2716 History Import when using Postgres as a database.
Copy link
Member

Choose a reason for hiding this comment

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

Can you rephrase this as what the end user would see, rather than what is happening internally? I.e. something like "Fix bug where servers using SQLite3 would fail to backfill in MSC2716 rooms" or something.

The changelog is aimed at server admins rather than developers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the problem is that I have never used this feature and don't really understand how this would look to a server admin :-).

Fix bug where servers using a Postgres database would fail to import historical messages using MSC2716.

Would that be more suitable?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, potentially? @MadLittleMods may have a better idea

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't fail to import for me with my local Postgres Synapse setup.

Looking at this spot of code, I am guessing it will fail to backfill from another server when it sees an insertion event if you have experimental_features.msc2716_enabled enabled.

This seems more accurate:

Fix bug where servers using a Postgres database would fail to backfill from an insertion event when MSC2716 is enabled (experimental_features.msc2716_enabled).

$ psql -V
psql (PostgreSQL) 13.5

$ postgres -V
postgres (PostgreSQL) 13.5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, my message was purely going off the name of the test. What you've written makes sense.

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