[pmg-devel] [PATCH] fix #3164: api: quarantine: allow to return spam from all users

[pmg-devel] [PATCH] fix #3164: api: quarantine: allow to return spam from all users

[Thomas Lamprecht]

The pmail was only checked for the spam quarantine call, and there
mainly to ensure that the quarantine user only can check their own
mails. Make the pmail parameter also optional for this quarantine
related endpoint as long as one has a role other than quser.
This allows to query all spam quarantine entries from all pmails at
once, providing the backend side to address #3164.

The main argument against this was performance, but postgres can
handle even hundreds of thousands of rows rather fine, it's a high
performant database after all and this is quite the simple query (no
joins, functions on columns or nested queries).

Some data, 45k records on a read limited disk, gathered with EXPLAIN
ANALYZE commands:

All caches dropped and fresh start: 440ms
Running for a bit with caches warm:  55ms

A simple extrapolation would mean that for half a million rows we
would spent about 5s in the DB, which is not too bad considering our
hard limit of 30s per requests, and the overhead of perl/https seems
to put the limit on my not so beefy VM at at least ~1.5 million rows
from a *cold* cache, which seems plenty (default 7 days keep window
and an avg. of 10 spam mails per day means >21k qusers). And with
warm caches and a beefier machine one can probably gain one or even
two order of magnitudes here.

And at the end, no mail admin is forced to use this and if they run a
setup with tens of millions of spam in their spam-keep time window,
well, they really should not be surprised that querying all has a
certain cost.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PMG/API2/Quarantine.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
index 56f248d..666dffa 100644
--- a/src/PMG/API2/Quarantine.pm
+++ b/src/PMG/API2/Quarantine.pm
@@ -597,14 +597,14 @@ my $quarantine_api = sub {
 
     my $rpcenv = PMG::RESTEnvironment->get();
     my $authuser = $rpcenv->get_user();
+    my $role = $rpcenv->get_role();
 
     my $start = $param->{starttime} // (time - 86400);
     my $end = $param->{endtime} // ($start + 86400);
 
     my $select;
     my $pmail;
-    if ($check_pmail) {
-	my $role = $rpcenv->get_role();
+    if ($check_pmail || $role eq 'quser') {
 	$pmail = $verify_optional_pmail->($authuser, $role, $param->{pmail});
 	$select = "SELECT * " .
 		  "FROM CMailStore, CMSReceivers WHERE " .
@@ -700,7 +700,7 @@ __PACKAGE__->register_method ({
     },
     code => sub {
 	my ($param) = @_;
-	return $quarantine_api->($param, 'S', 1);
+	return $quarantine_api->($param, 'S', defined($param->{pmail}));
     }});
 
 __PACKAGE__->register_method ({
-- 
2.20.1

[pmg-devel] [PATCH] override deselection in CheckboxModel to improve performance

[Thomas Lamprecht]

After the UI hang for tens of seconds for a few thousand elements got
deselected I investigated with Firefox developer tools performance
analysis, where the waterfall view showed that most of the time was
spent in array splicing.

Previously, all removed elements got removed on by one from the
`selected` Ext.util.Collection - which is basically an helper class
around arrays and objects, most of it may have become obsolete with
modern browsers. The single remove resulted into further splicing of
the array, and all in all it resulted in a dramatically increased
complexity, ~ O(n^3).

The "remove one by one" logic comes highly probably from the fact
that users can register a `beforedeselection` listener which can
block a removal of a specific record. But, that's not used by us and
not really something one would often need in practice, but still its
a documented feature of ExtJS grids we want to keep; so go for an
alternative.
So, override `doDeselect` and change the old removal logic to one
that first record those entries which got blocked from removal and
remove them in one go from the "to-be-removed" collection.

Before/After this patch on my FF 86.0.1 with my i9-9900K and
deselecting ~10k records went from ~40s to about 33 ms total, so for
that case we went 1000x faster.

The remaining time is now mostly spend in the event fire/handling
logic, but even with 50k records we spent <<500ms in total, so not
too bad and thus kept as is for now.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---

Note, I had some more optimizations for the deselectAll case specifically, but
I omitted that as that one provided small extra benefits, so less
changes/overrides where favored.

 src/Toolkit.js | 66 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/src/Toolkit.js b/src/Toolkit.js
index 6dcccd3..79bc5ae 100644
--- a/src/Toolkit.js
+++ b/src/Toolkit.js
@@ -492,11 +492,75 @@ Ext.define('Proxmox.validIdReOverride', {
     validIdRe: /^[a-z_][a-z0-9\-_@]*$/i,
 });
 
-// use whole checkbox cell to multiselect, not only the checkbox
 Ext.define('Proxmox.selection.CheckboxModel', {
     override: 'Ext.selection.CheckboxModel',
 
+    // [P] use whole checkbox cell to multiselect, not only the checkbox
     checkSelector: '.x-grid-cell-row-checker',
+
+    // [ P: optimized to remove all records at once as single remove is O(n^3) slow ]
+    // records can be an index, a record or an array of records
+    doDeselect: function(records, suppressEvent) {
+        var me = this,
+            selected = me.selected,
+            i = 0,
+            len, record,
+            commit;
+        if (me.locked || !me.store) {
+            return false;
+        }
+        if (typeof records === "number") {
+            // No matching record, jump out
+            record = me.store.getAt(records);
+            if (!record) {
+                return false;
+            }
+            records = [
+                record,
+            ];
+        } else if (!Ext.isArray(records)) {
+            records = [
+                records,
+            ];
+        }
+	// [P] a beforedeselection, triggered by me.onSelectChange below, can block removal by
+	// returning false, thus the original implementation removed only here in the commit fn,
+	// which has an abysmal performance O(n^3). As blocking removal is not the norm, go do the
+	// reverse, record blocked records and remove them from the to-be-removed array before
+	// applying it. A FF86 i9-9900K on 10k records goes from >40s to ~33ms for >90% deselection
+	let committed = false;
+	commit = function() {
+	    committed = true;
+	    if (record === me.selectionStart) {
+		me.selectionStart = null;
+	    }
+	};
+	let removalBlocked = [];
+        len = records.length;
+        me.suspendChanges();
+        for (; i < len; i++) {
+            record = records[i];
+            if (me.isSelected(record)) {
+		committed = false;
+                me.onSelectChange(record, false, suppressEvent, commit);
+		if (!committed) {
+		    removalBlocked.push(record);
+		}
+                if (me.destroyed) {
+                    return false;
+                }
+            }
+        }
+	if (removalBlocked.length > 0) {
+	    records.remove(removalBlocked);
+	}
+	selected.remove(records); // [P] FAST(er)
+	me.lastSelected = selected.last();
+        me.resumeChanges();
+        // fire selchange if there was a change and there is no suppressEvent flag
+	me.maybeFireSelectionChange(records.length > 0 && !suppressEvent);
+	return records.length > 0;
+    },
 });
 
 // force alert boxes to be rendered with an Error Icon
-- 
2.20.1

[pmg-devel] [PATCH] fix #3164: allow one to display all quarantined spam mails

[Thomas Lamprecht]

If the API call returned more than one pmail entry, inject an "all"
entry which, if selected, drops the user parameter and loads the
quarantine mails of all users from the backend.

The webinterface has only some issues regarding deselection (all in
the grid header or if we need to deselect due to the search filtering
out some elements) - for that the underlying issue was found and a
widget toolkit patch was provided.

The rest seems now pretty performant, albeit more than a few 100k
mails may become a problem here. But, in such big setups the mail
admin won't tinker to much whith the users mail anyway, if they are
even alowed to do so depending on their jurisdictions privacy laws
and companies privacy policy.

So, basically this is more for evaluation or for smaller setups but
got quite often requested, and as there's not more data
exposed/returned then already available I see no real argument
against it.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 js/QuarantineList.js | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/js/QuarantineList.js b/js/QuarantineList.js
index c5cab6e..9280489 100644
--- a/js/QuarantineList.js
+++ b/js/QuarantineList.js
@@ -76,6 +76,10 @@ Ext.define('PMG.QuarantineList', {
 		}
 		me.setEmptyText();
 	    }
+	    // deselect all first, else ExtJS does some funky O(n^3) comparissions as it tries
+	    // to keep the selection, but we do not care for that on a new load anyway
+	    view.getSelectionModel().deselectAll();
+
 	    store.load(function() {
 		if (me.savedPosition !== undefined) {
 		    if (store.getCount() - 1 < me.savedPosition) {
@@ -112,7 +116,11 @@ Ext.define('PMG.QuarantineList', {
 	setUser: function(user) {
 	    let view = this.getView();
 	    let params = view.getStore().getProxy().getExtraParams();
-	    params.pmail = user;
+	    if (user === null) {
+		delete params.pmail;
+	    } else {
+		params.pmail = user;
+	    }
 	    view.getStore().getProxy().setExtraParams(params);
 	    view.user = user;
 	},
@@ -164,7 +172,11 @@ Ext.define('PMG.QuarantineList', {
 	    let me = this;
 	    me.savedPosition = undefined;
 	    me.allowPositionSave = false;
-	    me.setUser(value);
+	    if (value === 'all') {
+		me.setUser(null);
+	    } else {
+		me.setUser(value);
+	    }
 	    me.load();
 	},
 
@@ -214,7 +226,8 @@ Ext.define('PMG.QuarantineList', {
 		return match;
 	    });
 	    if (toDeselect.length > 0) {
-		sm.deselect(toDeselect);
+		sm.deselect(toDeselect, true);
+		sm.maybeFireSelectionChange(true);
 	    }
 	    return selectedRecordId !== null && clearSelectedMail;
 	},
@@ -313,6 +326,13 @@ Ext.define('PMG.QuarantineList', {
 			    renderer: Ext.htmlEncode,
 			},
 		    ],
+		    listeners: {
+			load: function(store, records, successfull) {
+			    if (successfull && records.length > 1) {
+				store.insert(0, { mail: 'all' });
+			    }
+			},
+		    },
 		},
 		queryMode: 'local',
 		editable: true,
-- 
2.20.1

Re: [pmg-devel] [PATCH] fix #3164: api: quarantine: allow to return spam from all users

[Stoiko Ivanov]

Huge thanks for tackling this feature quite a few users have asked and
have been waiting for!


On Mon, 22 Mar 2021 10:00:44 +0100
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

> The pmail was only checked for the spam quarantine call, and there
> mainly to ensure that the quarantine user only can check their own
> mails. Make the pmail parameter also optional for this quarantine
> related endpoint as long as one has a role other than quser.
> This allows to query all spam quarantine entries from all pmails at
> once, providing the backend side to address #3164.
> 
> The main argument against this was performance, but postgres can
> handle even hundreds of thousands of rows rather fine, it's a high
> performant database after all and this is quite the simple query (no
> joins, functions on columns or nested queries).
agreed @postgres being a performant dbms - small nit - the quarantine
query has a join (it's quering cmsreceivers and cmailstore in one query)

On my machine a 30k mail test-set even showed some room for improvement
with the postgres parameters (sorting resorting to a temp-file for <
10Megs sounds like a bad tradeoff) - however this is a different topic and
as you said - users with huge deployments using quarantine, hopefully know
how to tweak a few postgres parameters.


quickly gave your patches a spin - it works quite well on my test-setup
(currently at 48k mails selecting 'all' takes 5s in the GUI (and yields
10.4MB of results)

The changes to pmg-api look good!
The ones to pmg-gui and proxmox-widet-toolkit as well (but I'm not the
best judge of java-script code)

with that said:
Tested-By: Stoiko Ivanov <s.ivanov@proxmox.com>
Reviewed-By: Stoiko Ivanov <s.ivanov@proxmox.com>


> 
> Some data, 45k records on a read limited disk, gathered with EXPLAIN
> ANALYZE commands:
> 
> All caches dropped and fresh start: 440ms
> Running for a bit with caches warm:  55ms
> 
> A simple extrapolation would mean that for half a million rows we
> would spent about 5s in the DB, which is not too bad considering our
> hard limit of 30s per requests, and the overhead of perl/https seems
> to put the limit on my not so beefy VM at at least ~1.5 million rows
> from a *cold* cache, which seems plenty (default 7 days keep window
> and an avg. of 10 spam mails per day means >21k qusers). And with
> warm caches and a beefier machine one can probably gain one or even
> two order of magnitudes here.
> 
> And at the end, no mail admin is forced to use this and if they run a
> setup with tens of millions of spam in their spam-keep time window,
> well, they really should not be surprised that querying all has a
> certain cost.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> ---
>  src/PMG/API2/Quarantine.pm | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
> index 56f248d..666dffa 100644
> --- a/src/PMG/API2/Quarantine.pm
> +++ b/src/PMG/API2/Quarantine.pm
> @@ -597,14 +597,14 @@ my $quarantine_api = sub {
>  
>      my $rpcenv = PMG::RESTEnvironment->get();
>      my $authuser = $rpcenv->get_user();
> +    my $role = $rpcenv->get_role();
>  
>      my $start = $param->{starttime} // (time - 86400);
>      my $end = $param->{endtime} // ($start + 86400);
>  
>      my $select;
>      my $pmail;
> -    if ($check_pmail) {
> -	my $role = $rpcenv->get_role();
> +    if ($check_pmail || $role eq 'quser') {
>  	$pmail = $verify_optional_pmail->($authuser, $role, $param->{pmail});
>  	$select = "SELECT * " .
>  		  "FROM CMailStore, CMSReceivers WHERE " .
> @@ -700,7 +700,7 @@ __PACKAGE__->register_method ({
>      },
>      code => sub {
>  	my ($param) = @_;
> -	return $quarantine_api->($param, 'S', 1);
> +	return $quarantine_api->($param, 'S', defined($param->{pmail}));
>      }});
>  
>  __PACKAGE__->register_method ({

Re: [pmg-devel] [PATCH] fix #3164: api: quarantine: allow to return spam from all users

[Fuhrmann, Markus]

[-- Attachment #1: Type: text/plain, Size: 5745 bytes --]

Yes! Thanks for your support and the implementation of this long awaited feature!

Regards,
Markus








Unternehmensgruppe Nassauische Heimstätte | Wohnstadt: Geschäftsführung: Dr. Thomas Hain (Leitender Geschäftsführer), Monika Fontaine-Kretschmer, Dr. Constantin Westphal: Nassauische Heimstätte GmbH | Schaumainkai 47 | 60596 Frankfurt am Main | Registergericht Frankfurt am Main | HRB 6712 | Aufsichtsratsvorsitz: Staatsminister Tarek Al-Wazir, NH ProjektStadt GmbH Schaumainkai 47 | 60596 Frankfurt am Main | Registergericht Frankfurt am Main | HRB 97395 | Aufsichtsratsvorsitz: Staatsminister Tarek Al-Wazir, Bauland-Offensive Hessen GmbH | Schaumainkai 47 | 60596 Frankfurt am Main | Registergericht Frankfurt am Main | HRB 109334 | Aufsichtsratsvorsitz: Staatsminister Tarek Al-Wazir, Wohnstadt GmbH | Wolfsschlucht 18 | 34117 Kassel | Registergericht Kassel | HRB 2157, MET GmbH | Wolfsschlucht 18 | 34117 Kassel | Registergericht Kassel | HRB 5898

Diese E-Mail ist vertraulich. Wenn Sie nicht der rechtmäßige Empfänger sind, dürfen Sie den Inhalt weder kopieren, verbreiten noch benutzen. Sollten Sie diese E-Mail versehentlich erhalten haben, informieren Sie mich bitte via E-Mail-Antwort und löschen Sie sie anschließend. Danke!



-----Ursprüngliche Nachricht-----
Von: pmg-devel <pmg-devel-bounces@lists.proxmox.com> Im Auftrag von Stoiko Ivanov
Gesendet: Montag, 22. März 2021 15:06
An: Thomas Lamprecht <t.lamprecht@proxmox.com>
Cc: pmg-devel@lists.proxmox.com
Betreff: Re: [pmg-devel] [PATCH] fix #3164: api: quarantine: allow to return spam from all users

Huge thanks for tackling this feature quite a few users have asked and have been waiting for!


On Mon, 22 Mar 2021 10:00:44 +0100
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

> The pmail was only checked for the spam quarantine call, and there
> mainly to ensure that the quarantine user only can check their own
> mails. Make the pmail parameter also optional for this quarantine
> related endpoint as long as one has a role other than quser.
> This allows to query all spam quarantine entries from all pmails at
> once, providing the backend side to address #3164.
>
> The main argument against this was performance, but postgres can
> handle even hundreds of thousands of rows rather fine, it's a high
> performant database after all and this is quite the simple query (no
> joins, functions on columns or nested queries).
agreed @postgres being a performant dbms - small nit - the quarantine query has a join (it's quering cmsreceivers and cmailstore in one query)

On my machine a 30k mail test-set even showed some room for improvement with the postgres parameters (sorting resorting to a temp-file for < 10Megs sounds like a bad tradeoff) - however this is a different topic and as you said - users with huge deployments using quarantine, hopefully know how to tweak a few postgres parameters.


quickly gave your patches a spin - it works quite well on my test-setup (currently at 48k mails selecting 'all' takes 5s in the GUI (and yields 10.4MB of results)

The changes to pmg-api look good!
The ones to pmg-gui and proxmox-widet-toolkit as well (but I'm not the best judge of java-script code)

with that said:
Tested-By: Stoiko Ivanov <s.ivanov@proxmox.com>
Reviewed-By: Stoiko Ivanov <s.ivanov@proxmox.com>


>
> Some data, 45k records on a read limited disk, gathered with EXPLAIN
> ANALYZE commands:
>
> All caches dropped and fresh start: 440ms Running for a bit with
> caches warm:  55ms
>
> A simple extrapolation would mean that for half a million rows we
> would spent about 5s in the DB, which is not too bad considering our
> hard limit of 30s per requests, and the overhead of perl/https seems
> to put the limit on my not so beefy VM at at least ~1.5 million rows
> from a *cold* cache, which seems plenty (default 7 days keep window
> and an avg. of 10 spam mails per day means >21k qusers). And with warm
> caches and a beefier machine one can probably gain one or even two
> order of magnitudes here.
>
> And at the end, no mail admin is forced to use this and if they run a
> setup with tens of millions of spam in their spam-keep time window,
> well, they really should not be surprised that querying all has a
> certain cost.
>
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> ---
>  src/PMG/API2/Quarantine.pm | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
> index 56f248d..666dffa 100644
> --- a/src/PMG/API2/Quarantine.pm
> +++ b/src/PMG/API2/Quarantine.pm
> @@ -597,14 +597,14 @@ my $quarantine_api = sub {
>
>      my $rpcenv = PMG::RESTEnvironment->get();
>      my $authuser = $rpcenv->get_user();
> +    my $role = $rpcenv->get_role();
>
>      my $start = $param->{starttime} // (time - 86400);
>      my $end = $param->{endtime} // ($start + 86400);
>
>      my $select;
>      my $pmail;
> -    if ($check_pmail) {
> -	my $role = $rpcenv->get_role();
> +    if ($check_pmail || $role eq 'quser') {
>  	$pmail = $verify_optional_pmail->($authuser, $role, $param->{pmail});
>  	$select = "SELECT * " .
>  		  "FROM CMailStore, CMSReceivers WHERE " .
> @@ -700,7 +700,7 @@ __PACKAGE__->register_method ({
>      },
>      code => sub {
>  	my ($param) = @_;
> -	return $quarantine_api->($param, 'S', 1);
> +	return $quarantine_api->($param, 'S', defined($param->{pmail}));
>      }});
>
>  __PACKAGE__->register_method ({



_______________________________________________
pmg-devel mailing list
pmg-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 7407 bytes --]

Re: [pmg-devel] [PATCH] fix #3164: allow one to display all quarantined spam mails

[Dominik Csapak]

like discussed off-list, LGTM

Reviewed-By: Dominik Csapak <d.csapak@proxmox.com>

On 3/22/21 10:00, Thomas Lamprecht wrote:
> If the API call returned more than one pmail entry, inject an "all"
> entry which, if selected, drops the user parameter and loads the
> quarantine mails of all users from the backend.
> 
> The webinterface has only some issues regarding deselection (all in
> the grid header or if we need to deselect due to the search filtering
> out some elements) - for that the underlying issue was found and a
> widget toolkit patch was provided.
> 
> The rest seems now pretty performant, albeit more than a few 100k
> mails may become a problem here. But, in such big setups the mail
> admin won't tinker to much whith the users mail anyway, if they are
> even alowed to do so depending on their jurisdictions privacy laws
> and companies privacy policy.
> 
> So, basically this is more for evaluation or for smaller setups but
> got quite often requested, and as there's not more data
> exposed/returned then already available I see no real argument
> against it.
> 
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> ---
>   js/QuarantineList.js | 26 +++++++++++++++++++++++---
>   1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/js/QuarantineList.js b/js/QuarantineList.js
> index c5cab6e..9280489 100644
> --- a/js/QuarantineList.js
> +++ b/js/QuarantineList.js
> @@ -76,6 +76,10 @@ Ext.define('PMG.QuarantineList', {
>   		}
>   		me.setEmptyText();
>   	    }
> +	    // deselect all first, else ExtJS does some funky O(n^3) comparissions as it tries
> +	    // to keep the selection, but we do not care for that on a new load anyway
> +	    view.getSelectionModel().deselectAll();
> +
>   	    store.load(function() {
>   		if (me.savedPosition !== undefined) {
>   		    if (store.getCount() - 1 < me.savedPosition) {
> @@ -112,7 +116,11 @@ Ext.define('PMG.QuarantineList', {
>   	setUser: function(user) {
>   	    let view = this.getView();
>   	    let params = view.getStore().getProxy().getExtraParams();
> -	    params.pmail = user;
> +	    if (user === null) {
> +		delete params.pmail;
> +	    } else {
> +		params.pmail = user;
> +	    }
>   	    view.getStore().getProxy().setExtraParams(params);
>   	    view.user = user;
>   	},
> @@ -164,7 +172,11 @@ Ext.define('PMG.QuarantineList', {
>   	    let me = this;
>   	    me.savedPosition = undefined;
>   	    me.allowPositionSave = false;
> -	    me.setUser(value);
> +	    if (value === 'all') {
> +		me.setUser(null);
> +	    } else {
> +		me.setUser(value);
> +	    }
>   	    me.load();
>   	},
>   
> @@ -214,7 +226,8 @@ Ext.define('PMG.QuarantineList', {
>   		return match;
>   	    });
>   	    if (toDeselect.length > 0) {
> -		sm.deselect(toDeselect);
> +		sm.deselect(toDeselect, true);
> +		sm.maybeFireSelectionChange(true);
>   	    }
>   	    return selectedRecordId !== null && clearSelectedMail;
>   	},
> @@ -313,6 +326,13 @@ Ext.define('PMG.QuarantineList', {
>   			    renderer: Ext.htmlEncode,
>   			},
>   		    ],
> +		    listeners: {
> +			load: function(store, records, successfull) {
> +			    if (successfull && records.length > 1) {
> +				store.insert(0, { mail: 'all' });
> +			    }
> +			},
> +		    },
>   		},
>   		queryMode: 'local',
>   		editable: true,
> 

Re: [pmg-devel] [PATCH] fix #3164: api: quarantine: allow to return spam from all users

[Thomas Lamprecht]

On 22.03.21 15:06, Stoiko Ivanov wrote:
> Huge thanks for tackling this feature quite a few users have asked and
> have been waiting for!
> 
> 
> On Mon, 22 Mar 2021 10:00:44 +0100
> Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> 
>> The pmail was only checked for the spam quarantine call, and there
>> mainly to ensure that the quarantine user only can check their own
>> mails. Make the pmail parameter also optional for this quarantine
>> related endpoint as long as one has a role other than quser.
>> This allows to query all spam quarantine entries from all pmails at
>> once, providing the backend side to address #3164.
>>
>> The main argument against this was performance, but postgres can
>> handle even hundreds of thousands of rows rather fine, it's a high
>> performant database after all and this is quite the simple query (no
>> joins, functions on columns or nested queries).
> agreed @postgres being a performant dbms - small nit - the quarantine
> query has a join (it's quering cmsreceivers and cmailstore in one query)
> 

you're naturally right, the psql explain even said so - but I did that one
only after writing above paragraph so forgot to adapt it.

> On my machine a 30k mail test-set even showed some room for improvement
> with the postgres parameters (sorting resorting to a temp-file for <
> 10Megs sounds like a bad tradeoff) - however this is a different topic and
> as you said - users with huge deployments using quarantine, hopefully know
> how to tweak a few postgres parameters.
> 
> 
> quickly gave your patches a spin - it works quite well on my test-setup
> (currently at 48k mails selecting 'all' takes 5s in the GUI (and yields
> 10.4MB of results)
> 
> The changes to pmg-api look good!
> The ones to pmg-gui and proxmox-widet-toolkit as well (but I'm not the
> best judge of java-script code)
> 
> with that said:
> Tested-By: Stoiko Ivanov <s.ivanov@proxmox.com>
> Reviewed-By: Stoiko Ivanov <s.ivanov@proxmox.com>
> 

Much thanks for looking at this, applied with above tags.