Bug 4890 - SQLBasedAddrList.pm is not using persistent statement handles
Summary: SQLBasedAddrList.pm is not using persistent statement handles
Status: NEW
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Libraries (show other bugs)
Version: 3.1.1
Hardware: All Linux
: P4 enhancement
Target Milestone: Undefined
Assignee: SpamAssassin Developer Mailing List
Depends on:
Reported: 2006-05-02 05:10 UTC by Japheth Cleaver
Modified: 2007-11-23 19:35 UTC (History)
1 user (show)

Attachment Type Modified Status Actions Submitter/CLA Status
Old patch - use newer one patch None Japheth Cleaver [NoCLA]
v1.1 of patch to enable persistent SQL handles and statements patch None Japheth Cleaver [NoCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Japheth Cleaver 2006-05-02 05:10:58 UTC
Upon reviewing the code, I noticed that SQLBasedAddrList.pm is settin up and 
tearing down the UPDATE/SELECT/INSERT statements each time it issues a query on 
the AutoWhitelist. Although the database handle itself is persistent, a speedup 
can be achieved by making the statement handles persistent as well.
Comment 1 Japheth Cleaver 2006-05-02 05:12:41 UTC
Created attachment 3500 [details]
Old patch - use newer one
Comment 2 Japheth Cleaver 2006-05-02 20:01:57 UTC
Comment on attachment 3500 [details]
Old patch - use newer one

New version of patch posted below.
Comment 3 Japheth Cleaver 2006-05-02 20:06:52 UTC
Created attachment 3501 [details]
v1.1 of patch to enable persistent SQL handles and statements

Hmm.. turns out there are more changes needed to impliment this than I thought.
The database handles themselves are not Persistent, as the factory->finish()
routine is called after each use. In addition, the new_checker routine creates
a new DB handle each time.

I've created a newer patch that a) fixes those things in SQLBasedAddrList.pm
and b) passes the $main environment in AutoWhitelist.pm so that we can create
the database handle ONCE during the factory->new() call. Tested and working
properly for me.
Comment 4 Michael Parker 2006-05-02 20:09:51 UTC
Comment 5 Japheth Cleaver 2006-05-02 20:25:12 UTC
(In reply to comment #4)
> http://wiki.apache.org/spamassassin/DBIPlugin

I think these are complimentary means of dealing with the issue, not substitute 
(ie, both could be used).

Global DBI caching is indeed important, and this type of Ppugin will handle 
things like auto_reconnect and ping testing (auto_reconnect was something I 
didn't put in the patch above simply to make the patch cleaner and more 
understandable), but I also think that a "Mail::SpamAssassin::SQLBasedAddrList" 
file that subclasses off "Mail::SpamAssassin::PersistentAddrList" should be 
modified to not set up and tear down its statements and connections itself each 
time through -- especially since the DB configuration is <a 
definition, not changable on a per-user basis</a>.
Comment 6 Japheth Cleaver 2006-08-10 01:28:43 UTC
Any update on this bug? Patch still applies cleanly to 3.1.4.

Although a long-term DBI caching solution is still important, there's no need 
for new logical statement handles to be explicitly created each time through 
this Module. The current code seems like it will need to be changed eventually, 
and there is only one change outside of SQLBasedAddrList, which (shouldn't) 
affect anything else using the AutoWhitelist functions.
Comment 7 Justin Mason 2006-08-10 08:54:18 UTC
given that DBIPlugin cannot be put into the main distro, due to its incompatible
license, can we put this patch into trunk instead?  Michael?
Comment 8 Japheth Cleaver 2006-09-26 00:27:28 UTC
Sorry to keep pinging on this, but has there been any further consideration on 
this bug? The patch continues to apply cleanly to the most recent SA release 
(3.1.5) and has been in production at our site since this bug was opened.