Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: Fix Postgres messagesNewerThan wrong buffer #276

Merged
merged 1 commit into from
Mar 6, 2017

Conversation

digitalcircuit
Copy link
Contributor

@digitalcircuit digitalcircuit commented Mar 5, 2017

In short

  • Fix the PostgreSQL select_messagesNewerThan statement limiting to the wrong buffer
    • Replace use of message ID $1 for bufferid in inner SELECT with the actual buffer ID, $2
    • Fixes using the wrong buffer for lastmsgid limit, resolving missing messages with unread fetching
    • Fixes regressions from pull request 273, missed in pull request 274
Criteria Rank Reason
Impact ★★★ 3/3 Fixes missing messages with Postgres unread backlog fetching
Risk ★☆☆ 1/3 Variable change shouldn't break more, risk accepted in previous PRs
Intrusiveness ★★☆ 2/3 Minor schema change, might interfere with other pull requests

Apologies for missing this in pull request 274. There, I tested that loading backlog worked for new setups. I did not test on a larger data-set.

Note: This does not fix the client-side issue of generating the invalid IDs. That still needs fixed in a follow-up.

Example

Before

Quassel with unread fetching misses new messages, e.g. Test message does not show, Freenode does not show activity in status buffer
Backlog fetching set to unread, Quassel client with broken core on PostgreSQL does not show all new messages that happened while disconnected, resulting in a gap

After

Unread fetching grabs the new messages, e.g. Test message does show, Freenode shows activity in status buffer
Backlog fetching set to unread, Quassel client with fixed core on PostgreSQL shows all new messages that happened while disconnected, no gaps, etc

Side-by-side (production)

Quasseldroid uses fixed fetching, Quassel desktop uses unread fetching, both on the git master core. Notice the #sandstorm channel and the Freenode status buffer
Backlog fetching set to unread, Quassel client with broken core on PostgreSQL misses buffer activity, Quasseldroid set to use fixed fetching works and shows activity

@Nevcairiel
Copy link
Contributor

Does this automatically apply to cores that already updated to git master today?

@digitalcircuit
Copy link
Contributor Author

@Nevcairiel If you upgraded to the latest commit (master as of today, 570fe57 ), got the schema upgrade, then upgrade to this, yes, it will fix the issue (no schema upgrade required). My testing involved swapping the binaries for with and without this PR, no database changes needed.

Make the inner SELECT statement use the buffer ID for bufferid, NOT
the messageid.

This fixes select_messagesNewerThan using the wrong variable for the
buffer ID, causing more recent messages to not show up when fetching
backlog with the "Unread messages" method.

Fixes regression in pull request quassel#273, missed by pull request quassel#274.

NOTE: The client still needs fixed to stop sending invalid IDs.  That
should be addressed with another set of commits.

See quassel#274 (comment)
@digitalcircuit
Copy link
Contributor Author

@egs-me Belated apology for the hassle over missing this in pull request #274. I'll be more careful with future testing.

@MrEgs
Copy link
Contributor

MrEgs commented Mar 6, 2017

OK, that's an obvious error and a clear fix.

@digitalcircuit No worries. It's not the first time that I've broken quassel:master. ;-) Unfortunately, I really do not have any time to do properly testing myself or even testing at all. So I have to rely on the testing of others and judge by the diffs, which, by and large, looked well.

Anyways: thanks again to everybody involved in this entire affair. All of you did valuable work. And although, the master branch should always be in a working state, I'm quite sure that it can overcome a small hickup.

Cheers,
Marcus

@MrEgs MrEgs closed this Mar 6, 2017
@MrEgs MrEgs reopened this Mar 6, 2017
@MrEgs MrEgs merged commit 8af0ba8 into quassel:master Mar 6, 2017
@digitalcircuit digitalcircuit deleted the fix-sql-postgres-newerthan branch April 24, 2017 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants