Fix self-referential resultset update/delete on MySQL (aggravated by 3…

…1073ac) MySQL is unable to figure out it needs a temp-table when it is trying to update a table with a condition it derived from that table. So what we do here is give it a helpful nudge by rewriting any "offending" subquery to a double subquery post-sql-generation. Performance seems to be about the same for moderately large sets. If it becomes a problem later we can always revisit and add the ability to induce "row-by-row" update/deletion instead. The implementation sucks, but is rather concise and most importantly contained to the MySQL codepath only - it does not affect the rest of the code flow in any way.
Perl5 · Jan 21, 2013 · 45150bc · 45150bc
1 parent 31073ac
commit 45150bc
Show file tree

Hide file tree

Showing 6 changed files with 264 additions and 2 deletions.
diff --git a/Changes b/Changes
@@ -11,6 +11,10 @@ Revision history for DBIx::Class
           qualifiers - leave the source name alone (RT#80015, RT#78844)
         - Fix incorrect behavior on resultset update/delete invoked on
           composite resultsets (e.g. as_subselect_rs)
+        - Fix update/delete operations referencing the updated table failing
+          on MySQL, due to its refusal to modify a table being directly
+          queried. As a workaround induce in-memory temp-table creation
+          (RT#81378, RT#81897)
         - More robust behavior under heavily threaded environments - make
           sure we do not have refaddr reuse in the global storage registry
         - Fix failing test on 5.8 under Win32 (RT#81114)

diff --git a/Makefile.PL b/Makefile.PL
@@ -75,6 +75,11 @@ my $runtime_requires = {
   'Scope::Guard'             => '0.03',
   'SQL::Abstract'            => '1.73',
   'Try::Tiny'                => '0.07',
+
+  # Technically this is not a core dependency - it is only required
+  # by the MySQL codepath. However this particular version is bundled
+  # since 5.10.0 and is a pure-perl module anyway - let it slide
+  'Text::Balanced'           => '2.00',
 };
 
 my $build_requires = {

diff --git a/lib/DBIx/Class/SQLMaker/MySQL.pm b/lib/DBIx/Class/SQLMaker/MySQL.pm
@@ -1,6 +1,9 @@
 package # Hide from PAUSE
   DBIx::Class::SQLMaker::MySQL;
 
+use warnings;
+use strict;
+
 use base qw( DBIx::Class::SQLMaker );
 
 #
@@ -29,6 +32,71 @@ sub _generate_join_clause {
     return $self->next::method($join_type);
 }
 
+my $force_double_subq;
+$force_double_subq = sub {
+  my ($self, $sql) = @_;
+
+  require Text::Balanced;
+  my $new_sql;
+  while (1) {
+
+    my ($prefix, $parenthesized);
+
+    ($parenthesized, $sql, $prefix) = do {
+      # idiotic design - writes to $@ but *DOES NOT* throw exceptions
+      local $@;
+      Text::Balanced::extract_bracketed( $sql, '()', qr/[^\(]*/ );
+    };
+
+    # this is how an error is indicated, in addition to crapping in $@
+    last unless $parenthesized;
+
+    if ($parenthesized =~ $self->{_modification_target_referenced_re}) {
+      # is this a select subquery?
+      if ( $parenthesized =~ /^ \( \s* SELECT \s+ /xi ) {
+        $parenthesized = "( SELECT * FROM $parenthesized `_forced_double_subquery` )";
+      }
+      # then drill down until we find it (if at all)
+      else {
+        $parenthesized =~ s/^ \( (.+) \) $/$1/x;
+        $parenthesized = join ' ', '(', $self->$force_double_subq( $parenthesized ), ')';
+      }
+    }
+
+    $new_sql .= $prefix . $parenthesized;
+  }
+
+  return $new_sql . $sql;
+};
+
+sub update {
+  my $self = shift;
+
+  # short-circuit unless understood identifier
+  return $self->next::method(@_) unless $self->{_modification_target_referenced_re};
+
+  my ($sql, @bind) = $self->next::method(@_);
+
+  $sql = $self->$force_double_subq($sql)
+    if $sql =~ $self->{_modification_target_referenced_re};
+
+  return ($sql, @bind);
+}
+
+sub delete {
+  my $self = shift;
+
+  # short-circuit unless understood identifier
+  return $self->next::method(@_) unless $self->{_modification_target_referenced_re};
+
+  my ($sql, @bind) = $self->next::method(@_);
+
+  $sql = $self->$force_double_subq($sql)
+    if $sql =~ $self->{_modification_target_referenced_re};
+
+  return ($sql, @bind);
+}
+
 # LOCK IN SHARE MODE
 my $for_syntax = {
    update => 'FOR UPDATE',

diff --git a/lib/DBIx/Class/Storage/DBI/mysql.pm b/lib/DBIx/Class/Storage/DBI/mysql.pm
@@ -5,6 +5,9 @@ use warnings;
 
 use base qw/DBIx::Class::Storage::DBI/;
 
+use List::Util 'first';
+use namespace::clean;
+
 __PACKAGE__->sql_maker_class('DBIx::Class::SQLMaker::MySQL');
 __PACKAGE__->sql_limit_dialect ('LimitXY');
 __PACKAGE__->sql_quote_char ('`');
@@ -32,6 +35,56 @@ sub _dbh_last_insert_id {
   $dbh->{mysql_insertid};
 }
 
+sub _prep_for_execute {
+  my $self = shift;
+  #(my $op, $ident, $args) = @_;
+
+  # Only update and delete need special double-subquery treatment
+  # Insert referencing the same table (i.e. SELECT MAX(id) + 1) seems
+  # to work just fine on MySQL
+  return $self->next::method(@_) if ( $_[0] eq 'select' or $_[0] eq 'insert' );
+
+
+  # FIXME FIXME FIXME - this is a terrible, gross, incomplete hack
+  # it should be trivial for mst to port this to DQ (and a good
+  # exercise as well, since we do not yet have such wide tree walking
+  # in place). For the time being this will work in limited cases,
+  # mainly complex update/delete, which is really all we want it for
+  # currently (allows us to fix some bugs without breaking MySQL in
+  # the process, and is also crucial for Shadow to be usable)
+
+  # extract the source name, construct modification indicator re
+  my $sm = $self->sql_maker;
+
+  my $target_name = $_[1]->from;
+
+  if (ref $target_name) {
+    if (
+      ref $target_name eq 'SCALAR'
+        and
+      $$target_name =~ /^ (?:
+          \` ( [^`]+ ) \` #`
+        | ( [\w\-]+ )
+      ) $/x
+    ) {
+      # this is just a plain-ish name, which has been literal-ed for
+      # whatever reason
+      $target_name = first { defined $_ } ($1, $2);
+    }
+    else {
+      # this is something very complex, perhaps a custom result source or whatnot
+      # can't deal with it
+      undef $target_name;
+    }
+  }
+
+  local $sm->{_modification_target_referenced_re} =
+      qr/ (?<!DELETE) [\s\)] FROM \s (?: \` \Q$target_name\E \` | \Q$target_name\E ) [\s\(] /xi
+    if $target_name;
+
+  $self->next::method(@_);
+}
+
 # here may seem like an odd place to override, but this is the first
 # method called after we are connected *and* the driver is determined
 # ($self is reblessed). See code flow in ::Storage::DBI::_populate_dbh

diff --git a/t/71mysql.t b/t/71mysql.t
@@ -17,8 +17,6 @@ plan skip_all => 'Test needs ' . DBIx::Class::Optional::Dependencies->req_missin
 
 my ($dsn, $user, $pass) = @ENV{map { "DBICTEST_MYSQL_${_}" } qw/DSN USER PASS/};
 
-#warn "$dsn $user $pass";
-
 plan skip_all => 'Set $ENV{DBICTEST_MYSQL_DSN}, _USER and _PASS to run this test'
   unless ($dsn && $user);
 
@@ -296,6 +294,47 @@ NULLINSEARCH: {
   }, 'count on grouped columns with the same name does not throw');
 }
 
+# a more contrived^Wcomplicated self-referential double-subquery test
+{
+  my $rs = $schema->resultset('Artist')->search({ name => { -like => 'baby_%' } });
+
+  $rs->populate([map { [$_] } ('name', map { "baby_$_" } (1..10) ) ]);
+
+  my ($count_sql, @count_bind) = @${$rs->count_rs->as_query};
+
+  my $complex_rs = $schema->resultset('Artist')->search(
+    { artistid => {
+      -in => $rs->get_column('artistid')
+                  ->as_query
+    } },
+  );
+
+  $complex_rs->update({ name => \[ "CONCAT( `name`, '_bell_out_of_', $count_sql )", @count_bind ] });
+
+  for (1..10) {
+    is (
+      $schema->resultset('Artist')->search({ name => "baby_${_}_bell_out_of_10" })->count,
+      1,
+      "Correctly updated babybell $_",
+    );
+  }
+
+  my $ac = $schema->resultset('Artist')->count_rs;
+  my $old_count = $ac->next;
+  $ac->reset;
+
+  my $orig_debug = $schema->storage->debug;
+  $schema->storage->debug(1);
+  my $query_count = 0;
+  $schema->storage->debugcb(sub { $query_count++ });
+  $complex_rs->delete;
+  $schema->storage->debugcb(undef);
+  $schema->storage->debug($orig_debug);
+
+  is ($query_count, 1, 'One delete query fired');
+  is ($old_count - $ac->next, 10, '10 Artists correctly deleted');
+}
+
 ZEROINSEARCH: {
   my $cds_per_year = {
     2001 => 2,

diff --git a/t/sqlmaker/mysql.t b/t/sqlmaker/mysql.t
@@ -0,0 +1,93 @@
+use strict;
+use warnings;
+
+use Test::More;
+
+use lib qw(t/lib);
+use DBICTest;
+use DBICTest::Schema;
+use DBIC::SqlMakerTest;
+use DBIC::DebugObj;
+
+my $schema = DBICTest::Schema->connect (DBICTest->_database, { quote_char => '`' });
+# cheat
+require DBIx::Class::Storage::DBI::mysql;
+bless ( $schema->storage, 'DBIx::Class::Storage::DBI::mysql' );
+
+# check that double-subqueries are properly wrapped
+{
+  my ($sql, @bind);
+  my $debugobj = DBIC::DebugObj->new (\$sql, \@bind);
+  my $orig_debugobj = $schema->storage->debugobj;
+  my $orig_debug = $schema->storage->debug;
+
+  $schema->storage->debugobj ($debugobj);
+  $schema->storage->debug (1);
+
+  # the expected SQL may seem wastefully nonsensical - this is due to
+  # CD's tablename being \'cd', which triggers the "this can be anything"
+  # mode, and forces a subquery. This in turn forces *another* subquery
+  # because mysql is being mysql
+  # Also we know it will fail - never deployed. All we care about is the
+  # SQL to compare
+  eval { $schema->resultset ('CD')->update({ genreid => undef }) };
+  is_same_sql_bind (
+    $sql,
+    \@bind,
+    'UPDATE cd SET `genreid` = ? WHERE `cdid` IN ( SELECT * FROM ( SELECT `me`.`cdid` FROM cd `me` ) `_forced_double_subquery` )',
+    [ 'NULL' ],
+    'Correct update-SQL with double-wrapped subquery',
+  );
+
+  # same comment as above
+  eval { $schema->resultset ('CD')->delete };
+  is_same_sql_bind (
+    $sql,
+    \@bind,
+    'DELETE FROM cd WHERE `cdid` IN ( SELECT * FROM ( SELECT `me`.`cdid` FROM cd `me` ) `_forced_double_subquery` )',
+    [],
+    'Correct delete-SQL with double-wrapped subquery',
+  );
+
+  # and a really contrived example (we test it live in t/71mysql.t)
+  my $rs = $schema->resultset('Artist')->search({ name => { -like => 'baby_%' } });
+  my ($count_sql, @count_bind) = @${$rs->count_rs->as_query};
+  eval {
+    $schema->resultset('Artist')->search(
+      { artistid => {
+        -in => $rs->get_column('artistid')
+                    ->as_query
+      } },
+    )->update({ name => \[ "CONCAT( `name`, '_bell_out_of_', $count_sql )", @count_bind ] });
+  };
+
+  is_same_sql_bind (
+    $sql,
+    \@bind,
+    q(
+      UPDATE `artist`
+        SET `name` = CONCAT(`name`, '_bell_out_of_', (
+          SELECT *
+            FROM (
+              SELECT COUNT( * )
+                FROM `artist` `me`
+                WHERE `name` LIKE ?
+            ) `_forced_double_subquery`
+        ))
+      WHERE
+        `artistid` IN (
+          SELECT *
+            FROM (
+              SELECT `me`.`artistid`
+                FROM `artist` `me`
+              WHERE `name` LIKE ?
+            ) `_forced_double_subquery` )
+    ),
+    [ ("'baby_%'") x 2 ],
+  );
+
+  $schema->storage->debugobj ($orig_debugobj);
+  $schema->storage->debug ($orig_debug);
+}
+
+done_testing;