Freeze privatemsg db schema + msg delete functionality + changes to query builder

If it can be done, I would also favour that - I am finding keeping track of UNION's a pain and with current schema we will probably need more going forward...

Most of the UNION's come from having to process the index table twice - once for the author and then once again for the recipients.

Combining them into one UID column should fix that, so when sending a message to N users, there will be N+1 rows created in the index table - the extra row having the UID = author UID and new = 0.

This would also simplify delete architecture as all you need is a single column - "deleted" instead of two, one for author, the other for recipient.

Another fringe benefit - the inbox automatically turns into an "all messages" folder.

Patch attached which does the above and a little bit more.

For good measure, I have also included a reworked patch from #291206: Delete Architecture. and removed the from/to columns - they will need to be reworked to fit in with conversations anyway and would complicate this patch too much if they were a part of it. IMO a separate issue.

I have NOT renamed the inbox to "All Messages" as I dislike that title and it could cause problems further down the line when we have tagging support. I will leave the renaming and any potential minefields to you. :P

(we can also get rid of "sent messages pretty simply now - just remove the menu item and make a few simple simple deletions in the privatemsg_list function.)

I have included delete architecture.

As for tagging, I would assume another JOIN in the query and two additional tables: pm_tags (tag_name, tag_id) and pm_tags_index (tag_id, thread_id, uid)

and then some syntax in the list_alter functions along the lines of

if ($tag) {
$fragments['inner_join'][]  = 'INNER JOIN {pm_tags} pmtags ON pmtags.name = %d';
$fragments['inner_join'][]  = 'INNER JOIN {pm_tags_index} pmtags_index ON pmtags.tag_id = pmtags_index.tag_id';
$fragments['where'][]       = 'pmi.thread_id = pmtags_index.thread_id';

}

No idea about forwarding and the rest... forwarding would have to involve a new thread_id (and be per message rather than per conversation) but probably not much else.

EDIT - forgot to mention the main reason for this approach - it feels right. :P

@ comment 8 - marking for deletion works with the proposed patch. Actual deletion would cause issues, but even with the three table situation, the actual message is not deleted from pm_messages. I don't think we can delete the stuff in pm_messages until all participants have marked the message as deleted. (the problem when I first proposed moving (back) to this schema was the intent to remove the row containing the data for the delete action.)

As for the proposed schema in comment 10, the number of JOIN's I do not think that is an issue - when we have tagging, we will probably have to have more JOIN's anyway.

I don't think the proposal in 10 adds enough yet (seeing the actual queries may change my mind, as one of the hurdles is overcoming unforseen issues :P). An improvement in it that I like is moving the timestamp to the pm_messages table, but apart from that, the queries will probably not be much simpler - just involving different tables for different actions.

I might give this a go too - but a couple of concerns first:

1. thread_id in pm_messages. Currently we calculate the thread_id AFTER inserting into that table (if it already exists, we keep that value, but if not, thread_id = mid.). Doing this with the proposed schema means we will write to pm_messages, and then update that same record. No idea how bad this is or even whether it is worse than what we do now, but the concern should be noted. On the other hand, getting participants will be much simpler.

I think getting rid of thread_id from pm_messages will cause the read queries needing thread_id to become more complicated.

2. pm_thread assumes that a conversation does not allow the participants to change. Good for new users, but does the same apply when updating from drupal 5 where the recipients can be arbitrary? (to be fair, my approach also ignores this). This is probably ok.

Question: is it acceptable to use string matching instead of integer matching? (ie how big is the hit?)

Reason: I can extend my above patch and make the UID column in pm_index be smalltext or soemthing similar - that way we can prefix user id's with u_ and later expand and add g_{group_id} for OG support, r_{role_id} for roles etc etc.

Would this be scalable for larger sites/installations?

just making sure the stuff is expandable in the future - from what I see, both approaches can be explianed in the mentioned way - but we probably will not want to as there would be other downsides (person leaving a group/role will lose messages etc etc...)

updated version of my patch in comment #5 to move timestamp from index to messages table.

(Now time to modify this to get as close as possible to mrtoner's proposed schema so that there is a choice of paths.)

same as above, but with upgrade function to move data from drupal 5 version of privatemsg (there is still a need to test for duplicate messages and also to remove the other tables such as privatemsg_block_user and privatemsg_folder).

My thinking - it is easier to upgrade from privatemsg for drupal 5 to my version of this patch for drupal 6 as there is no dataloss.

If we decide to go mrtoner's way, moving from here to his schema should be trivial and we can also provide an upgrade path (create pm_threads, fill in participants and then drop all rows in pm_index where deleted = 1 and then drop the deleted column).

A curious thing - I migrated a privatemsg install with the privatemsg for being approx 2.4 MB, and privatemsg_archive being 14KB. The new combined pm_message and pm_index is 5.2 MB (3.9 for the former, 1.3 for the latter.)

(I would also like to be the place from where we give support changes to the schema)

untested update function from above patch to mrtoners layout:

function privatemsg_update_6001() {
  $schema = array();
  $schema['pm_thread'] = array(
    'description'       => t('{pm_thread} holds the participant information'),
    'fields'        => array(
      'thread_id'    => array(
        'description'   => t('Private Message ID'),
        'type'          => 'serial',
        'not null'      => TRUE,
        'unsigned'      => TRUE,
      ),
      'participants'     => array(
        'description'   => t('list of participants by id'),
        'type'          => 'varchar',
        'length'        => 255,
        'not null'      => TRUE,
      ),
    ),
  );
  $ret = array;
  db_create_table($ret, 'pm_thread', $schema['pm_thread']);
  $threads = db_query('SELECT DISTINCT thread_id from {pm_index}');
  while ($result = db_fetch_array($data)) {
    $query = db_query('SELECT DISTINCT uid from {pm_index} WHERE thread_id = %d', $result['thread_id']);
    $participants = array();
    while ($participant = db_fetch_array($query)) {
    $participants = $participant['uid'];
    }
    $participants = explode(',', $participants);
    $ret = db_query("INSERT INTO {pm_thread} (thread_id, participants) VALUES (%d, '%s')", $result['thread_id'], $participants);
  }
  $ret = db_query('DELETE FROM {pm_index} WHERE deleted = %d', 1);
  db_drop_field($ret, 'pm_index', 'deleted');

}

The following would also be added to the schema

  $schema['pm_thread'] = array(
    'description'       => t('{pm_thread} holds the participant information'),
    'fields'        => array(
      'thread_id'    => array(
        'description'   => t('Private Message ID'),
        'type'          => 'serial',
        'not null'      => TRUE,
        'unsigned'      => TRUE,
      ),
      'participants'     => array(
        'description'   => t('list of participants by id'),
        'type'          => 'varchar',
        'length'        => 255,
        'not null'      => TRUE,
      ),
    ),
  );

and apart from this, the only functions that need to be altered are privatemsg_privatemsg_participants_alter, privatemsg_send, privatemsg_form_alter and privatemsg_preprocess_privatemsg_to.

I will be honest and think this is mostly unneeded as the thing it simplifies is getting all the participants in a thread, but we already have a working function for that.

(still as CNW as I have not created a patch for this, nor have I finished the code writing, but I need sleep.)

"data normalisation rules"? hehe, I have no idea what that is. The reason I moved thread_id back to pm_index was that it just felt wrong doing an INSERT INTO followed straight up by an UPDATE into the same row.

as for the size of the field - I cheated and copied the schema from the subject, hence the current limit. No real reason. as for storing them all in one field vs one user per row, I misunderstood what you meant. However, going the latter way, it is no different from the current pm_index and is just duplicating the data.

What do you say? which way should I take the patch?

As for speed, I doubt it makes a difference (but i am totally clueless when it comes to databases - I just try things and either they go boom or they don't.) - we will still need to load the subject from the pm_message table.

PS can we keep pm_index called that instead of pm_instances? If we do keep that, the changes for your scheme are pretty minor over my patch.

(PPS - patch in comment 25 is almost CNR - just need to change the update function to use update_sql instead of db_query...)

Thanks for the explanations. There is a problem with putting thread_id in pm_messages - for new threads it is derrived AFTER writing the message to pm_messages. TO get around this you need a hack - add dummy therad data i first time around, calculate the thread_id and then update the field.

I don't think there are (m)any cases where pm_messages can be used without a JOIN with pm_index/instances, so IMO thread_id should only be in the latter table.

The problem is not a read problem, but a write one - there is also a similar issue with writing to pm_thread where you would only write to it if the post starts a new thread, so there would need to be too loops - one if a new thread, and then another for writing to pm_instances.

untested version of mrtoner's proposal - with the difference that the thread_id is staying in pm_instances instead of pm_message.

Now that both proposals are here, time to review and select one.

and my competing patch - without the update function this time.

Just a guide, the differences between the two patches:

1. pm_message is currently identical in both. mrtoner DID want thread_id in there, but I resisted in my version of his architecture. :P

Also a question. me and mrtoner have both moved the timestamp from pm_index/pm_instances into pm_message. Was there a reason for the original design? such as speed issues? If so, that bit can be trivially reverted in both patches as pm_message and pm_[index|instances] are rarely used without a JOIN between the two.

2. In my version, delete is indicated by a "deleted" column = 1 in pm_index. On mrtoner's, it is indicated by lack of the data in pm_instances (making that table lossy, but the can probably be recreated by using the data in pm_thread).

3. With mrtoners way, deleting the row from pm_instances means that if a participant deletes a message, the instances table no longer knows that the user was a participant. To get around this, there is another table pm_thread which stores all participants in a thread.

Query wise, the list alter functions lack a where deleted = 0 in mrtoner's version (making his query a little simpler) and also the participants alter is different at it reads from pm_thread instead of pm_index as in mine.

4. The only major difference is in pm_send where there is extra logic in mrtoner's version to insert the relevant records into pm_thread on thread creation.

5. For upgrading from Drupal 5, I don't see how we can easily get to mrtoner's version of the schema without going through an intermediary step similar to my proposed version.

On the whole, I still prefer mine (call it ego), but both are IMO an improvement over what we currently have and extending either to eg add tag support should be pretty similar.

so... any reviews?

I have pretty extensively tested the patch in comment 30 and it works well.

There is an acid to see how well the architecture(s) works: #314327: Tagging architecture. however, I cannot figure out how to make that work - the framework is there, but the query does not give any results.

No probs - you're not the only one who has suddenly run out of free time :(

The "acid test" #314327: Tagging architecture. patch is now also ready (for the single tag case) and may be best to test at the same time as this patch.

(I would also like #315325: Userblocking architecture + other fixes patch to go in soon as these three should complete the schema rearchitecture.)

Added a delete button - not very elegant.

and a slightly less ugly patch - this approach can be extended and made less ugly when needed.

and updated again - this time to add a function to add a function for per message actions which will allow a row of buttons.

Also, made changes to only show the reply box if there are messages that have been displayed and give an error message if there are no (undeleted/readable) messages in the thread.

Wahey!

As for the remaining issues:

For number 1, I would prefer to go the route of #303087: Move "Sent Messages" to privatemsg_filter, Add inbox, improve user filtering. - atleast for the beta and then stay that way unless there is some fuss over the lack of sent items.

If you want to actually fix the buy, you can change the following (around line 679):

  $fragments['where'][]       = 'pm.author = %d';
  $fragments['where'][]       = 'pmi.deleted = 0';

to

  $fragments['where'][]       = 'pmi.uid = %d';
  $fragments['query_args'][]  = $account->uid;
  $fragments['where'][]       = 'pm.author = %d';
  $fragments['where'][]       = 'pmi.deleted = 0';

As for number 2, I cannot replicate that behaviour.