DELETE FROM ... INNER JOIN is by convention not allowed in Drupal [#1079620]

This is not allowed in Drupal:

function og_mailinglist_delete_group_threads($gid, $uid) {
  // Delete threads.
  db_query("DELETE m FROM {og_mailinglist_thread} m
           INNER JOIN {og_ancestry} o
           WHERE o.nid = m.nid
           AND m.uid = %d
           AND o.group_nid = %d", $uid, $gid);  
}

Per http://drupal.org/node/555562, it should be something like this instead:

DELETE m FROM {og_mailinglist_thread} AS m
USING {og_ancestry} AS o
WHERE o.nid = m.nid
AND m.uid = %d
AND o.group_nid = %d

Pretty straightforward; I just don't have time to apply and test this tonight, so I'm leaving myself a note.

Comment	File	Size	Author
#6	og_mailinglist-delete-using.patch	1.13 KB	greg.1.anderson

Comments

Comment #1

kyle_mathews commented 3 March 2011 at 17:25

I tried running the following query on a test database using MySQL but it didn't work. Any clue what's wrong?

DELETE
FROM og_mailinglist_thread AS m 
USING og_ancestry AS o 
WHERE (o.nid = m.nid) 
AND m.uid = 3 
AND o.group_nid = 149

Comment #2

greg.1.anderson commented 3 March 2011 at 20:04

Single quotes around the constants, maybe? I'm not sure, I just got the query from the page I referenced above; it looked plausible. :) I'll look into it and see if I can find a query that works both places.

Comment #3

kyle_mathews commented 10 March 2011 at 17:17

I'm going to working on ogm on Friday at the Drupalcon code sprint. If you have a chance to test this and write a patch before Friday, that'd be much appreciated.

Comment #4

greg.1.anderson commented 10 March 2011 at 18:45

I'll try; I've been swamped by other things, e.g. getting drush site-upgrade ready for drupalcon (which alas I am unable to attend). I think I should have a minute today to experiment.

Comment #5

greg.1.anderson commented 11 March 2011 at 01:21

This works in Postgres:

function og_mailinglist_delete_group_threads($gid, $uid) {
  // Delete threads.
  db_query("DELETE FROM {og_mailinglist_thread} AS m
           USING {og_ancestry} AS o
           WHERE o.nid = m.nid
           AND m.uid = %d
           AND o.group_nid = %d", $uid, $gid);  
}

This works in MYSQL:

function og_mailinglist_delete_group_threads($gid, $uid) {
  // Delete threads.
  db_query("DELETE FROM m, o
           USING {og_mailinglist_thread} AS m, {og_ancestry} AS o
           WHERE (o.nid = m.nid)
           AND m.uid = %d
           AND o.group_nid = %d", $uid, $gid);  
}

aka:

DELETE FROM m, o USING og_mailinglist_thread AS m, og_ancestry AS o WHERE (o.nid = m.nid) AND m.uid = '3' AND o.group_nid = '149';

I have not yet found a way to express it that works in both databases, nor have I found similar usage of USING in Drupal. The referenced issues in the above-quoted page are all unresolved.

Comment #6

greg.1.anderson commented 11 March 2011 at 14:42

Status:

Active

» Needs review

Status	File	Size
new	og_mailinglist-delete-using.patch	1.13 KB

The best I could come up with in the short term would be to special-case postgres in og_mailinglist. Patch enclosed. A longer-term solution would be to define a new function in Drupal core, db_delete_using, and have database.mysql.inc and database.pgsql.inc generate the correct SQL for their domain.

My recommendation would be to use this patch as an interim measure; if you agree that db_delete_using is a good idea, then I will begin the long path of getting a patch accepted to Drupal 7 core and then back-ported to Drupal 6 core.

Comment #7

kyle_mathews commented 11 March 2011 at 17:01

Status:

Needs review

» Fixed

I've added your patch. I needed to modify slightly the MySQL syntax as we only want to delete rows from og_mailinglist_thread not also from og_ancestry.

http://drupalcode.org/project/og_mailinglist.git/blobdiff/ecb519022157fa...

Comment #8

greg.1.anderson commented 11 March 2011 at 17:47

Sorry, you are right; the FROM m, o slipped in there, and it shouldn't have. Looks like the postgres code is correct. Thanks for the commit.

Comment #9

greg.1.anderson commented 12 March 2011 at 00:12

This appears to already be handled by DBTNG: http://api.drupal.org/api/drupal/includes--database--database.inc/functi...

I have not delved in to look up the exact methods of delete to call to generate a delete multi (it would need to be added if not already available), but in short, in order to fix (re-fix) this issue in a db-independent way, the best solution would be to depend on the d6 DBTNG code and rewrite all of your queries and db ops using that mechanism. I'd recommend leaving the db special-case code in until such a time as you decide to do a d7 port of your module.