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

Possible fix for #917 #930

Merged
merged 2 commits into from
Mar 23, 2019

Conversation

jacklul
Copy link
Collaborator

@jacklul jacklul commented Feb 28, 2019

This should prevent constrain errors, one of side effects observed is that some messages might not get deleted in the first run. #917

Usage of two additional nested subqueries is extremely heavy here though... not sure if doing one select query and then using PHP to select ids to remove wouldn't be a lighter solution here.

PS. telegram_update query was broken because of this missing AND updated_at < \'%1$s\' statement for chat table - it didn't clean the table until I added that - @noplanman ?

@jacklul jacklul requested a review from noplanman February 28, 2019 19:25
@noplanman
Copy link
Member

Ok, I've looked into this and there is a super simple fix for this:
Reverse the direction of message deletion!

Then there are no constraint problems 🎉

@jacklul I've added a commit to this PR with just that tiny change, please test it to see if it also works for you.

@noplanman noplanman force-pushed the fix_message_cleanup branch from 29b31c8 to 112ef4b Compare March 8, 2019 23:48
@noplanman noplanman force-pushed the fix_message_cleanup branch from 242fd58 to e568ae5 Compare March 22, 2019 18:28
@noplanman noplanman force-pushed the fix_message_cleanup branch from e568ae5 to 82f3b63 Compare March 22, 2019 18:29
@noplanman
Copy link
Member

@jacklul Have reverted my changes and left yours, which does the trick 😉

Good to merge?

@jacklul
Copy link
Collaborator Author

jacklul commented Mar 23, 2019

Actually I think the subquery is not required and will only impact performance, works on my end!
Merge if it works for you too!

@noplanman
Copy link
Member

@jacklul Doesn't work for me, I get this:

Warning: PDO::query(): SQLSTATE[HY000]: General error: 1093 You can't specify target table 'message' for update in FROM clause in /vendor/longman/telegram-bot/src/Commands/AdminCommands/CleanupCommand.php on line 398

@jacklul jacklul force-pushed the fix_message_cleanup branch from 47b4c01 to 82f3b63 Compare March 23, 2019 18:18
@noplanman noplanman merged commit 663acfc into php-telegram-bot:develop Mar 23, 2019
@jacklul jacklul deleted the fix_message_cleanup branch March 23, 2019 18:23
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.

2 participants