In order to fix bug #28787 I commented out pendingproc_cleanup_pending_messages_db. According to the doc of this fucntion, that's just an optionnal optimisation so that's not the end of the world but we should fix it properly. Few things seem wrong to me in it: - SQL procedures can take a while. Is there any reason to block returning from ObserveChannels on this? If not, this function should be removed from the channel preparing chain. - tpl_channel_text_clean_up_stale_tokens() iterates on the tokens and call _tpl_log_store_sqlite_set_acknowledgment() for each token. So a new SQL command is executed for each token! I'm not a SQL guru but I'm sure we could create a single command operating on all the tokens. - According to pendingproc_cleanup_pending_messages_db's DEBUG message, this function is supposed to remove entries from the DB. But _tpl_log_store_sqlite_set_acknowledgment() does a SQL UPDATE so nothing is actually removed. Is that expected?
(In reply to comment #0) > In order to fix bug #28787 I commented out > pendingproc_cleanup_pending_messages_db. > According to the doc of this fucntion, that's just an optionnal optimisation so > that's not the end of the world but we should fix it properly. > > Few things seem wrong to me in it: > > - SQL procedures can take a while. Is there any reason to block returning from > ObserveChannels on this? If not, this function should be removed from the > channel preparing chain. I think the original goal was to be able to catch messages in-between receptions and message being acknowledged (which would generally occurs when the logger crash). But as we don't want to log them twice, we store the message ID associated with the channel pending list, and use a hash of different information including this ID to detect that message has been acknowledge. As soon as the message is acknowledged, there is no use of keeping the message in the pending message list (in database). > > - tpl_channel_text_clean_up_stale_tokens() iterates on the tokens and call > _tpl_log_store_sqlite_set_acknowledgment() for each token. So a new SQL command > is executed for each token! I'm not a SQL guru but I'm sure we could create a > single command operating on all the tokens. > > - According to pendingproc_cleanup_pending_messages_db's DEBUG message, this > function is supposed to remove entries from the DB. But > _tpl_log_store_sqlite_set_acknowledgment() does a SQL UPDATE so nothing is > actually removed. Is that expected? As explained, the implementation is off from the original goal. Instead of removing the messages from the pending message cache (stored to disc in case of crash) it simply clears the pending ID to mark the message as acknowledge. This extra step takes time, is indeed implemented the slow way and does not reduce the memory usage as pretended by the comment. To save the day, the SQLite log store clears older logs after a certain period of time, keeping it small. The pending message cache is important to avoid logging twice, or not logging at all messages when recovering from crash. I think this concept of pending message cache should be better abstracted, especially without coming call logs, where the message cache could be used to rebuild call event that where lost due to crash during call while recovery take place after the call has ended. If/when SQLite will become the main writable log-store, the need of separate message cache might disappear with the ability to store incomplete messages early and update them while more information are being retreived.
The pending message cache has been rewritten and this method have gone away.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.