Rspamd: statistics bayes insufficient number of learns

[beiDei8z]

Classification (Please choose one option):

• [ ] Crash/Hang/Data loss
• [ ] WebUI/Usability
• [X] Serious bug
• [ ] Other bug
• [ ] Feature
• [ ] Enhancement

Reproducibility (Please choose one option):

• [X] Always
• [ ] Sometimes
• [ ] Rarely
• [ ] Unable
• [ ] I didn’t try
• [ ] Not applicable

Rspamd version:

1.7.4

Description:

Bayes classification is taking place with insufficient data.

Steps to reproduce:

Engage Bayes classification with no saved bayes data.
Note the number and nature of messages received prior to start of classification.

Expected results:

No classification until sufficient learned data are collected for the message's user.
Users with no or minimal data should not have classification at the same point as all users. Each user identity should have its own counter. "min_learns" messages should be learnt per user before classification starts for the user.

Actual results:

BAYES_SPAM and BAYES_HAM are showing in the output when insufficient learned data have been collected. Classification is allowed for users simultaneously. Classification starts long before "min_learns" has been reach for all users.

Configuration:

local.d/classifier-bayes.conf

autolearn = [ 0, 10 ];
per_language = false;
languages_enabled = false;
min_learns = 1000;

per_user = <<EOD
return function(task)
    local rcpt = task:get_recipients(1)
    if rcpt then
        local one_rcpt = rcpt[1]
        local domain = one_rcpt['domain']
        if domain then
           return domain:lower():gsub("%.uk", ''):gsub("%.co", ''):gsub(".*%.", '')
        end
    end
    return nil
end
EOD

The "per_user" function maps: sub.DOMAIN.TLD => domain. I know all the forms of tld as I know the accounts.
"select * from users;" and "select count(*) from tokens WHERE user = N;" shows the data are being collected correctly per user. It also shows minimal learns for some users being bayes classified.

Additional information:

$ rspamc stat
...
Statfile: BAYES_SPAM type: sqlite3; length: 2.69M; free blocks: 0; total blocks: 37.73k; free: 0.00%; learned: 56; users: 5; languages: 1
Statfile: BAYES_HAM type: sqlite3; length: 3.91M; free blocks: 0; total blocks: 53.92k; free: 0.00%; learned: 84; users: 7; languages: 1

Total "learned" = 56 + 84 = 140. (Some may have been manual via rspamc).

output.txt

file "output" was generated with:

cat rspamd.log.0 rspamd.log | sed -n \
  -e '/skip classification as [^ ]* class has not enough learns/p' \
  -e '/rspamd_task_write_log.*BAYES/s/[^[]*\[\([^/]*\).* rcpts: <[^@]*@\([^.]*\).*/MESSAGE \2 BAYES \1/p' \
  -e '/rspamd_task_write_log/s/[^[]*\[\([^/]*\).* rcpts: <[^@]*@\([^.]*\).*/MESSAGE \2 NOT \1/p' \
  | sed -f ~/usermap > output

The usermap was created with...

$ rm -f usermap
$ echo "SELECT name FROM domain;" | psql -t $database | sed -e 's/.uk//' -e 's/.co//' -e 's/.*\.//' -e '/^[ ]*$/d' | sort | uniq | cat -n | \
while read index key
do
    echo "/^MESSAGE/s/ $key / $index /" >> usermap
    key=$(echo ${key} | tr '[:lower:]' '[:upper:]')
    echo "/^MESSAGE/s/ $key / $index /" >> usermap
done

...which disguises the users.

Note min_learns was adjusted from 200 to 1000 during the test to stop the premature classification.

In particular look at user 5

$ grep 'MESSAGE 5 ' output
MESSAGE 5 NOT 9.40
MESSAGE 5 NOT -0.30
MESSAGE 5 NOT 3.99
MESSAGE 5 NOT 6.39
MESSAGE 5 NOT 6.39
MESSAGE 5 NOT 0.90
MESSAGE 5 NOT 1.00
MESSAGE 5 NOT 19.30
MESSAGE 5 BAYES 10.84

There was just one message at ham threshold and one at spam before it started classifying.

It is not clear from the documentation:
https://rspamd.com/doc/configuration/statistic.html
what the units of "min_learns" are: number of emails identified as ham or spam? The sum of learned spam and ham? The number of tokens learned? ie, it's a "learn count" but of what? During the test period less then "min_learns" total emails were received, not even ham, spam or per user.

[beiDei8z]

Additional observations:

Restarting rspamd upsets the counters.

Table "users" in the sqlite Bayes data has a column "learns".

sqlite> .schema users
CREATE TABLE users(id INTEGER PRIMARY KEY,name TEXT,learns INTEGER);
CREATE UNIQUE INDEX un ON users(name);

This remains at zero.

sqlite> SELECT learns FROM users WHERE ID = 1;
0
sqlite> SELECT count(*) FROM tokens WHERE user = 1;
12296

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[beiDei8z]

The problem is still live. It's just been ignored.

[vstakhov]

I don't care about sqlite backend I'm afraid. So wontfix label is quite appropriate here. I will consider patches of course.

[beiDei8z]

When I looked at the code I couldn't see why it was particular to sqlite but it's easier for me to see using SQL, eg "SELECT count(*) FROM tokens WHERE user = 1;" in redis is?

I will revisit this again. Please allow me some time, rspamd is hard for a mortal like me to debug.

[beiDei8z]

Hello,

I have set up a test environment and found multiple problems not all of which relate to sqlite.

This prepared statement does not work:
"UPDATE languages SET learns=learns + 1 WHERE id = ?1;UPDATE users SET learns=learns + 1 WHERE id = ?2;"

If I substitute a value or a ?N and send the value it works. I can not see why if does not increment. it works from the command line.

Realising A workaround of sending the new value I looked for the statement that gets the learns from the database. I could increment in rspamd and send the new value to the DB in a second statement. This is no statement to get the learns per user so It would not matter if the value was save or not as it is not being read back.

There is RSPAMD_STAT_BACKEND_GET_LEARNS:
SELECT SUM(MAX(0, learns)) FROM languages;

but no:
SELECT SUM(MAX(0, learns)) FROM users;

The report:
bayes_classify: skip classification as ham class has not enough learns: 4, 200 required
is getting the value from somewhere but no per user from the database.

Summary: there is an sqlite problem that I don't understand but it's not the whole story. Work continues.

[beiDei8z]

I'll add the per user query would be:
SELECT learns FROM users WHERE id = ?1;
to be per user and not as for languages.

[beiDei8z]

Patch to fix one sqlite problem.

[beiDei8z]

"Body cannot not be blank" in 2nd attempt to attach patch file

[beiDei8z]

3rd...

[beiDei8z]

I give up, here it is pasted:

--- ../original/src/libstat/backends/sqlite3_backend.c 2019-03-12 15:56:30.000000000 +0000
+++ src/libstat/backends/sqlite3_backend.c 2019-03-29 16:59:27.626435273 +0000
@@ -84,8 +84,10 @@
RSPAMD_STAT_BACKEND_TRANSACTION_ROLLBACK,
RSPAMD_STAT_BACKEND_GET_TOKEN,
RSPAMD_STAT_BACKEND_SET_TOKEN,

• RSPAMD_STAT_BACKEND_INC_LEARNS,
• RSPAMD_STAT_BACKEND_DEC_LEARNS,
• RSPAMD_STAT_BACKEND_INC_LEARNS_LANG,
• RSPAMD_STAT_BACKEND_INC_LEARNS_USER,
• RSPAMD_STAT_BACKEND_DEC_LEARNS_LANG,
• RSPAMD_STAT_BACKEND_DEC_LEARNS_USER,
  RSPAMD_STAT_BACKEND_GET_LEARNS,
  RSPAMD_STAT_BACKEND_GET_LANGUAGE,
  RSPAMD_STAT_BACKEND_GET_USER,
  @@ -169,22 +171,38 @@
  .flags = 0,
  .ret = ""
  },
• [RSPAMD_STAT_BACKEND_INC_LEARNS] = {
• .idx = RSPAMD_STAT_BACKEND_INC_LEARNS,
• .sql = "UPDATE languages SET learns=learns + 1 WHERE id=?1;"
• "UPDATE users SET learns=learns + 1 WHERE id=?2;",
• [RSPAMD_STAT_BACKEND_INC_LEARNS_LANG] = {
• .idx = RSPAMD_STAT_BACKEND_INC_LEARNS_LANG,
• .sql = "UPDATE languages SET learns=learns + 1 WHERE id=?1;",
  .stmt = NULL,
• .args = "II",
• .args = "I",
  .result = SQLITE_DONE,
  .flags = 0,
  .ret = ""
  },
• [RSPAMD_STAT_BACKEND_DEC_LEARNS] = {
• .idx = RSPAMD_STAT_BACKEND_DEC_LEARNS,
• .sql = "UPDATE languages SET learns=MAX(0, learns - 1) WHERE id=?1;"
• "UPDATE users SET learns=MAX(0, learns - 1) WHERE id=?2;",
• [RSPAMD_STAT_BACKEND_INC_LEARNS_USER] = {
• .idx = RSPAMD_STAT_BACKEND_INC_LEARNS_USER,
• .sql = "UPDATE users SET learns=learns + 1 WHERE id=?1;",
  .stmt = NULL,
• .args = "II",
• .args = "I",
• .result = SQLITE_DONE,
• .flags = 0,
• .ret = ""
• },
• [RSPAMD_STAT_BACKEND_DEC_LEARNS_LANG] = {
• .idx = RSPAMD_STAT_BACKEND_DEC_LEARNS_LANG,
• .sql = "UPDATE languages SET learns=MAX(0, learns - 1) WHERE id=?1;",
• .stmt = NULL,
• .args = "I",
• .result = SQLITE_DONE,
• .flags = 0,
• .ret = ""
• },
• [RSPAMD_STAT_BACKEND_DEC_LEARNS_USER] = {
• .idx = RSPAMD_STAT_BACKEND_DEC_LEARNS_USER,
• .sql = "UPDATE users SET learns=MAX(0, learns - 1) WHERE id=?1;",
• .stmt = NULL,
• .args = "I",
  .result = SQLITE_DONE,
  .flags = 0,
  .ret = ""
  @@ -890,8 +908,11 @@
  g_assert (rt != NULL);
  bk = rt->db;
  rspamd_sqlite3_run_prstmt (task->task_pool, bk->sqlite, bk->prstmt,
• RSPAMD_STAT_BACKEND_INC_LEARNS,
• rt->lang_id, rt->user_id);
• RSPAMD_STAT_BACKEND_INC_LEARNS_LANG,
• rt->lang_id);
• rspamd_sqlite3_run_prstmt (task->task_pool, bk->sqlite, bk->prstmt,
• RSPAMD_STAT_BACKEND_INC_LEARNS_USER,
• rt->user_id);

if (bk->in_transaction) {
    rspamd_sqlite3_run_prstmt (task->task_pool, bk->sqlite, bk->prstmt,

@@ -916,8 +937,11 @@
g_assert (rt != NULL);
bk = rt->db;
rspamd_sqlite3_run_prstmt (task->task_pool, bk->sqlite, bk->prstmt,

• RSPAMD_STAT_BACKEND_DEC_LEARNS,
• rt->lang_id, rt->user_id);
• RSPAMD_STAT_BACKEND_DEC_LEARNS_LANG,
• rt->lang_id);
• rspamd_sqlite3_run_prstmt (task->task_pool, bk->sqlite, bk->prstmt,
• RSPAMD_STAT_BACKEND_DEC_LEARNS_USER,
• rt->user_id);

if (bk->in_transaction) {
    rspamd_sqlite3_run_prstmt (task->task_pool, bk->sqlite, bk->prstmt,

[vstakhov]

Can you make a pull request?

[beiDei8z]

There is a "pull request" in the links but it does nothing. Phew, how is something easy that's been possible for 20 years (a form post) made so hard? [Rhetorical]

[vstakhov]

You can always send a patch to the mailing list :)

[beiDei8z]

sqlite-rspamd-patch.txt

[vstakhov]

Ok, it makes sense. Actually, the original idea of per language classifiers was a mistake. Especially counting that you can make per-whatever classifier using Lua in per_user option.

[beiDei8z]

Success. My apologies. I had to email the file to myself and use VirtualBox.

There appears to be a problem executing multiple statements separated by semicolons, eg:
"SELECT 1; SELECT 2;"
This patch breaks the inc and dec into 2 statements. This is not the final solution but is a step to wards understanding the process. Sorry again for posts, it didn't take that long but I'm multitasking with cooking.

[beiDei8z]

This patch removes unneeded joins from the SQL. There is no functional change - neither good nor bad, those can follow.

rspamd-join-cleanup-patch.txt

[beiDei8z]

I have a patched rspamd working that respects per user learn counts however it makes functional changes so I will discuss the thinking.

Currently it is possible to specify a language and user. This gives language*users number of options but only number per language or per user is stored. It is thus impossible to get the number of learns per user-language with the current system. The get command uses:

  "AND (languages.id=?3 OR languages.id=0);",

so it is not using the selected languages purely anyway in always using lang=0 tokens. As per the comment above "the original idea of per language classifiers was a mistake" would it be best to remove the language selection from the backend?

I have always understood that the bayes process was language agnostic anyway and it just deals with strings of bytes. Latin or Cyrillic sources of the bytes makes no difference. This should be true of Chinese too perhaps depending on the tokeniser. Am I right that tokens are not split on word boundaries anyway? The redis backend does not handle languages so removal from sqlite will align and simplify rspamd. As noted above the per user can include a language part by setting the user variable to userlang if needed. (It would be possible to amalgamate the user and lang as user in the current database but I do not suggest doing it.)

I propose the language part is removed. This is a multi-phase operation, the first phase is simply to disable by using lang=0 always. Later the language data can be deleted, finally a new schma used, maybe with a new SQLITE3_SCHEMA_VERSION. Backward compatibility with users' existing stores is a priority.

[beiDei8z]

rspamd-define-patch.txt

redundant code and to stop compiler whinging.

[beiDei8z]

Patch to make user count work:
rspamd-user-count-patch.txt

[beiDei8z]

This patch makes per user stats works and solves this problem. This patch is for 1.9.1. I have been using it for 2 weeks and it appears to be fine. There are still several other questions/problems around this.

1. How or when are the hashes in learns, file learn_cache.sqlite, expired?
2. How or when are the bayes tokens expired?

The learn_cache lacks a timestamp and no where can I see a "DELETE FROM ..." statement.

I have added a timestamp, sqlite does not allow me to add a default on an additional column, so recreate:
\$ sqlite3 learn_cache.sqlite
DROP TABLE learns;
CREATE TABLE learns(id INTEGER PRIMARY KEY,flag INTEGER NOT NULL,digest TEXT NOT NULL,created TIMESTAMP DEFAULT (DATETIME('now')));
CREATE UNIQUE INDEX d ON learns(digest);

The learn count is incremented for each mail learned, this total is used to see if the number of learns is above the threshold for which it is deemed classification is viable or useful. In fact the threshold should be based on the tokens. I have written my own expiry which runs externally. This expires the tokens based on time and frequency count and sets the learns counts to a pseudo value. This will reduce the counts to below the thresholds if there are insufficient data remaining.

A periodic "VACUUM;" in sqlite reduces files sizes and probably aids speed.

[vstakhov]

The last patch is quite a slashing one as it removes languages support. It will cause lots of blaming I'm afraid. Also, why cannot you send pull requests??

[beiDei8z]

The logic for the patch was posted for discussion well before the patch.
What is a "pull request"? - I should expand as something may get lost in translation. See my earlier comment about a pull link that does nothing.

[supersophie]

I have had the same problem with version 1.9.4

[mundschenk-at]

What is a "pull request"?

https://help.github.com/en/articles/about-pull-requests

[beiDei8z]

What is a "pull request"?

https://help.github.com/en/articles/about-pull-requests

My comment was indeed lost in translation...

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[beiDei8z]

Fix offered. Refusal to adopt.

[mundschenk-at]

@beiDei8z The problem might be that you come across as unwilling to engage with current development practices (to put it charitably).

[beiDei8z]

As I'm the one doing the giving I suggest it not I who needs to engage. Cricky, anyone would think I was expecting a thank you for the effort I have put in.

[mundschenk-at]

Well, that attitude exemplifies the problem. What's your contribution compared to @vstakhov's?Using established development practices to not put additional burden on hime would be the reasonable and proper thing to do.

[beiDei8z]

It's just a reaction not the problem. The problem, if there is one, starts with vstakhov's comment "I don't care about sqlite backend.." and given this is the default for users it means I don't care about users or presumably the reputation of rspamd

Try "the original idea of per language classifiers was a mistake." then criticise for taking it out: "The last patch is quite a slashing one as it removes languages support. It will cause lots of blaming I'm afraid.". Make up your mind.

[supersophie]

Hi,

I am afraid this list can be an angry list with a sprinkling of arrogance and attitude. Of course there are some wonderfully helpful and amicable folk also listening. I tend to stay well away from the flames, having been burnt before.

Comments like these could be better phrased, and I wonder of he's not overloaded in some way:
" vstakhov's comment "I don't care about sqlite backend.." "

It's very frustrating.

All the best.

sent from phone, thus not too brief

On 3 Aug 2019, at 4:44 pm, beiDei8z [e-mail] wrote:

It's just a reaction not the problem. The problem, if there is one, starts with vstakhov's comment "I don't care about sqlite backend.." and given this is the default for users it means I don't care about users or presumable the reputation.

Try "the original idea of per language classifiers was a mistake." then criticise for taking it out: "The last patch is quite a slashing one as it removes languages support. It will cause lots of blaming I'm afraid.". Make up your mind.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[sudo-bmitch]

I'm still seeing this issue.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[KardinalReusen]

I'm also still seeing this issue.