Mysql MemberAdaptor 1.61 and Mailman 2.1.6

Hello all. I have been working with Kev Green's MysqlMemberships.py Adaptor version 1.61 (SourceForge.net Mailman patch ID 839386) and Mailman 2.1.6 with Python 2.3.4. So far the adaptor has worked fairly well (with a minor patch to deal with one of the change made in Mailman 2.1.4, I believe, and the addition of a test to prevent the creation of unnecessary tables in the MySQL database - so this isn't a completely plain 1.61 version).
However, there appears to be a bug/compatibility issue with this adapter concerning setting bounce information for list members. This process appears to be handled by a function called setBounceInfo in the adaptor. As it works now, setBounceInfo only successfully sets initial values in the MySQL database but fails to update subsequent bounce information. The bounce log seems to support these findings. It will report that the score was set to 2.0 but the MySQL database will still show a score of 1.0 for a bouncing list member... as a result members cannot receive bounce scores higher than 1.0!
I am not proficient in Python and don't completely understand how Mailman operates so I'm interested in finding some help to understand how information generated by registerBounce in Bouncer.py is supposed to reach setBounceInfo in MysqlMemberships.py. Even a general understanding of how bounce information is processed in Mailman would be helpful for investigating this.
Thank you, -Adrian

"Adrian Wells" <adrian@whatifnet.com> on Monday, October 24, 2005 at 1:56 PM +0000 wrote:
After some time and further testing... it seems that the Mysql MemberAdaptor maybe OK after all, but it is not being fully utilized (or any other member adaptor, for that matter)... The function registerBounce only calls setBounceInfo once (line 116: "self.setBounceInfo(member, info)"). This occurs after testing whether "this is the first bounce we've seen from this member". It would seem as though setBounceInfo should be called a few more times if other conditions are met, right? For example, after determining that the bounce information for a member is valid and is not stale? As a result, I've created a patch that seems to correct the unexpected behavior mentioned in my earlier message. This patch may not cover recording when probes occur or how many probes remain (for example in sendNextNotification). --- Bouncer.py.10.25.2005 2005-10-25 12:21:57.000000000 -0400 +++ Bouncer.py 2005-10-25 13:21:02.000000000 -0400 @@ -137,6 +137,7 @@ if lastbounce + self.bounce_info_stale_after < now: # Information is stale, so simply reset it info.reset(weight, day, self.bounce_you_are_disabled_warnings) + self.setBounceInfo(member, info) syslog('bounce', '%s: %s has stale bounce info, resetting', self.internal_name(), member) else: @@ -144,6 +145,7 @@ # score and take any necessary action. info.score += weight info.date = day + self.setBounceInfo(member, info) syslog('bounce', '%s: %s current bounce score: %s', member, self.internal_name(), info.score) # Continue to the check phase below Please let me know if this is not good or will otherwise cause problems down the line. As a minor side note, I noticed the bounce log receives two different formatted messages for the first bounce and subsequent bounces. An example: ... Oct 25 10:50:51 2005 (2687) samplelist: falseaddresstest@somedomain.net bounce score: 1.0 Oct 25 11:06:54 2005 (2687) falseaddresstest@somedomain.net: samplelist current bounce score: 2.0 ... This is not a major issue but it is inconsistent and it not clear why it should be this way. Is there reason is should be different? Finally, the Mysql MemberAdaptor has a __del__() method. However, it doesn't seem like this is utilized. Searching the Mailman developer's mailing list archives yielded comments from Barry stating that such a method is "not a reliable way to free external resources because you really don't know when Python will call it it, but in this case it might work okay (and may be the only option without some hacking. ;)" <http://www.mail-archive.com/mailman-developers@python.org/msg06810.html>. I'm curious, what kind of hacking would be required to reliably close connections? For the sake of full disclosure, I did make a minor change to the MysqlMemberships.py but this should not have affected the issue concerning storing subsequent bounce information. Here is a patch containing for the change made in the adaptor: --- MysqlMemberships.py.10.25.2005 2005-10-25 12:31:02.000000000 -0400 +++ MysqlMemberships.py 2005-10-25 13:14:41.000000000 -0400 @@ -969,8 +969,8 @@ except MySQLdb.Warning, e: syslog("error", "MySQL update warning setting Delivery Status info to '%s' for member '%s' in setBounceInfo()" % (status, member) ) else: - self._prodServerConnection try: + self._prodServerConnection # Hack the dates to work with MySQL. lnsql=(info.lastnotice[0],info.lastnotice[1],info.lastnotice[2],0,0,0,0,0,0) lnsql = time.strftime("%Y-%m-%d", lnsql) -Adrian

Adrian Wells wrote:
I think you are correct about the MysqlMemberships.py MemberAdaptor in particular, but Mailman with OldStyleMemberships.py clearly does record subsequent bounces. See below for more.
I've been looking at this off and on since your first post. I'm kind of "the new kid on the block" here, so even though I think I understand what's going on, I'm not clear on the best way to 'fix' it. What is happening is this. Bouncer.registerBounce calls getBounceInfo to get the bounce info for the member. If there is no bounce info for the member, getBounceInfo returns None and registerBounce creates an instance of the _BounceInfo class and calls setBounceInfo to save it. If there is existing bounce info for the member, getBounceInfo returns the appropriate _BounceInfo class instance which contains the member's info for this list. registerBounce then proceeds to update some attributes of this _BounceInfo instance. Now the tricky part is that in the OldStyleMemberships case, the member _BounceInfo instance is an item in a list of _BounceInfo instances which is the bounce_info attribute of the Mailman list itself. Thus the _BounceInfo instance returned by getBounceInfo is in a sense a pointer into the bounce_info list attribute so when registerBounce changes attributes of the _BounceInfo instance, it is also changing the lists bounce_info attribute so when Save() is ultimately called for the list, the updated bounce info is actually saved. Now, MysqlMemberships.py doesn't work in the same way. its setBounceInfo and getBounceInfo methods take the attributes out of the _BounceInfo instance and store them separately in the database and vice versa, so saving the list doesn't commit any changes that registerBounces may have made to the _BounceInfo instance.
It looks good to me, but as you recognize, it's incomplete. As I said, I'm the new kid on the block. It seems to me that this fix is the right way to go, but others may differ. I've worked up a more complete patch which is pasted to the end of this mail. It addresses the other places where the bounce info is changed. I've also searched for places outside Bouncer.py where bounce info is used, and I think they are all OK as is.
I don't think so. All the other log messages from Bouncer are "list: member". I don't see any reason why this one shouldn't also be that way. Here's my patch - watch out for wrapped lines. --- mailman-2.1.6/Mailman/Bouncer.py 2004-12-03 21:01:11.000000000 -0800 +++ mailman-mas/Mailman/Bouncer.py 2005-10-26 12:41:37.984375000 -0700 @@ -146,6 +146,10 @@ info.date = day syslog('bounce', '%s: %s current bounce score: %s', member, self.internal_name(), info.score) + # We've changed info above. In case the MemberAdaptor + # stores bounce info externally to the list, we need + # to tell it to update + self.setBounceInfo(member, info) # Continue to the check phase below # # Now that we've adjusted the bounce score for this bounce, let's @@ -166,6 +170,9 @@ # first bounce, it'll expire by the time we get the disabling bounce. cookie = self.pend_new(Pending.RE_ENABLE, self.internal_name(), member) info.cookie = cookie + # In case the MemberAdaptor stores bounce info externally to + # the list, we need to tell it to save the cookie + self.setBounceInfo(member, info) # Disable them if mm_cfg.VERP_PROBES: syslog('bounce', '%s: %s disabling due to probe bounce received', @@ -271,6 +278,9 @@ msg.send(self) info.noticesleft -= 1 info.lastnotice = time.localtime()[:3] + # In case the MemberAdaptor stores bounce info externally to + # the list, we need to tell it to update + self.setBounceInfo(member, info) def BounceMessage(self, msg, msgdata, e=None): # Bounce a message back to the sender, with an error message if -- Mark Sapiro <msapiro@value.net> The highway is for gamblers, San Francisco Bay Area, California better use your sense - B. Dylan

As an aside question I would ask: do you notice speed improvement by switching to MySQL-based membership? I have a big list of ~180k subscribers and unfortunately it is now *very* difficult to use the web interface to unsubscribe people.
Right now I have to resort to command-line instructions and it's not very practical. I wonder if I should not go the MySQL way, but I'm a bit worried about taking this risk to my databases (I'd hate to reconstruct 70+ lists from a backup). Especially if the switch does not bring a solution.
I'd be happy to get advice and maybe even some help if things turn bad, from people who know this piece of patch.
-- Fil

Mark Sapiro <msapiro@value.net> on Wednesday, October 26, 2005 at 3:45 PM +0000 wrote:
Thank you for the reply. I had not looked much at OldStyleMemberships.py as it's replaced by the MysqlMemberships.py MemberAdaptor in MailList.py, and I'm continuing to slowly learn how Mailman operates. It turns out there is a bug with MysqlMemberships.py MemberAdaptor which deals with retrieving bounce info cookie which is externally stored (something I learned after reading your helpful comments). I've included the patch for MysqlMemberships.py MemberAdaptor at the end of this message.
"The new kid on the block"... this sounds like a bit of an understatement but I'll have to try to take your word for it. I'm not sure the best way to 'fix' this either hence the initial post to this list.
What is happening is this.
[ snipped helpful and detailed explanation ] Thank you for the helpful and detailed explanation.
OK. I imagine that not much can easily done to change this.
[ snipped my first patch attempt and comments about it]
Thank you for looking over the patch and for providing a more complete patch. Today, I also found the additional sections in which bounce info is changed (as covered by your patch). However, I think there's a couple additional sections that the supplied patch misses - those are when the bounce information is reset (info.reset()). So I've included another patch at the end of this message which seems to be even more complete.
OK. Should this be entered as a bug on SF?
Here's my patch - watch out for wrapped lines.
[ snipped Mark's more complete patch for the sake of brevity (or at least an attempt at it) ] Here's the possibly even more complete patch (as you noted earlier, watch for wrapped lines) for Bouncer.py: --- Bouncer.py.10.25.2005 2005-10-25 12:21:57.000000000 -0400 +++ Bouncer.py 2005-10-26 17:28:46.000000000 -0400 @@ -137,6 +137,10 @@ if lastbounce + self.bounce_info_stale_after < now: # Information is stale, so simply reset it info.reset(weight, day, self.bounce_you_are_disabled_warnings) + # We've changed info above. In case the MemberAdaptor + # stores bounce info externally to the list, we need + # to tell it to update + self.setBounceInfo(member, info) syslog('bounce', '%s: %s has stale bounce info, resetting', self.internal_name(), member) else: @@ -144,6 +148,10 @@ # score and take any necessary action. info.score += weight info.date = day + # We've changed info above. In case the MemberAdaptor + # stores bounce info externally to the list, we need + # to tell it to update + self.setBounceInfo(member, info) syslog('bounce', '%s: %s current bounce score: %s', member, self.internal_name(), info.score) # Continue to the check phase below @@ -158,6 +166,10 @@ self.bounce_score_threshold) self.sendProbe(member, msg) info.reset(0, info.date, info.noticesleft) + # We've changed info above. In case the MemberAdaptor + # stores bounce info externally to the list, we need + # to tell it to update + self.setBounceInfo(member, info) else: self.disableBouncingMember(member, info, msg) @@ -166,6 +178,9 @@ # first bounce, it'll expire by the time we get the disabling bounce. cookie = self.pend_new(Pending.RE_ENABLE, self.internal_name(), member) info.cookie = cookie + # In case the MemberAdaptor stores bounce info externally to + # the list, we need to tell it to save the cookie + self.setBounceInfo(member, info) # Disable them if mm_cfg.VERP_PROBES: syslog('bounce', '%s: %s disabling due to probe bounce received', @@ -271,6 +286,9 @@ msg.send(self) info.noticesleft -= 1 info.lastnotice = time.localtime()[:3] + # In case the MemberAdaptor stores bounce info externally to + # the list, we need to tell it to update + self.setBounceInfo(member, info) def BounceMessage(self, msg, msgdata, e=None): # Bounce a message back to the sender, with an error message if This is the patch for the MysqlMemberships.py MemberAdaptor (note this patch was generated against an already patched/modified version of the Mysql MemberAdaptor 1.61): --- MysqlMemberships.py.10.26.2005 2005-10-26 15:07:54.000000000 -0400 +++ MysqlMemberships.py 2005-10-26 15:11:48.000000000 -0400 @@ -499,7 +499,8 @@ DAYOFMONTH(bi_lastnotice), YEAR(bi_date), MONTH(bi_date), - DAYOFMONTH(bi_date) + DAYOFMONTH(bi_date), + bi_cookie FROM mailman_mysql WHERE listname='%s' AND address = '%s'""" @@ -513,7 +514,8 @@ DAYOFMONTH(bi_lastnotice), YEAR(bi_date), MONTH(bi_date), - DAYOFMONTH(bi_date) + DAYOFMONTH(bi_date), + bi_cookie FROM %s WHERE address = '%s'""" %( self.__mlist.internal_name(), MySQLdb.escape_string(member) ) ) @@ -528,6 +530,7 @@ # Otherwise, populate a bounce_info structure. bounce_info = _BounceInfo(member, row[0], (row[5],row[6],row[7]), row[1]) bounce_info.lastnotice = (row[2],row[3],row[4]) + bounce_info.cookie = row[8] return bounce_info # -Adrian

Adrian Wells wrote:
I think that's right. The MemberAdaptor is not supposed to be in the business of determining when a _BounceInfo instance has changed behind its back and divining when to commit changes to it. The documentation in MemberAdaptor.py says that the getBounceInfo() method returns the info that was set with setBounceInfo(), so except for what you discovered about the cookie, MysqlMemberships.py appears to be doing the right thing at this level.
Actually, it is not really doing the right thing because it is not supposed to be aware of what's in the _BounceInfo class. The info that is passed to it is a string representation of the _BounceInfo instance, and it should really just be saving and retrieving that. IMO, there should be just one column in the MySQL table for this string representation. The only possible snag I see is that the string contains new-lines, and I don't know MySQL so I don't know if new-lines are allowed in a string field/column.
If MysqlMemberships.py were just storing and retrieving the representation that it is passed, it wouldn't have to worry about things like the fact that the 'cookie' argument disappeared from the _BounceInfo instantiation call in Mailman 2.1.4
Yes. I definitely overlooked the info.reset() two lines before the end of registerBounce. Good Catch!
However in the earlier part of registerBounce, I deliberately combined your two calls to setBounceInfo() in the "if info is stale" clause and its "else" clause into a single call following the if - else but still within the containing else.
I did this even though I think it is logically equivalent, because I think that all else equal, fewer lines is better.
Yes, I think so, but I'd be inclined to wait a bit and see if there are more comments from the list.
As I indicate above, I think the better way to fix MysqlMemberships.py is to remove its knowledge of the _BounceInfo class and just save and retrieve the string representation that it is handed.
-- Mark Sapiro <msapiro@value.net> The highway is for gamblers, San Francisco Bay Area, California better use your sense - B. Dylan

On 10/26/05 6:06 PM, "Mark Sapiro" <msapiro@value.net> wrote:
Based on these tests dashed off using one of Exim's debugging capabilities $ exim -be the newlines are OK but have to be quoted (as do CR characters, and others).
This, of course, assumes that Exim's quote_mysql operator is doing the right thing.
The best thing would be to check the MySQL documentation (which I'm too lazy to do this evening).
--John

"John W. Baxter" <jwblist@olympus.net> on Thursday, October 27, 2005 at 1:22 AM +0000 wrote:
Thank you John for the examples and suggestion to reference the MySQL documentation.
It appears as though the MySQL VARCHAR type can preserve newlines <http://dev.mysql.com/doc/refman/4.1/en/char.html>. However trailing space is removed in this data type. If trailing space must be preserved, one could use the MySQL TEXT type <http://dev.mysql.com/doc/refman/4.1/en/blob.html>.
-Adrian

Adrian Wells wrote:
There is no trailing white space in the string representation of a _BounceInfo instance. See the __repr__ method in the _BounceInfo class in Bouncer.py for details of what it is.
There is an issue however in that the representation can exceed 255 characters if there is a cookie and/or a longish member address.
The first reference above seems unclear on this. It says
Values in VARCHAR columns are variable-length strings. The length can be specified as 1 to 255 before MySQL 4.0.2 and 0 to 255 as of MySQL 4.0.2.
but the next paragraph says
In contrast to CHAR, VARCHAR values are stored using only as many characters as are needed, plus one byte to record the length (two bytes for columns that are declared with a length longer than 255).
I don't know if means you can actually have VARCHAR data longer than 255 or not.
-- Mark Sapiro <msapiro@value.net> The highway is for gamblers, San Francisco Bay Area, California better use your sense - B. Dylan

A lot of good work on this MySQLMemberAdaptor! If you think it can make things easier and faster I'm willing to open a subversion repository to develop Mailman patches and plugins. Please tell. (I already have one running multiple projects on my server, so one more cannot hurt. It uses trac as a web frontend)
-- Fil

On Thu, 2005-10-27 at 09:25 -0700, Mark Sapiro wrote:
VARCHAR is upper bound limited, you cannot store more than 255 characters in a VARCHAR. However the TEXT data type are virtually unlimited (as is the BLOB, or Binary Large Object, don't you love the name?) But since the bounce data is converted to text the TEXT data type makes more sense than BLOB.
John Dennis <jdennis@redhat.com>

Mark Sapiro <msapiro@value.net> on Wednesday, October 26, 2005 at 9:06 PM +0000 wrote:
So, if I understand this, ideally, Bouncer.py should not be changed to include additional calls to setBounceInfo(), right? I'm still trying to understand whether this is a hack to allow MysqlMemberships.py to work as is (more or less) OR if setBounceInfo() calls were original "missing" in Bouncer.py. I gather the answer is the former.
I have done some searching in the mailman-developers' archives to try to understand why it was decided to separate BounceInfo. Here are some findings:
<http://www.mail-archive.com/mailman-developers@python.org/msg06790.html>: "I'm putting the "info" parameter from setBounceInfo directly into the database, which I think is an array itself, not a single value, and the above doesn't look like Python's just traversing an array, and dumping it into the database(the LHS names don't tie up with what I think are the keys for the subelements of "info"), so it looks like I'll have to take a "best guess" at how to implement this."
<http://www.mail-archive.com/mailman-developers@python.org/msg06806.html>: "...the only changes of any import that I've made are that the Member data structures are stored in a way that fits MySQL and converted as they are loaded to the way that fits Mailman, which you'd expect..."
I surmise that the rationale for storing the BounceInfo in separate columns is to provide easier access via SQL queries to the information that would otherwise be stored in this object. I can imagine where this would be desirable (e.g. quickly querying which members recently received an increased bounce score).
Agreed. Fewer lines is preferred. I apologize for not recognizing what you had done there.
I have no problem waiting.
<http://www.mail-archive.com/mailman-developers@python.org/msg06808.html>: "My suggestion would be to pickle the BounceInfo object on the way into the database, and unpickle it on the way out."
Or pickle and unpickle this information, right? Making this change, of course, will require more effort to extract information stored in MySQL for other purposes (e.g. a custom web interface) but if it's the best way to handle this information then I would consider making these changes. I will try like to discuss this with the original author of MysqlMemberships.py.
-Adrian

Adrian Wells wrote:
No. I think the additional calls to setBounceInfo() are required for any MemberAdaptor that doesn't store the bounce info in a list attribute.
I.e., they ARE required for MysqlMemberships.py, but they should be minimized because for MysqlMemberships.py and perhaps other MemberAdaptors they involve database access which may be relatively expensive. I think the patch we arrived at does achieve the minimum.
The issue with the existing MysqlMemberships.py is that it should not be burdened with knowing any details about the bounce info. As the documentation (in MemberAdaptor.py) says, bounce info is opaque to the MemberAdaptor. It is set by setBounceInfo() and returned by getBounceInfo() without modification.
Obviously, the MemberAdaptor has to know enough about the bounce info it gets (e.g., maximum length) so it can store and return it without modification, and getBounceInfo() has to know to return None when there is no previous bounce info for the member, but that should be it. The lengths to which MysqlMemberships.py goes to extract attributes from the bounce info, save them separately, and construct a _BounceInfo instance to return the data only get it in trouble when aspects of the _BounceInfo class change from version to version.
I can see that would be desirable, but the price is difficulty of maintenance because then the get and set BounceInfo methods have to know things about the _BounceInfo class that may change, however see below for a compromise.
No problem.
See below.
Looking at this more deeply, I think it is not as simple as I first thought to simply save the string representation and return it. I think pickle.dump() to a StringIO file and then saving its contents in the set... method and then retrieving the string and returning pickle.loads() in the get... method is the way to go. It has the advantage that the string written to the database is a bit shorter too.
If you also want to be able to see and use some of the bounce info, you could save certain attributes of the _BounceInfo instance in additional columns of the database table.
I think discussing with the author is a good idea.
-- Mark Sapiro <msapiro@value.net> The highway is for gamblers, San Francisco Bay Area, California better use your sense - B. Dylan

We are currently two on irc on the #mailman channel trying to install this patch on our respective systems. Could anyone drop by and/or tell us where to find the latest series of updated files?
thanks in advance
-- Fil

There is an issue with the "flat" mode, when you create the db according to the README file and choose PRIMARY KEY (listname, address)
MySQL brings up the following error : ERROR 1071: Specified key was too long. Max key length is 500
So in fact the initial definitions of address varchar(255) NOT NULL, listname varchar(255) NOT NULL,
do not work, you need to get under the 500 limit (I chose to kepp 255 for the address, and 100 for the listname)
-- Fil

Okay I found out how to install this stuff for just one list (test list): use extend.py mechanism with:
lists/test/extend.py containing the following lines:
# override the default for this list def extend(mlist): mlist._memberadaptor = MysqlMemberships(mlist) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
hope this helps
-- Fil

"Adrian Wells" <adrian@whatifnet.com> on Monday, October 24, 2005 at 1:56 PM +0000 wrote:
After some time and further testing... it seems that the Mysql MemberAdaptor maybe OK after all, but it is not being fully utilized (or any other member adaptor, for that matter)... The function registerBounce only calls setBounceInfo once (line 116: "self.setBounceInfo(member, info)"). This occurs after testing whether "this is the first bounce we've seen from this member". It would seem as though setBounceInfo should be called a few more times if other conditions are met, right? For example, after determining that the bounce information for a member is valid and is not stale? As a result, I've created a patch that seems to correct the unexpected behavior mentioned in my earlier message. This patch may not cover recording when probes occur or how many probes remain (for example in sendNextNotification). --- Bouncer.py.10.25.2005 2005-10-25 12:21:57.000000000 -0400 +++ Bouncer.py 2005-10-25 13:21:02.000000000 -0400 @@ -137,6 +137,7 @@ if lastbounce + self.bounce_info_stale_after < now: # Information is stale, so simply reset it info.reset(weight, day, self.bounce_you_are_disabled_warnings) + self.setBounceInfo(member, info) syslog('bounce', '%s: %s has stale bounce info, resetting', self.internal_name(), member) else: @@ -144,6 +145,7 @@ # score and take any necessary action. info.score += weight info.date = day + self.setBounceInfo(member, info) syslog('bounce', '%s: %s current bounce score: %s', member, self.internal_name(), info.score) # Continue to the check phase below Please let me know if this is not good or will otherwise cause problems down the line. As a minor side note, I noticed the bounce log receives two different formatted messages for the first bounce and subsequent bounces. An example: ... Oct 25 10:50:51 2005 (2687) samplelist: falseaddresstest@somedomain.net bounce score: 1.0 Oct 25 11:06:54 2005 (2687) falseaddresstest@somedomain.net: samplelist current bounce score: 2.0 ... This is not a major issue but it is inconsistent and it not clear why it should be this way. Is there reason is should be different? Finally, the Mysql MemberAdaptor has a __del__() method. However, it doesn't seem like this is utilized. Searching the Mailman developer's mailing list archives yielded comments from Barry stating that such a method is "not a reliable way to free external resources because you really don't know when Python will call it it, but in this case it might work okay (and may be the only option without some hacking. ;)" <http://www.mail-archive.com/mailman-developers@python.org/msg06810.html>. I'm curious, what kind of hacking would be required to reliably close connections? For the sake of full disclosure, I did make a minor change to the MysqlMemberships.py but this should not have affected the issue concerning storing subsequent bounce information. Here is a patch containing for the change made in the adaptor: --- MysqlMemberships.py.10.25.2005 2005-10-25 12:31:02.000000000 -0400 +++ MysqlMemberships.py 2005-10-25 13:14:41.000000000 -0400 @@ -969,8 +969,8 @@ except MySQLdb.Warning, e: syslog("error", "MySQL update warning setting Delivery Status info to '%s' for member '%s' in setBounceInfo()" % (status, member) ) else: - self._prodServerConnection try: + self._prodServerConnection # Hack the dates to work with MySQL. lnsql=(info.lastnotice[0],info.lastnotice[1],info.lastnotice[2],0,0,0,0,0,0) lnsql = time.strftime("%Y-%m-%d", lnsql) -Adrian

Adrian Wells wrote:
I think you are correct about the MysqlMemberships.py MemberAdaptor in particular, but Mailman with OldStyleMemberships.py clearly does record subsequent bounces. See below for more.
I've been looking at this off and on since your first post. I'm kind of "the new kid on the block" here, so even though I think I understand what's going on, I'm not clear on the best way to 'fix' it. What is happening is this. Bouncer.registerBounce calls getBounceInfo to get the bounce info for the member. If there is no bounce info for the member, getBounceInfo returns None and registerBounce creates an instance of the _BounceInfo class and calls setBounceInfo to save it. If there is existing bounce info for the member, getBounceInfo returns the appropriate _BounceInfo class instance which contains the member's info for this list. registerBounce then proceeds to update some attributes of this _BounceInfo instance. Now the tricky part is that in the OldStyleMemberships case, the member _BounceInfo instance is an item in a list of _BounceInfo instances which is the bounce_info attribute of the Mailman list itself. Thus the _BounceInfo instance returned by getBounceInfo is in a sense a pointer into the bounce_info list attribute so when registerBounce changes attributes of the _BounceInfo instance, it is also changing the lists bounce_info attribute so when Save() is ultimately called for the list, the updated bounce info is actually saved. Now, MysqlMemberships.py doesn't work in the same way. its setBounceInfo and getBounceInfo methods take the attributes out of the _BounceInfo instance and store them separately in the database and vice versa, so saving the list doesn't commit any changes that registerBounces may have made to the _BounceInfo instance.
It looks good to me, but as you recognize, it's incomplete. As I said, I'm the new kid on the block. It seems to me that this fix is the right way to go, but others may differ. I've worked up a more complete patch which is pasted to the end of this mail. It addresses the other places where the bounce info is changed. I've also searched for places outside Bouncer.py where bounce info is used, and I think they are all OK as is.
I don't think so. All the other log messages from Bouncer are "list: member". I don't see any reason why this one shouldn't also be that way. Here's my patch - watch out for wrapped lines. --- mailman-2.1.6/Mailman/Bouncer.py 2004-12-03 21:01:11.000000000 -0800 +++ mailman-mas/Mailman/Bouncer.py 2005-10-26 12:41:37.984375000 -0700 @@ -146,6 +146,10 @@ info.date = day syslog('bounce', '%s: %s current bounce score: %s', member, self.internal_name(), info.score) + # We've changed info above. In case the MemberAdaptor + # stores bounce info externally to the list, we need + # to tell it to update + self.setBounceInfo(member, info) # Continue to the check phase below # # Now that we've adjusted the bounce score for this bounce, let's @@ -166,6 +170,9 @@ # first bounce, it'll expire by the time we get the disabling bounce. cookie = self.pend_new(Pending.RE_ENABLE, self.internal_name(), member) info.cookie = cookie + # In case the MemberAdaptor stores bounce info externally to + # the list, we need to tell it to save the cookie + self.setBounceInfo(member, info) # Disable them if mm_cfg.VERP_PROBES: syslog('bounce', '%s: %s disabling due to probe bounce received', @@ -271,6 +278,9 @@ msg.send(self) info.noticesleft -= 1 info.lastnotice = time.localtime()[:3] + # In case the MemberAdaptor stores bounce info externally to + # the list, we need to tell it to update + self.setBounceInfo(member, info) def BounceMessage(self, msg, msgdata, e=None): # Bounce a message back to the sender, with an error message if -- Mark Sapiro <msapiro@value.net> The highway is for gamblers, San Francisco Bay Area, California better use your sense - B. Dylan

As an aside question I would ask: do you notice speed improvement by switching to MySQL-based membership? I have a big list of ~180k subscribers and unfortunately it is now *very* difficult to use the web interface to unsubscribe people.
Right now I have to resort to command-line instructions and it's not very practical. I wonder if I should not go the MySQL way, but I'm a bit worried about taking this risk to my databases (I'd hate to reconstruct 70+ lists from a backup). Especially if the switch does not bring a solution.
I'd be happy to get advice and maybe even some help if things turn bad, from people who know this piece of patch.
-- Fil

Mark Sapiro <msapiro@value.net> on Wednesday, October 26, 2005 at 3:45 PM +0000 wrote:
Thank you for the reply. I had not looked much at OldStyleMemberships.py as it's replaced by the MysqlMemberships.py MemberAdaptor in MailList.py, and I'm continuing to slowly learn how Mailman operates. It turns out there is a bug with MysqlMemberships.py MemberAdaptor which deals with retrieving bounce info cookie which is externally stored (something I learned after reading your helpful comments). I've included the patch for MysqlMemberships.py MemberAdaptor at the end of this message.
"The new kid on the block"... this sounds like a bit of an understatement but I'll have to try to take your word for it. I'm not sure the best way to 'fix' this either hence the initial post to this list.
What is happening is this.
[ snipped helpful and detailed explanation ] Thank you for the helpful and detailed explanation.
OK. I imagine that not much can easily done to change this.
[ snipped my first patch attempt and comments about it]
Thank you for looking over the patch and for providing a more complete patch. Today, I also found the additional sections in which bounce info is changed (as covered by your patch). However, I think there's a couple additional sections that the supplied patch misses - those are when the bounce information is reset (info.reset()). So I've included another patch at the end of this message which seems to be even more complete.
OK. Should this be entered as a bug on SF?
Here's my patch - watch out for wrapped lines.
[ snipped Mark's more complete patch for the sake of brevity (or at least an attempt at it) ] Here's the possibly even more complete patch (as you noted earlier, watch for wrapped lines) for Bouncer.py: --- Bouncer.py.10.25.2005 2005-10-25 12:21:57.000000000 -0400 +++ Bouncer.py 2005-10-26 17:28:46.000000000 -0400 @@ -137,6 +137,10 @@ if lastbounce + self.bounce_info_stale_after < now: # Information is stale, so simply reset it info.reset(weight, day, self.bounce_you_are_disabled_warnings) + # We've changed info above. In case the MemberAdaptor + # stores bounce info externally to the list, we need + # to tell it to update + self.setBounceInfo(member, info) syslog('bounce', '%s: %s has stale bounce info, resetting', self.internal_name(), member) else: @@ -144,6 +148,10 @@ # score and take any necessary action. info.score += weight info.date = day + # We've changed info above. In case the MemberAdaptor + # stores bounce info externally to the list, we need + # to tell it to update + self.setBounceInfo(member, info) syslog('bounce', '%s: %s current bounce score: %s', member, self.internal_name(), info.score) # Continue to the check phase below @@ -158,6 +166,10 @@ self.bounce_score_threshold) self.sendProbe(member, msg) info.reset(0, info.date, info.noticesleft) + # We've changed info above. In case the MemberAdaptor + # stores bounce info externally to the list, we need + # to tell it to update + self.setBounceInfo(member, info) else: self.disableBouncingMember(member, info, msg) @@ -166,6 +178,9 @@ # first bounce, it'll expire by the time we get the disabling bounce. cookie = self.pend_new(Pending.RE_ENABLE, self.internal_name(), member) info.cookie = cookie + # In case the MemberAdaptor stores bounce info externally to + # the list, we need to tell it to save the cookie + self.setBounceInfo(member, info) # Disable them if mm_cfg.VERP_PROBES: syslog('bounce', '%s: %s disabling due to probe bounce received', @@ -271,6 +286,9 @@ msg.send(self) info.noticesleft -= 1 info.lastnotice = time.localtime()[:3] + # In case the MemberAdaptor stores bounce info externally to + # the list, we need to tell it to update + self.setBounceInfo(member, info) def BounceMessage(self, msg, msgdata, e=None): # Bounce a message back to the sender, with an error message if This is the patch for the MysqlMemberships.py MemberAdaptor (note this patch was generated against an already patched/modified version of the Mysql MemberAdaptor 1.61): --- MysqlMemberships.py.10.26.2005 2005-10-26 15:07:54.000000000 -0400 +++ MysqlMemberships.py 2005-10-26 15:11:48.000000000 -0400 @@ -499,7 +499,8 @@ DAYOFMONTH(bi_lastnotice), YEAR(bi_date), MONTH(bi_date), - DAYOFMONTH(bi_date) + DAYOFMONTH(bi_date), + bi_cookie FROM mailman_mysql WHERE listname='%s' AND address = '%s'""" @@ -513,7 +514,8 @@ DAYOFMONTH(bi_lastnotice), YEAR(bi_date), MONTH(bi_date), - DAYOFMONTH(bi_date) + DAYOFMONTH(bi_date), + bi_cookie FROM %s WHERE address = '%s'""" %( self.__mlist.internal_name(), MySQLdb.escape_string(member) ) ) @@ -528,6 +530,7 @@ # Otherwise, populate a bounce_info structure. bounce_info = _BounceInfo(member, row[0], (row[5],row[6],row[7]), row[1]) bounce_info.lastnotice = (row[2],row[3],row[4]) + bounce_info.cookie = row[8] return bounce_info # -Adrian

Adrian Wells wrote:
I think that's right. The MemberAdaptor is not supposed to be in the business of determining when a _BounceInfo instance has changed behind its back and divining when to commit changes to it. The documentation in MemberAdaptor.py says that the getBounceInfo() method returns the info that was set with setBounceInfo(), so except for what you discovered about the cookie, MysqlMemberships.py appears to be doing the right thing at this level.
Actually, it is not really doing the right thing because it is not supposed to be aware of what's in the _BounceInfo class. The info that is passed to it is a string representation of the _BounceInfo instance, and it should really just be saving and retrieving that. IMO, there should be just one column in the MySQL table for this string representation. The only possible snag I see is that the string contains new-lines, and I don't know MySQL so I don't know if new-lines are allowed in a string field/column.
If MysqlMemberships.py were just storing and retrieving the representation that it is passed, it wouldn't have to worry about things like the fact that the 'cookie' argument disappeared from the _BounceInfo instantiation call in Mailman 2.1.4
Yes. I definitely overlooked the info.reset() two lines before the end of registerBounce. Good Catch!
However in the earlier part of registerBounce, I deliberately combined your two calls to setBounceInfo() in the "if info is stale" clause and its "else" clause into a single call following the if - else but still within the containing else.
I did this even though I think it is logically equivalent, because I think that all else equal, fewer lines is better.
Yes, I think so, but I'd be inclined to wait a bit and see if there are more comments from the list.
As I indicate above, I think the better way to fix MysqlMemberships.py is to remove its knowledge of the _BounceInfo class and just save and retrieve the string representation that it is handed.
-- Mark Sapiro <msapiro@value.net> The highway is for gamblers, San Francisco Bay Area, California better use your sense - B. Dylan

On 10/26/05 6:06 PM, "Mark Sapiro" <msapiro@value.net> wrote:
Based on these tests dashed off using one of Exim's debugging capabilities $ exim -be the newlines are OK but have to be quoted (as do CR characters, and others).
This, of course, assumes that Exim's quote_mysql operator is doing the right thing.
The best thing would be to check the MySQL documentation (which I'm too lazy to do this evening).
--John

"John W. Baxter" <jwblist@olympus.net> on Thursday, October 27, 2005 at 1:22 AM +0000 wrote:
Thank you John for the examples and suggestion to reference the MySQL documentation.
It appears as though the MySQL VARCHAR type can preserve newlines <http://dev.mysql.com/doc/refman/4.1/en/char.html>. However trailing space is removed in this data type. If trailing space must be preserved, one could use the MySQL TEXT type <http://dev.mysql.com/doc/refman/4.1/en/blob.html>.
-Adrian

Adrian Wells wrote:
There is no trailing white space in the string representation of a _BounceInfo instance. See the __repr__ method in the _BounceInfo class in Bouncer.py for details of what it is.
There is an issue however in that the representation can exceed 255 characters if there is a cookie and/or a longish member address.
The first reference above seems unclear on this. It says
Values in VARCHAR columns are variable-length strings. The length can be specified as 1 to 255 before MySQL 4.0.2 and 0 to 255 as of MySQL 4.0.2.
but the next paragraph says
In contrast to CHAR, VARCHAR values are stored using only as many characters as are needed, plus one byte to record the length (two bytes for columns that are declared with a length longer than 255).
I don't know if means you can actually have VARCHAR data longer than 255 or not.
-- Mark Sapiro <msapiro@value.net> The highway is for gamblers, San Francisco Bay Area, California better use your sense - B. Dylan

A lot of good work on this MySQLMemberAdaptor! If you think it can make things easier and faster I'm willing to open a subversion repository to develop Mailman patches and plugins. Please tell. (I already have one running multiple projects on my server, so one more cannot hurt. It uses trac as a web frontend)
-- Fil

On Thu, 2005-10-27 at 09:25 -0700, Mark Sapiro wrote:
VARCHAR is upper bound limited, you cannot store more than 255 characters in a VARCHAR. However the TEXT data type are virtually unlimited (as is the BLOB, or Binary Large Object, don't you love the name?) But since the bounce data is converted to text the TEXT data type makes more sense than BLOB.
John Dennis <jdennis@redhat.com>

Mark Sapiro <msapiro@value.net> on Wednesday, October 26, 2005 at 9:06 PM +0000 wrote:
So, if I understand this, ideally, Bouncer.py should not be changed to include additional calls to setBounceInfo(), right? I'm still trying to understand whether this is a hack to allow MysqlMemberships.py to work as is (more or less) OR if setBounceInfo() calls were original "missing" in Bouncer.py. I gather the answer is the former.
I have done some searching in the mailman-developers' archives to try to understand why it was decided to separate BounceInfo. Here are some findings:
<http://www.mail-archive.com/mailman-developers@python.org/msg06790.html>: "I'm putting the "info" parameter from setBounceInfo directly into the database, which I think is an array itself, not a single value, and the above doesn't look like Python's just traversing an array, and dumping it into the database(the LHS names don't tie up with what I think are the keys for the subelements of "info"), so it looks like I'll have to take a "best guess" at how to implement this."
<http://www.mail-archive.com/mailman-developers@python.org/msg06806.html>: "...the only changes of any import that I've made are that the Member data structures are stored in a way that fits MySQL and converted as they are loaded to the way that fits Mailman, which you'd expect..."
I surmise that the rationale for storing the BounceInfo in separate columns is to provide easier access via SQL queries to the information that would otherwise be stored in this object. I can imagine where this would be desirable (e.g. quickly querying which members recently received an increased bounce score).
Agreed. Fewer lines is preferred. I apologize for not recognizing what you had done there.
I have no problem waiting.
<http://www.mail-archive.com/mailman-developers@python.org/msg06808.html>: "My suggestion would be to pickle the BounceInfo object on the way into the database, and unpickle it on the way out."
Or pickle and unpickle this information, right? Making this change, of course, will require more effort to extract information stored in MySQL for other purposes (e.g. a custom web interface) but if it's the best way to handle this information then I would consider making these changes. I will try like to discuss this with the original author of MysqlMemberships.py.
-Adrian

Adrian Wells wrote:
No. I think the additional calls to setBounceInfo() are required for any MemberAdaptor that doesn't store the bounce info in a list attribute.
I.e., they ARE required for MysqlMemberships.py, but they should be minimized because for MysqlMemberships.py and perhaps other MemberAdaptors they involve database access which may be relatively expensive. I think the patch we arrived at does achieve the minimum.
The issue with the existing MysqlMemberships.py is that it should not be burdened with knowing any details about the bounce info. As the documentation (in MemberAdaptor.py) says, bounce info is opaque to the MemberAdaptor. It is set by setBounceInfo() and returned by getBounceInfo() without modification.
Obviously, the MemberAdaptor has to know enough about the bounce info it gets (e.g., maximum length) so it can store and return it without modification, and getBounceInfo() has to know to return None when there is no previous bounce info for the member, but that should be it. The lengths to which MysqlMemberships.py goes to extract attributes from the bounce info, save them separately, and construct a _BounceInfo instance to return the data only get it in trouble when aspects of the _BounceInfo class change from version to version.
I can see that would be desirable, but the price is difficulty of maintenance because then the get and set BounceInfo methods have to know things about the _BounceInfo class that may change, however see below for a compromise.
No problem.
See below.
Looking at this more deeply, I think it is not as simple as I first thought to simply save the string representation and return it. I think pickle.dump() to a StringIO file and then saving its contents in the set... method and then retrieving the string and returning pickle.loads() in the get... method is the way to go. It has the advantage that the string written to the database is a bit shorter too.
If you also want to be able to see and use some of the bounce info, you could save certain attributes of the _BounceInfo instance in additional columns of the database table.
I think discussing with the author is a good idea.
-- Mark Sapiro <msapiro@value.net> The highway is for gamblers, San Francisco Bay Area, California better use your sense - B. Dylan

We are currently two on irc on the #mailman channel trying to install this patch on our respective systems. Could anyone drop by and/or tell us where to find the latest series of updated files?
thanks in advance
-- Fil

There is an issue with the "flat" mode, when you create the db according to the README file and choose PRIMARY KEY (listname, address)
MySQL brings up the following error : ERROR 1071: Specified key was too long. Max key length is 500
So in fact the initial definitions of address varchar(255) NOT NULL, listname varchar(255) NOT NULL,
do not work, you need to get under the 500 limit (I chose to kepp 255 for the address, and 100 for the listname)
-- Fil

Okay I found out how to install this stuff for just one list (test list): use extend.py mechanism with:
lists/test/extend.py containing the following lines:
# override the default for this list def extend(mlist): mlist._memberadaptor = MysqlMemberships(mlist) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
hope this helps
-- Fil
participants (5)
-
Adrian Wells
-
Fil
-
John Dennis
-
John W. Baxter
-
Mark Sapiro