Large Mailbox Append Fix

[Ken Murchison]

Bron Gondwana wrote:
> On Mon, 24 Sep 2007 23:39:52 -0400, "Ken Murchison" <murch at andrew.cmu.edu> said:
>> Yes, my for() loop test was incorrect, and SHOULD BE:
>>
>> (index_list->count < max_count) &&
>> 		 (msgno <= mailbox->exists)
>>
>>
>> So, for mailbox->exists=1000, and max_count=1000, when we exit the for() 
>> loop for case b), index_list->count=1000 and
>> msgno=1001.  Since msgno > mailbox->exists, we set index_list->last_uid 
>> to 3005, which should result in:
>>
>> UPLOAD 3005 <date> [1000 messages]\r\n
> 
> I'm afraid I still don't see how your code addresses the case where there
> turn out to be no more messages with higher UIDs which need uploading even
> though we've hit exactly max_count messages.  It doesn't pre-scan to the
> end and check if there's going to be another one so it can predict if it
> needs to set last_uid to the mailbox value.

Ahh, my rusty old brain finally figured out what you're talking about. 
I added a check for that case (see below).  In my infinite wisdom, I 
also didn't have a way for the while() loop to exit cleanly.


> Can you please email the sync_client.c with the patch applied so I can see
> the function in its final form and run some thought experiments against it.
> At the moment I feel I might be attacking a strawman, but given how bad the
> performance hit of un-necessary cache rewrites on big folders was, I want
> to make sure we get this logic right!

Here's what I currently have:

static int upload_messages_list(struct mailbox *mailbox,
				struct sync_msg_list *list)
{
     unsigned long msgno;
     int r = 0;
     struct index_record record;
     struct sync_msg *msg;
     struct sync_index_list *index_list;
     int max_count = config_getint(IMAPOPT_SYNC_BATCH_SIZE);

     if (max_count <= 0) max_count = INT_MAX;

     if (chdir(mailbox->path)) {
         syslog(LOG_ERR, "Couldn't chdir to %s: %s",
                mailbox->path, strerror(errno));
         return(IMAP_IOERROR);
     }

     msgno = 1;
     msg = list->head;
     do {
	/* Break UPLOAD into chunks of <=max_count messages */
	index_list = sync_index_list_create();

	for (; (index_list->count < max_count) &&
		 (msgno <= mailbox->exists); msgno++) {
	    r = mailbox_read_index_record(mailbox, msgno, &record);

	    if (r) {
		syslog(LOG_ERR,
		       "IOERROR: reading index entry for msgno %lu of %s: %m",
		       record.uid, mailbox->name);
		return(IMAP_IOERROR);
	    }

	    /* Skip over messages recorded on server which are missing on client
	     * (either will be expunged or have been expunged already) */
	    while (msg && (record.uid > msg->uid))
		msg = msg->next;

	    if (msg && (record.uid == msg->uid) &&
		message_guid_compare_allow_null(&record.guid, &msg->guid)) {
		msg = msg->next;  /* Ignore exact match */
		continue;
	    }

	    /* Message with this GUID exists on client but not server */
	    sync_index_list_add(index_list, msgno, &record);
	}

	if (index_list->count == 0) {
	    /* Reached end of existing messages, with nothing in our list -
	     * final UID might not be same as UIDLAST, force update */
	    r = update_uidlast(mailbox);
	}
	else {
	    /* We have a chunk of messages to UPLOAD */
	    if (msgno > mailbox->exists) {
		/* Last chunk - set final UID to be the same as UIDLAST */
		index_list->last_uid = mailbox->last_uid;
	    } else {
		syslog(LOG_NOTICE,
		       "Hit upload limit %d at UID %lu for %s, sending",
		       max_count, index_list->last_uid, mailbox->name);
	    }

	    r = index_list_work(mailbox, index_list);
	}
	sync_index_list_free(&index_list);

     } while (!r && (msgno <= mailbox->exists));

     return(r);
}


-- 
Kenneth Murchison
Systems Programmer
Project Cyrus Developer/Maintainer
Carnegie Mellon University