From: Peter Rabbitson Date: Thu, 13 Aug 2009 05:40:44 +0000 (+0000) Subject: Caelum was right to make _get_dbh private - reverting (and some code refactoring) X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=9ae966b90f1220796b4fef69a97e7c4fb360d6bc;p=dbsrgits%2FDBIx-Class-Historic.git Caelum was right to make _get_dbh private - reverting (and some code refactoring) --- diff --git a/Changes b/Changes index 5918ec2..72aa3a4 100644 --- a/Changes +++ b/Changes @@ -33,9 +33,6 @@ Revision history for DBIx::Class prefetch - Massive optimization of the DBI storage layer - reduce the amount of connected() calls - - New ::Storage::DBI method last_dbh() - it will still return a - newly connected $dbh if we start unconnected, but will not ping - the server on every invocation unlike dbh() - Some fixes of multi-create corner cases - Multiple POD improvements - Added exception when resultset is called without an argument diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm index 9fdb67c..42867ba 100644 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/lib/DBIx/Class/Storage/DBI.pm @@ -452,7 +452,11 @@ sub connect_info { } sub _default_dbi_connect_attributes { - return { AutoCommit => 1 }; + return { + AutoCommit => 1, + RaiseError => 1, + PrintError => 0, + }; } =head2 on_connect_do @@ -551,6 +555,7 @@ sub dbh_do { } }; + # ->connected might unset $@ - copy my $exception = $@; if(!$exception) { return $want_array ? @result : $result[0] } @@ -598,6 +603,7 @@ sub txn_do { $self->txn_commit; }; + # ->connected might unset $@ - copy my $exception = $@; if(!$exception) { return $want_array ? @result : $result[0] } @@ -687,22 +693,32 @@ answering, etc.) This method is used internally by L. =cut sub connected { - my ($self) = @_; + my $self = shift; + return 0 unless $self->_seems_connected; - if(my $dbh = $self->_dbh) { - if(defined $self->_conn_tid && $self->_conn_tid != threads->tid) { - $self->_dbh(undef); - $self->{_dbh_gen}++; - return; - } - else { - $self->_verify_pid; - return 0 if !$self->_dbh; - } - return ($dbh->FETCH('Active') && $self->_ping); + #be on the safe side + local $self->_dbh->{RaiseError} = 1; + + return $self->_ping; +} + +sub _seems_connected { + my $self = shift; + + my $dbh = $self->_dbh + or return 0; + + if(defined $self->_conn_tid && $self->_conn_tid != threads->tid) { + $self->_dbh(undef); + $self->{_dbh_gen}++; + return 0; + } + else { + $self->_verify_pid; + return 0 if !$self->_dbh; } - return 0; + return $dbh->FETCH('Active'); } sub _ping { @@ -739,7 +755,9 @@ sub ensure_connected { Returns a C<$dbh> - a data base handle of class L. The returned handle is guaranteed to be healthy by implicitly calling L, and if -necessary performing a reconnection before returning. +necessary performing a reconnection before returning. Keep in mind that this +is very B on some database engines. Consider using L +instead. =cut @@ -754,17 +772,8 @@ sub dbh { return $self->_dbh; } -=head2 last_dbh - -This returns the B available C<$dbh> if any, or attempts to -connect and returns the resulting handle. This method differs from -L by not validating if a preexisting handle is still healthy -via L. Make sure you take appropriate precautions -when using this method, as the C<$dbh> may be useless at this point. - -=cut - -sub last_dbh { +# this is the internal "get dbh or connect (don't check)" method +sub _get_dbh { my $self = shift; $self->_populate_dbh unless $self->_dbh; return $self->_dbh; @@ -776,7 +785,7 @@ sub _sql_maker_args { return ( bindtype=>'columns', array_datatypes => 1, - limit_dialect => $self->last_dbh, + limit_dialect => $self->_get_dbh, %{$self->_sql_maker_opts} ); } @@ -797,6 +806,7 @@ sub _populate_dbh { my ($self) = @_; my @info = @{$self->_dbi_connect_info || []}; + $self->_dbh(undef); # in case ->connected failed we might get sent here $self->_dbh($self->_connect(@info)); $self->_conn_pid($$); @@ -1235,7 +1245,7 @@ sub insert { $updated_cols->{$col} = $to_insert->{$col} = $self->_sequence_fetch( 'nextval', $col_info->{sequence} || - $self->_dbh_get_autoinc_seq($self->last_dbh, $source) + $self->_dbh_get_autoinc_seq($self->_get_dbh, $source) ); } } @@ -2025,7 +2035,7 @@ Returns the database driver name. =cut -sub sqlt_type { shift->last_dbh->{Driver}->{Name} } +sub sqlt_type { shift->_get_dbh->{Driver}->{Name} } =head2 bind_attribute_by_data_type @@ -2271,8 +2281,6 @@ See L for a list of values for C<$sqlt_args>. sub deployment_statements { my ($self, $schema, $type, $version, $dir, $sqltargs) = @_; - # Need to be connected to get the correct sqlt_type - $self->last_dbh() unless $type; $type ||= $self->sqlt_type; $version ||= $schema->schema_version || '1.x'; $dir ||= './'; @@ -2317,10 +2325,9 @@ sub deploy { return if $line =~ /^\s+$/; # skip whitespace only $self->_query_start($line); eval { - # a previous error may invalidate $dbh - thus we need to use dbh() - # to guarantee a healthy $dbh (this is temporary until we get - # proper error handling on deploy() ) - $self->dbh->do($line); + # do a dbh_do cycle here, as we need some error checking in + # place (even though we will ignore errors) + $self->dbh_do (sub { $_[1]->do($line) }); }; if ($@) { carp qq{$@ (running "${line}")}; @@ -2349,7 +2356,7 @@ Returns the datetime parser class sub datetime_parser { my $self = shift; return $self->{datetime_parser} ||= do { - $self->last_dbh; + $self->_populate_dbh unless $self->_dbh; $self->build_datetime_parser(@_); }; } diff --git a/lib/DBIx/Class/Storage/DBI/MSSQL.pm b/lib/DBIx/Class/Storage/DBI/MSSQL.pm index 674d458..b0da553 100644 --- a/lib/DBIx/Class/Storage/DBI/MSSQL.pm +++ b/lib/DBIx/Class/Storage/DBI/MSSQL.pm @@ -30,14 +30,14 @@ sub insert_bulk { if ($identity_insert) { my $table = $source->from; - $self->last_dbh->do("SET IDENTITY_INSERT $table ON"); + $self->_get_dbh->do("SET IDENTITY_INSERT $table ON"); } $self->next::method(@_); if ($identity_insert) { my $table = $source->from; - $self->last_dbh->do("SET IDENTITY_INSERT $table OFF"); + $self->_get_dbh->do("SET IDENTITY_INSERT $table OFF"); } } @@ -68,7 +68,7 @@ sub insert { grep { not exists $to_insert->{$_} } (@pk_guids, @auto_guids); for my $guid_col (@get_guids_for) { - my ($new_guid) = $self->last_dbh->selectrow_array('SELECT NEWID()'); + my ($new_guid) = $self->_get_dbh->selectrow_array('SELECT NEWID()'); $updated_cols->{$guid_col} = $to_insert->{$guid_col} = $new_guid; } @@ -145,7 +145,7 @@ sub last_insert_id { shift->_identity } sub _svp_begin { my ($self, $name) = @_; - $self->last_dbh->do("SAVE TRANSACTION $name"); + $self->_get_dbh->do("SAVE TRANSACTION $name"); } # A new SAVE TRANSACTION with the same name releases the previous one. @@ -154,7 +154,7 @@ sub _svp_release { 1 } sub _svp_rollback { my ($self, $name) = @_; - $self->last_dbh->do("ROLLBACK TRANSACTION $name"); + $self->_get_dbh->do("ROLLBACK TRANSACTION $name"); } sub build_datetime_parser { diff --git a/lib/DBIx/Class/Storage/DBI/ODBC.pm b/lib/DBIx/Class/Storage/DBI/ODBC.pm index 7c3b739..d9b810a 100644 --- a/lib/DBIx/Class/Storage/DBI/ODBC.pm +++ b/lib/DBIx/Class/Storage/DBI/ODBC.pm @@ -8,7 +8,7 @@ use mro 'c3'; sub _rebless { my ($self) = @_; - my $dbtype = eval { $self->last_dbh->get_info(17) }; + my $dbtype = eval { $self->_get_dbh->get_info(17) }; unless ( $@ ) { # Translate the backend name into a perl identifier diff --git a/lib/DBIx/Class/Storage/DBI/ODBC/Microsoft_SQL_Server.pm b/lib/DBIx/Class/Storage/DBI/ODBC/Microsoft_SQL_Server.pm index 77c20ad..5279502 100644 --- a/lib/DBIx/Class/Storage/DBI/ODBC/Microsoft_SQL_Server.pm +++ b/lib/DBIx/Class/Storage/DBI/ODBC/Microsoft_SQL_Server.pm @@ -137,7 +137,7 @@ sub connect_call_use_server_cursors { my $self = shift; my $sql_rowset_size = shift || 2; - $self->_dbh->{odbc_SQL_ROWSET_SIZE} = $sql_rowset_size; + $self->_get_dbh->{odbc_SQL_ROWSET_SIZE} = $sql_rowset_size; } =head2 connect_call_use_MARS @@ -165,9 +165,9 @@ sub connect_call_use_MARS { if ($dsn !~ /MARS_Connection=/) { $self->_dbi_connect_info->[0] = "$dsn;MARS_Connection=Yes"; - my $connected = defined $self->_dbh; + my $was_connected = defined $self->_dbh; $self->disconnect; - $self->ensure_connected if $connected; + $self->ensure_connected if $was_connected; } } diff --git a/lib/DBIx/Class/Storage/DBI/Oracle.pm b/lib/DBIx/Class/Storage/DBI/Oracle.pm index c0edb9a..7a49b50 100644 --- a/lib/DBIx/Class/Storage/DBI/Oracle.pm +++ b/lib/DBIx/Class/Storage/DBI/Oracle.pm @@ -9,7 +9,7 @@ use mro 'c3'; sub _rebless { my ($self) = @_; - my $version = eval { $self->last_dbh->get_info(18); }; + my $version = eval { $self->_get_dbh->get_info(18); }; if ( !$@ ) { my ($major, $minor, $patchlevel) = split(/\./, $version); diff --git a/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm b/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm index 832da97..b97e34f 100644 --- a/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm +++ b/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm @@ -76,7 +76,7 @@ sub _dbh_get_autoinc_seq { sub _sequence_fetch { my ( $self, $type, $seq ) = @_; - my ($id) = $self->last_dbh->selectrow_array("SELECT ${seq}.${type} FROM DUAL"); + my ($id) = $self->_get_dbh->selectrow_array("SELECT ${seq}.${type} FROM DUAL"); return $id; } @@ -209,7 +209,7 @@ sub connect_call_datetime_setup { sub _svp_begin { my ($self, $name) = @_; - $self->last_dbh->do("SAVEPOINT $name"); + $self->_get_dbh->do("SAVEPOINT $name"); } =head2 source_bind_attributes @@ -263,7 +263,7 @@ sub _svp_release { 1 } sub _svp_rollback { my ($self, $name) = @_; - $self->last_dbh->do("ROLLBACK TO SAVEPOINT $name") + $self->_get_dbh->do("ROLLBACK TO SAVEPOINT $name") } =head1 AUTHOR diff --git a/lib/DBIx/Class/Storage/DBI/Pg.pm b/lib/DBIx/Class/Storage/DBI/Pg.pm index 91418a4..a664808 100644 --- a/lib/DBIx/Class/Storage/DBI/Pg.pm +++ b/lib/DBIx/Class/Storage/DBI/Pg.pm @@ -15,7 +15,7 @@ warn "DBD::Pg 2.9.2 or greater is strongly recommended\n" sub with_deferred_fk_checks { my ($self, $sub) = @_; - $self->last_dbh->do('SET CONSTRAINTS ALL DEFERRED'); + $self->_get_dbh->do('SET CONSTRAINTS ALL DEFERRED'); $sub->(); } @@ -90,26 +90,26 @@ sub bind_attribute_by_data_type { sub _sequence_fetch { my ( $self, $type, $seq ) = @_; - my ($id) = $self->last_dbh->selectrow_array("SELECT nextval('${seq}')"); + my ($id) = $self->_get_dbh->selectrow_array("SELECT nextval('${seq}')"); return $id; } sub _svp_begin { my ($self, $name) = @_; - $self->last_dbh->pg_savepoint($name); + $self->_get_dbh->pg_savepoint($name); } sub _svp_release { my ($self, $name) = @_; - $self->last_dbh->pg_release($name); + $self->_get_dbh->pg_release($name); } sub _svp_rollback { my ($self, $name) = @_; - $self->last_dbh->pg_rollback_to($name); + $self->_get_dbh->pg_rollback_to($name); } 1; diff --git a/lib/DBIx/Class/Storage/DBI/Sybase.pm b/lib/DBIx/Class/Storage/DBI/Sybase.pm index c7031a1..41b0c81 100644 --- a/lib/DBIx/Class/Storage/DBI/Sybase.pm +++ b/lib/DBIx/Class/Storage/DBI/Sybase.pm @@ -13,7 +13,7 @@ sub _rebless { my $self = shift; my $dbtype = eval { - @{$self->last_dbh + @{$self->_get_dbh ->selectrow_arrayref(qq{sp_server_info \@attribute_id=1}) }[2] }; diff --git a/lib/DBIx/Class/Storage/DBI/Sybase/Base.pm b/lib/DBIx/Class/Storage/DBI/Sybase/Base.pm index b375a8c..757d4d9 100644 --- a/lib/DBIx/Class/Storage/DBI/Sybase/Base.pm +++ b/lib/DBIx/Class/Storage/DBI/Sybase/Base.pm @@ -29,7 +29,7 @@ sub _ping { sub _placeholders_supported { my $self = shift; - my $dbh = $self->last_dbh; + my $dbh = $self->_get_dbh; return eval { # There's also $dbh->{syb_dynamic_supported} but it can be inaccurate for this diff --git a/lib/DBIx/Class/Storage/DBI/Sybase/Microsoft_SQL_Server.pm b/lib/DBIx/Class/Storage/DBI/Sybase/Microsoft_SQL_Server.pm index 6cda222..6a20ba4 100644 --- a/lib/DBIx/Class/Storage/DBI/Sybase/Microsoft_SQL_Server.pm +++ b/lib/DBIx/Class/Storage/DBI/Sybase/Microsoft_SQL_Server.pm @@ -11,7 +11,7 @@ use mro 'c3'; sub _rebless { my $self = shift; - my $dbh = $self->last_dbh; + my $dbh = $self->_get_dbh; if (not $self->_placeholders_supported) { bless $self, diff --git a/lib/DBIx/Class/Storage/DBI/mysql.pm b/lib/DBIx/Class/Storage/DBI/mysql.pm index 89c8f89..6224d53 100644 --- a/lib/DBIx/Class/Storage/DBI/mysql.pm +++ b/lib/DBIx/Class/Storage/DBI/mysql.pm @@ -40,28 +40,28 @@ sub sqlt_type { sub _svp_begin { my ($self, $name) = @_; - $self->last_dbh->do("SAVEPOINT $name"); + $self->_get_dbh->do("SAVEPOINT $name"); } sub _svp_release { my ($self, $name) = @_; - $self->last_dbh->do("RELEASE SAVEPOINT $name"); + $self->_get_dbh->do("RELEASE SAVEPOINT $name"); } sub _svp_rollback { my ($self, $name) = @_; - $self->last_dbh->do("ROLLBACK TO SAVEPOINT $name") + $self->_get_dbh->do("ROLLBACK TO SAVEPOINT $name") } sub is_replicating { - my $status = shift->last_dbh->selectrow_hashref('show slave status'); + my $status = shift->_get_dbh->selectrow_hashref('show slave status'); return ($status->{Slave_IO_Running} eq 'Yes') && ($status->{Slave_SQL_Running} eq 'Yes'); } sub lag_behind_master { - return shift->last_dbh->selectrow_hashref('show slave status')->{Seconds_Behind_Master}; + return shift->_get_dbh->selectrow_hashref('show slave status')->{Seconds_Behind_Master}; } # MySql can not do subquery update/deletes, only way is slow per-row operations. diff --git a/t/92storage_ping_count.t b/t/92storage_ping_count.t index 9dddd64..07659cb 100644 --- a/t/92storage_ping_count.t +++ b/t/92storage_ping_count.t @@ -22,15 +22,13 @@ my $ping_count = 0; } -# We do not count pings during deploy() because of the flux -# around sqlt. Eventually there should be no pings at all +# measure pings around deploy() separately my $schema = DBICTest->init_schema( sqlite_use_file => 1, no_populate => 1 ); -TODO: { - local $TODO = 'Unable to fix before proper deploy() error handling'; - is ($ping_count, 0, 'no _ping() calls during deploy'); - $ping_count = 0; -} +is ($ping_count, 0, 'no _ping() calls during deploy'); +$ping_count = 0; + + DBICTest->populate_schema ($schema);