Re: [pmg-devel] [PATCH pmg-api v2 1/1] fix #3450: api: queue: add POST endpoint for batch deletion/delivery

[Hannes Laimer <h.laimer@proxmox.com>]

Thanks for taking a look! two comments inline, and I'll send a v3

On 23.09.25 15:22, Thomas Lamprecht wrote:
> Am 23.09.25 um 11:33 schrieb Hannes Laimer:
>> Delivery is done using `postqueue -i <id>`, this is slower than using
>> `postsuper -r -`. But `postsuper -r -` would only re-queue the mails,
>> they'd also recieve new IDs. Flush should rather be an immediate delivery
>> attempt.
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>> for 2000 mails flush took ~8s, since 2000 is the max we can do at once
>> through the UI I think this is ok. Alternatively we could also disable
>> the Flush button if more than 200 or so a selected, but I don't think
>> there is much value in that.
> 
> Such info is fine to have recorded in the commit message
> 
>>
>> v2:
>> - single POST endpoitn that takes type(delete/deliver) and list of id's
> 
> Fine for delivery, but why not add a new optional ids parameter to the
> existing DELETE endpoint?
> 

To have it mentioned here, the reason is that
DELETE doesn't take data, at least our proxy doesn't, and something like
`.../{queue}/?ids=AABC234,DDEF4543,EEFG6473,...`
is at best odd and brittle.

I should have included this and I'll add it for v3.

> I mean, I have no strong objection against this, but I'd at least like
> to have a reason recorded in the commit message for why that route was
> not chosen.
> 
>>
>>   src/PMG/API2/Postfix.pm | 43 +++++++++++++++++++++++++++++++++++++++++
>>   src/PMG/Postfix.pm      | 33 +++++++++++++++++++++++++++++++
>>   2 files changed, 76 insertions(+)
>>
>> diff --git a/src/PMG/API2/Postfix.pm b/src/PMG/API2/Postfix.pm
>> index ba0689c..90a74e9 100644
>> --- a/src/PMG/API2/Postfix.pm
>> +++ b/src/PMG/API2/Postfix.pm
>> @@ -335,6 +335,49 @@ __PACKAGE__->register_method({
>>       },
>>   });
>>   
>> +__PACKAGE__->register_method({
>> +    name => 'queue_action',
>> +    path => 'queue/{queue}',
>> +    method => 'POST',
>> +    description => "Perform an action on the given queue IDs (delete/deliver).",
>> +    proxyto => 'node',
>> +    permissions => { check => ['admin'] },
>> +    protected => 1,
>> +    parameters => {
>> +        additionalProperties => 0,
>> +        properties => {
>> +            node => get_standard_option('pve-node'),
>> +            queue => $queue_name_option,
>> +            action => {
>> +                description => 'Operation to perform on the given IDs.',
>> +                type => 'string',
>> +                enum => ['delete', 'deliver'],
>> +            },
>> +            id => {
> 
> "ids" would slightly better signal that this can be more than one.
> 
>> +                description => 'Queue ID(s), separated by semicolons (;).',
>> +                type => 'string',
>> +                pattern => '[A-Za-z0-9]{8,20}(;[A-Za-z0-9]{8,20})*',
> 
> hmm, don't we have a format for this or at least an existing regex
> we can reuse?
> 

not really, there's only
```
my $queue_id_option = {
     description => "The Message queue ID.",
     type => 'string',
     pattern => '[a-zA-Z0-9]+',
     minLength => 8,
     maxLength => 20,
};
```
but I don't think a thing for just `[a-zA-Z0-9]+` adds much.

>> +            },
>> +        },
>> +    },
>> +    returns => { type => 'null' },
>> +    code => sub {
>> +        my ($param) = @_;
>> +
>> +        my @ids = defined($param->{id}) && length($param->{id})
>> +            ? split(/\s*;\s*/, $param->{id})
>> +            : ();
> 
> I'd prefer using PVE::Tools' split_list method, which handles most edge
> cases already.
> 
>> +
>> +        if ($param->{action} eq 'delete') {
>> +            PMG::Postfix::delete_queue_ids($param->{queue}, \@ids);
>> +        } elsif ($param->{action} eq 'deliver') {
>> +            PMG::Postfix::flush_queue_ids(\@ids);
>> +        }
>> +
>> +        return undef;
>> +    },
>> +});
>> +
>>   __PACKAGE__->register_method({
>>       name => 'delete_queue',
>>       path => 'queue/{queue}',
>> diff --git a/src/PMG/Postfix.pm b/src/PMG/Postfix.pm
>> index 966130f..dff8ec6 100644
>> --- a/src/PMG/Postfix.pm
>> +++ b/src/PMG/Postfix.pm
>> @@ -221,6 +221,39 @@ sub delete_queue {
>>       PVE::Tools::run_command($cmd);
>>   }
>>   
>> +# delete multiple mails by queue IDs
>> +sub delete_queue_ids {
>> +    my ($queue, $ids) = @_;
>> +
>> +    return if !$ids || ref($ids) ne 'ARRAY' || !@$ids;
>> +
>> +    my %seen;
>> +    my @queue_ids;
>> +    foreach my $qid (@$ids) {
> 
> please prefer for over foreach for new code.
> 
>> +        next if !$qid;
>> +        next if $seen{$qid}++;
>> +        push @queue_ids, $qid;
>> +    }
>> +
>> +    return if !@queue_ids;
>> +
>> +    my $input = join("\n", @queue_ids) . "\n";
> 
> I mean, if you already iterate above, why not assemble the input string
> directly there? While sometimes it can be nicer for code readability to
> use a intermediate array This is short enough to still be easy enough to
> read and grasp.
> 
> And btw. as of now you have the $id 4 times in memory:
> 
> 1. in $ids
> 2. in %seen (well deduplicated, but still)
> 3. in @queue_ids
> 4. in $input
> 
> If you want to cope with duplicates then it would be probably even better
> to just do
> 
> my $unique_qids = { map { $_ => 1 } $ids->@* };
> 
> my $input = join("\n", keys $unique_qids->%*) . "\n";
> 
> That avoids at least one copy.
> 
>> +    my $cmd = ['/usr/sbin/postsuper', '-d', '-'];
>> +    push @$cmd, $queue if defined($queue);
>> +    PVE::Tools::run_command($cmd, input => $input);
>> +}
>> +
>> +# flush for multiple queue IDs
>> +sub flush_queue_ids {
>> +    my ($ids) = @_;
>> +
>> +    return if !$ids || ref($ids) ne 'ARRAY' || !@$ids;
>> +
>> +    foreach my $qid (@$ids) {
> 
> s/foreach/for/
> 
>> +        PVE::Tools::run_command(['/usr/sbin/postqueue', '-i', $qid]);
>> +    }
>> +}
>> +
>>   sub discard_verify_cache {
>>       unlink "/var/lib/postfix/verify_cache.db";
>>   
> 



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