Merge 'trunk' into 'on_connect_call'
Rafael Kitover [Fri, 19 Jun 2009 21:09:48 +0000 (21:09 +0000)]
r5559@hlagh (orig r6713):  caelum | 2009-06-18 16:03:01 -0700
fix broken link in manual
r5571@hlagh (orig r6725):  ribasushi | 2009-06-19 08:25:19 -0700
 r6706@Thesaurus (orig r6705):  ribasushi | 2009-06-18 12:30:08 +0200
 Branch to attempt prefetch with limit fix
 r6709@Thesaurus (orig r6708):  ribasushi | 2009-06-18 15:54:38 +0200
 This seems to be the prefetch+limit fix - ugly as hell but appears to work
 r6710@Thesaurus (orig r6709):  ribasushi | 2009-06-18 16:13:31 +0200
 More comments
 r6717@Thesaurus (orig r6716):  ribasushi | 2009-06-19 15:39:43 +0200
 single() throws with has_many prefetch
 r6718@Thesaurus (orig r6717):  ribasushi | 2009-06-19 15:40:38 +0200
 Rename test
 r6719@Thesaurus (orig r6718):  ribasushi | 2009-06-19 15:44:26 +0200
 cleanup svn attrs
 r6720@Thesaurus (orig r6719):  ash | 2009-06-19 16:31:11 +0200
 Add extra test for prefetch+has_many

 r6721@Thesaurus (orig r6720):  ribasushi | 2009-06-19 16:33:49 +0200
 no need to slice as use_prefetch already has a limit
 r6722@Thesaurus (orig r6721):  ribasushi | 2009-06-19 16:36:08 +0200
 throw in an extra limit, sophisticate test a bit
 r6723@Thesaurus (orig r6722):  ribasushi | 2009-06-19 16:36:54 +0200
 Fix the fix
 r6725@Thesaurus (orig r6724):  ribasushi | 2009-06-19 17:24:23 +0200
 Fix dubious optimization

1  2 
lib/DBIx/Class/Storage/DBI.pm

@@@ -14,15 -14,13 +14,15 @@@ use List::Util()
  
  __PACKAGE__->mk_group_accessors('simple' =>
      qw/_connect_info _dbi_connect_info _dbh _sql_maker _sql_maker_opts
 -       _conn_pid _conn_tid transaction_depth _dbh_autocommit savepoints/
 +       _conn_pid _conn_tid transaction_depth _dbh_autocommit _on_connect_do
 +       _on_disconnect_do _on_connect_do_store _on_disconnect_do_store
 +       savepoints/
  );
  
  # the values for these accessors are picked out (and deleted) from
  # the attribute hashref passed to connect_info
  my @storage_options = qw/
 -  on_connect_do on_disconnect_do disable_sth_caching unsafe auto_savepoint
 +  on_connect_call on_disconnect_call disable_sth_caching unsafe auto_savepoint
  /;
  __PACKAGE__->mk_group_accessors('simple' => @storage_options);
  
@@@ -179,91 -177,6 +179,91 @@@ immediately before disconnecting from t
  Note, this only runs if you explicitly call L</disconnect> on the
  storage object.
  
 +=item on_connect_call
 +
 +A more generalized form of L</on_connect_do> that calls the specified
 +C<connect_call_METHOD> methods in your storage driver.
 +
 +  on_connect_do => 'select 1'
 +
 +is equivalent to:
 +
 +  on_connect_call => [ [ do_sql => 'select 1' ] ]
 +
 +Its values may contain:
 +
 +=over
 +
 +=item a scalar
 +
 +Will call the C<connect_call_METHOD> method.
 +
 +=item a code reference
 +
 +Will execute C<< $code->($storage) >>
 +
 +=item an array reference
 +
 +Each value can be a method name or code reference.
 +
 +=item an array of arrays
 +
 +For each array, the first item is taken to be the C<connect_call_> method name
 +or code reference, and the rest are parameters to it.
 +
 +=back
 +
 +Some predefined storage methods you may use:
 +
 +=over
 +
 +=item do_sql
 +
 +Executes a SQL string or a code reference that returns a SQL string. This is
 +what L</on_connect_do> and L</on_disconnect_do> use.
 +
 +It can take:
 +
 +=over
 +
 +=item a scalar
 +
 +Will execute the scalar as SQL.
 +
 +=item an arrayref
 +
 +Taken to be arguments to L<DBI/do>, the SQL string optionally followed by the
 +attributes hashref and bind values.
 +
 +=item a code reference
 +
 +Will execute C<< $code->($storage) >> and execute the return array refs as
 +above.
 +
 +=back
 +
 +=item datetime_setup
 +
 +Execute any statements necessary to initialize the database session to return
 +and accept datetime/timestamp values used with
 +L<DBIx::Class::InflateColumn::DateTime>.
 +
 +Only necessary for some databases, see your specific storage driver for
 +implementation details.
 +
 +=back
 +
 +=item on_disconnect_call
 +
 +Takes arguments in the same form as L</on_connect_call> and executes them
 +immediately before disconnecting from the database.
 +
 +Calls the C<disconnect_call_METHOD> methods as opposed to the
 +C<connect_call_METHOD> methods called by L</on_connect_call>.
 +
 +Note, this only runs if you explicitly call L</disconnect> on the
 +storage object.
 +
  =item disable_sth_caching
  
  If set to a true value, this option will disable the caching of
@@@ -434,11 -347,6 +434,11 @@@ sub connect_info 
          $self->_sql_maker_opts->{$sql_maker_opt} = $opt_val;
        }
      }
 +    for my $connect_do_opt (qw/on_connect_do on_disconnect_do/) {
 +      if(my $opt_val = delete $attrs{$connect_do_opt}) {
 +        $self->$connect_do_opt($opt_val);
 +      }
 +    }
    }
  
    %attrs = () if (ref $args[0] eq 'CODE');  # _connect() never looks past $args[0] in this case
  
  This method is deprecated in favour of setting via L</connect_info>.
  
 +=cut
 +
 +sub on_connect_do {
 +  my $self = shift;
 +  $self->_setup_connect_do(on_connect_do => @_);
 +}
 +
 +=head2 on_disconnect_do
 +
 +This method is deprecated in favour of setting via L</connect_info>.
 +
 +=cut
 +
 +sub on_disconnect_do {
 +  my $self = shift;
 +  $self->_setup_connect_do(on_disconnect_do => @_);
 +}
 +
 +sub _setup_connect_do {
 +  my ($self, $opt) = (shift, shift);
 +
 +  my $accessor = "_$opt";
 +  my $store    = "_${opt}_store";
 +
 +  return $self->$accessor if not @_;
 +
 +  my $val = shift;
 +
 +  if (not defined $val) {
 +    $self->$accessor(undef);
 +    $self->$store(undef);
 +    return;
 +  }
 +
 +  my @store;
 +
 +  if (not ref($val)) {
 +    push @store, [ 'do_sql', $val ];
 +  } elsif (ref($val) eq 'CODE') {
 +    push @store, $val;
 +  } elsif (ref($val) eq 'ARRAY') {
 +    push @store, map [ 'do_sql', $_ ], @$val;
 +  } else {
 +    $self->throw_exception("Invalid type for $opt ".ref($val));
 +  }
 +
 +  $self->$store(\@store);
 +  $self->$accessor($val);
 +}
  
  =head2 dbh_do
  
@@@ -647,12 -506,8 +647,12 @@@ sub disconnect 
    my ($self) = @_;
  
    if( $self->connected ) {
 -    my $connection_do = $self->on_disconnect_do;
 -    $self->_do_connection_actions($connection_do) if ref($connection_do);
 +    if (my $connection_call = $self->on_disconnect_call) {
 +      $self->_do_connection_actions(disconnect_call_ => $connection_call)
 +    }
 +    if (my $connection_do   = $self->_on_disconnect_do_store) {
 +      $self->_do_connection_actions(disconnect_call_ => $connection_do)
 +    }
  
      $self->_dbh->rollback unless $self->_dbh_autocommit;
      $self->_dbh->disconnect;
@@@ -769,12 -624,8 +769,12 @@@ sub _populate_dbh 
    #  there is no transaction in progress by definition
    $self->{transaction_depth} = $self->_dbh_autocommit ? 0 : 1;
  
 -  my $connection_do = $self->on_connect_do;
 -  $self->_do_connection_actions($connection_do) if $connection_do;
 +  if (my $connection_call = $self->on_connect_call) {
 +    $self->_do_connection_actions(connect_call_ => $connection_call)
 +  }
 +  if (my $connection_do = $self->_on_connect_do_store) {
 +    $self->_do_connection_actions(connect_call_ => $connection_do)
 +  }
  }
  
  sub _determine_driver {
  }
  
  sub _do_connection_actions {
 -  my $self = shift;
 -  my $connection_do = shift;
 -
 -  if (!ref $connection_do) {
 -    $self->_do_query($connection_do);
 -  }
 -  elsif (ref $connection_do eq 'ARRAY') {
 -    $self->_do_query($_) foreach @$connection_do;
 -  }
 -  elsif (ref $connection_do eq 'CODE') {
 -    $connection_do->($self);
 -  }
 -  else {
 -    $self->throw_exception (sprintf ("Don't know how to process conection actions of type '%s'", ref $connection_do) );
 +  my $self          = shift;
 +  my $method_prefix = shift;
 +  my $call          = shift;
 +
 +  if (not ref($call)) {
 +    my $method = $method_prefix . $call;
 +    $self->$method(@_);
 +  } elsif (ref($call) eq 'CODE') {
 +    $self->$call(@_);
 +  } elsif (ref($call) eq 'ARRAY') {
 +    if (ref($call->[0]) ne 'ARRAY') {
 +      $self->_do_connection_actions($method_prefix, $_) for @$call;
 +    } else {
 +      $self->_do_connection_actions($method_prefix, @$_) for @$call;
 +    }
 +  } else {
 +    $self->throw_exception (sprintf ("Don't know how to process conection actions of type '%s'", ref($call)) );
    }
  
    return $self;
  }
  
 +sub connect_call_do_sql {
 +  my $self = shift;
 +  $self->_do_query(@_);
 +}
 +
 +sub disconnect_call_do_sql {
 +  my $self = shift;
 +  $self->_do_query(@_);
 +}
 +
 +# override in db-specific backend when necessary
 +sub connect_call_datetime_setup { 1 }
 +
  sub _do_query {
    my ($self, $action) = @_;
  
@@@ -1391,21 -1226,13 +1391,13 @@@ sub _select_args_to_query 
  }
  
  sub _select_args {
-   my ($self, $ident, $select, $condition, $attrs) = @_;
+   my ($self, $ident, $select, $where, $attrs) = @_;
  
    my $sql_maker = $self->sql_maker;
-   $sql_maker->{for} = delete $attrs->{for};
-   my $order = { map
-     { $attrs->{$_} ? ( $_ => $attrs->{$_} ) : ()  }
-     (qw/order_by group_by having _virtual_order_by/ )
-   };
-   my $bind_attrs = {};
    my $alias2source = $self->_resolve_ident_sources ($ident);
  
+   # calculate bind_attrs before possible $ident mangling
+   my $bind_attrs = {};
    for my $alias (keys %$alias2source) {
      my $bindtypes = $self->source_bind_attributes ($alias2source->{$alias}) || {};
      for my $col (keys %$bindtypes) {
      }
    }
  
-   # This would be the point to deflate anything found in $condition
-   # (and leave $attrs->{bind} intact). Problem is - inflators historically
-   # expect a row object. And all we have is a resultsource (it is trivial
-   # to extract deflator coderefs via $alias2source above).
-   #
-   # I don't see a way forward other than changing the way deflators are
-   # invoked, and that's just bad...
-   my @args = ('select', $attrs->{bind}, $ident, $bind_attrs, $select, $condition, $order);
+   my @limit;
    if ($attrs->{software_limit} ||
        $sql_maker->_default_limit_syntax eq "GenericSubQ") {
          $attrs->{software_limit} = 1;
  
      # MySQL actually recommends this approach.  I cringe.
      $attrs->{rows} = 2**48 if not defined $attrs->{rows} and defined $attrs->{offset};
-     push @args, $attrs->{rows}, $attrs->{offset};
+     if ($attrs->{rows} && keys %{$attrs->{collapse}}) {
+       ($ident, $select, $where, $attrs)
+         = $self->_adjust_select_args_for_limited_prefetch ($ident, $select, $where, $attrs);
+     }
+     else {
+       push @limit, $attrs->{rows}, $attrs->{offset};
+     }
    }
-   return @args;
+ ###
+   # This would be the point to deflate anything found in $where
+   # (and leave $attrs->{bind} intact). Problem is - inflators historically
+   # expect a row object. And all we have is a resultsource (it is trivial
+   # to extract deflator coderefs via $alias2source above).
+   #
+   # I don't see a way forward other than changing the way deflators are
+   # invoked, and that's just bad...
+ ###
+   my $order = { map
+     { $attrs->{$_} ? ( $_ => $attrs->{$_} ) : ()  }
+     (qw/order_by group_by having _virtual_order_by/ )
+   };
+   $sql_maker->{for} = delete $attrs->{for};
+   return ('select', $attrs->{bind}, $ident, $bind_attrs, $select, $where, $order, @limit);
+ }
+ sub _adjust_select_args_for_limited_prefetch {
+   my ($self, $from, $select, $where, $attrs) = @_;
+   if ($attrs->{group_by} and @{$attrs->{group_by}}) {
+     $self->throw_exception ('Prefetch with limit (rows/offset) is not supported on resultsets with a group_by attribute');
+   }
+   $self->throw_exception ('Prefetch with limit (rows/offset) is not supported on resultsets with a custom from attribute')
+     if (ref $from ne 'ARRAY');
+   # separate attributes
+   my $sub_attrs = { %$attrs };
+   delete $attrs->{$_} for qw/where bind rows offset/;
+   delete $sub_attrs->{$_} for qw/for collapse select order_by/;
+   my $alias = $attrs->{alias};
+   # create subquery select list
+   my $sub_select = [ grep { $_ =~ /^$alias\./ } @{$attrs->{select}} ];
+   # bring over all non-collapse-induced order_by into the inner query (if any)
+   # the outer one will have to keep them all
+   if (my $ord_cnt = @{$attrs->{order_by}} - @{$attrs->{_collapse_order_by}} ) {
+     $sub_attrs->{order_by} = [
+       @{$attrs->{order_by}}[ 0 .. ($#{$attrs->{order_by}} - $ord_cnt - 1) ]
+     ];
+   }
+   # mangle the head of the {from}
+   my $self_ident = shift @$from;
+   my %join_info = map { $_->[0]{-alias} => $_->[0] } (@$from);
+   my (%inner_joins);
+   # decide which parts of the join will remain on the inside
+   #
+   # this is not a very viable optimisation, but it was written
+   # before I realised this, so might as well remain. We can throw
+   # away _any_ branches of the join tree that are:
+   # 1) not mentioned in the condition/order
+   # 2) left-join leaves (or left-join leaf chains)
+   # Most of the join ocnditions will not satisfy this, but for real
+   # complex queries some might, and we might make some RDBMS happy.
+   #
+   #
+   # since we do not have introspectable SQLA, we fall back to ugly
+   # scanning of raw SQL for WHERE, and for pieces of ORDER BY
+   # in order to determine what goes into %inner_joins
+   # It may not be very efficient, but it's a reasonable stop-gap
+   {
+     # produce stuff unquoted, so it can be scanned
+     my $sql_maker = $self->sql_maker;
+     local $sql_maker->{quote_char};
+     my @order_by = (map
+       { ref $_ ? $_->[0] : $_ }
+       $sql_maker->_order_by_chunks ($sub_attrs->{order_by})
+     );
+     my $where_sql = $sql_maker->where ($where);
+     # sort needed joins
+     for my $alias (keys %join_info) {
+       # any table alias found on a column name in where or order_by
+       # gets included in %inner_joins
+       # Also any parent joins that are needed to reach this particular alias
+       for my $piece ($where_sql, @order_by ) {
+         if ($piece =~ /\b$alias\./) {
+           $inner_joins{$alias} = 1;
+         }
+       }
+     }
+   }
+   # scan for non-leaf/non-left joins and mark as needed
+   # also mark all ancestor joins that are needed to reach this particular alias
+   # (e.g.  join => { cds => 'tracks' } - tracks will bring cds too )
+   #
+   # traverse by the size of the -join_path i.e. reverse depth first
+   for my $alias (sort { @{$join_info{$b}{-join_path}} <=> @{$join_info{$a}{-join_path}} } (keys %join_info) ) {
+     my $j = $join_info{$alias};
+     $inner_joins{$alias} = 1 if (! $j->{-join_type} || ($j->{-join_type} !~ /^left$/i) );
+     if ($inner_joins{$alias}) {
+       $inner_joins{$_} = 1 for (@{$j->{-join_path}});
+     }
+   }
+   # construct the inner $from for the subquery
+   my $inner_from = [ $self_ident ];
+   if (keys %inner_joins) {
+     for my $j (@$from) {
+       push @$inner_from, $j if $inner_joins{$j->[0]{-alias}};
+     }
+     # if a multi-type join was needed in the subquery ("multi" is indicated by
+     # presence in {collapse}) - add a group_by to simulate the collapse in the subq
+     for my $alias (keys %inner_joins) {
+       # the dot comes from some weirdness in collapse
+       # remove after the rewrite
+       if ($attrs->{collapse}{".$alias"}) {
+         $sub_attrs->{group_by} = $sub_select;
+         last;
+       }
+     }
+   }
+   # generate the subquery
+   my $subq = $self->_select_args_to_query (
+     $inner_from,
+     $sub_select,
+     $where,
+     $sub_attrs
+   );
+   # put it back in $from
+   unshift @$from, { $alias => $subq };
+   # This is totally horrific - the $where ends up in both the inner and outer query
+   # Unfortunately not much can be done until SQLA2 introspection arrives
+   #
+   # OTOH it can be seen as a plus: <ash> (notes that this query would make a DBA cry ;)
+   return ($from, $select, $where, $attrs);
  }
  
  sub _resolve_ident_sources {