From: Peter Rabbitson Date: Wed, 21 Nov 2012 11:38:43 +0000 (+0100) Subject: Fix self-referential resultset update/delete on MySQL (aggravated by 31073ac7) X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=45150bc4f3b983d64477728e2bd8e722c10c7a31;p=dbsrgits%2FDBIx-Class-Historic.git Fix self-referential resultset update/delete on MySQL (aggravated by 31073ac7) 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. --- diff --git a/Changes b/Changes index 353bbdf..0e5d8f8 100644 --- 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 index fd0c7f0..5212dcd 100644 --- 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 index c96b11c..34ee054 100644 --- 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 index dc7ff90..ae55f1f 100644 --- 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/ (?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 index 488697f..4e89d9f 100644 --- 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 new file mode 100644 index 0000000..9de4c7f --- /dev/null +++ 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;