Refactor/improve support of DBD::Sybase compiled against FreeTDS (mainly MSSQL)
Rafael Kitover [Fri, 24 Dec 2010 12:53:11 +0000 (13:53 +0100)]
Changes
lib/DBIx/Class/Storage/DBI/Sybase.pm
lib/DBIx/Class/Storage/DBI/Sybase/ASE.pm
lib/DBIx/Class/Storage/DBI/Sybase/FreeTDS.pm [new file with mode: 0644]
lib/DBIx/Class/Storage/DBI/Sybase/Microsoft_SQL_Server.pm
lib/DBIx/Class/Storage/DBI/Sybase/Microsoft_SQL_Server/NoBindVars.pm
t/74mssql.t

diff --git a/Changes b/Changes
index 6a1c165..dc243b4 100644 (file)
--- a/Changes
+++ b/Changes
@@ -38,6 +38,8 @@ Revision history for DBIx::Class
         - Automatically require the requested cursor class before use
           (RT#64795)
         - Work around a Firebird ODBC driver bug exposed by DBD::ODBC 1.29
+        - Fix (to the extent allowed by the driver) transaction support in
+          DBD::Sybase compiled against FreeTDS
         - Fix exiting via next warnings in ResultSource::sequence()
         - Fix stripping of table qualifiers in update/delete in arrayref
           condition elements
index abf15bf..79b8226 100644 (file)
@@ -45,6 +45,31 @@ sub _rebless {
   }
 }
 
+sub _init {
+  # once the driver is determined see if we need to insert the DBD::Sybase w/ FreeTDS fixups
+  # this is a dirty version of "instance role application", \o/ DO WANT Moo \o/
+  my $self = shift;
+  if (! $self->isa('DBIx::Class::Storage::DBI::Sybase::FreeTDS') and $self->using_freetds) {
+    $self->ensure_class_loaded('DBIx::Class::Storage::DBI::Sybase::FreeTDS');
+
+    my @isa = @{mro::get_linear_isa(ref $self)};
+    my $class = shift @isa; # this is our current ref
+
+    my $trait_class = $class . '::FreeTDS';
+    mro::set_mro ($trait_class, 'c3');
+    no strict 'refs';
+    @{"${trait_class}::ISA"} = ($class, 'DBIx::Class::Storage::DBI::Sybase::FreeTDS', @isa);
+
+    bless ($self, $trait_class);
+
+    Class::C3->reinitialize() if DBIx::Class::_ENV_::OLD_MRO;
+
+    $self->_init(@_);
+  }
+
+  $self->next::method(@_);
+}
+
 sub _ping {
   my $self = shift;
 
@@ -53,17 +78,25 @@ sub _ping {
   local $dbh->{RaiseError} = 1;
   local $dbh->{PrintError} = 0;
 
+# FIXME if the main connection goes stale, does opening another for this statement
+# really determine anything?
+
   if ($dbh->{syb_no_child_con}) {
-# if extra connections are not allowed, then ->ping is reliable
-    return try { $dbh->ping } catch { 0; };
+    return try {
+      $self->_connect(@{$self->_dbi_connect_info || [] })
+        ->do('select 1');
+      1;
+    }
+    catch {
+      0;
+    };
   }
 
   return try {
-# XXX if the main connection goes stale, does opening another for this statement
-# really determine anything?
     $dbh->do('select 1');
     1;
-  } catch {
+  }
+  catch {
     0;
   };
 }
@@ -97,33 +130,6 @@ sub using_freetds {
   return $self->_get_dbh->{syb_oc_version} =~ /freetds/i;
 }
 
-=head2 set_textsize
-
-When using FreeTDS and/or MSSQL, C<< $dbh->{LongReadLen} >> is not available,
-use this function instead. It does:
-
-  $dbh->do("SET TEXTSIZE $bytes");
-
-Takes the number of bytes, or uses the C<LongReadLen> value from your
-L<connect_info|DBIx::Class::Storage::DBI/connect_info> if omitted, lastly falls
-back to the C<32768> which is the L<DBD::Sybase> default.
-
-=cut
-
-sub set_textsize {
-  my $self = shift;
-  my $text_size =
-    shift
-      ||
-    try { $self->_dbi_connect_info->[-1]->{LongReadLen} }
-      ||
-    32768; # the DBD::Sybase default
-
-  return unless defined $text_size;
-
-  $self->_dbh->do("SET TEXTSIZE $text_size");
-}
-
 1;
 
 =head1 AUTHORS
index 0e57f02..f70db66 100644 (file)
@@ -173,35 +173,30 @@ sub disconnect {
   $self->next::method;
 }
 
+# This is only invoked for FreeTDS drivers by ::Storage::DBI::Sybase::FreeTDS
+sub _set_autocommit_stmt {
+  my ($self, $on) = @_;
+
+  return 'SET CHAINED ' . ($on ? 'OFF' : 'ON');
+}
+
 # Set up session settings for Sybase databases for the connection.
 #
 # Make sure we have CHAINED mode turned on if AutoCommit is off in non-FreeTDS
 # DBD::Sybase (since we don't know how DBD::Sybase was compiled.) If however
 # we're using FreeTDS, CHAINED mode turns on an implicit transaction which we
 # only want when AutoCommit is off.
-#
-# Also SET TEXTSIZE for FreeTDS because LongReadLen doesn't work.
 sub _run_connection_actions {
   my $self = shift;
 
   if ($self->_is_bulk_storage) {
-# this should be cleared on every reconnect
+    # this should be cleared on every reconnect
     $self->_began_bulk_work(0);
     return;
   }
 
-  if (not $self->using_freetds) {
-    $self->_dbh->{syb_chained_txn} = 1;
-  } else {
-    # based on LongReadLen in connect_info
-    $self->set_textsize;
-
-    if ($self->_dbh_autocommit) {
-      $self->_dbh->do('SET CHAINED OFF');
-    } else {
-      $self->_dbh->do('SET CHAINED ON');
-    }
-  }
+  $self->_dbh->{syb_chained_txn} = 1
+    unless $self->using_freetds;
 
   $self->next::method(@_);
 }
@@ -859,7 +854,7 @@ In L<connect_info|DBIx::Class::Storage::DBI/connect_info> to set:
 On connection for use with L<DBIx::Class::InflateColumn::DateTime>, using
 L<DateTime::Format::Sybase>, which you will need to install.
 
-This works for both C<DATETIME> and C<SMALLDATETIME> columns, although
+This works for both C<DATETIME> and C<SMALLDATETIME> columns, note that
 C<SMALLDATETIME> columns only have minute precision.
 
 =cut
@@ -883,12 +878,6 @@ sub connect_call_datetime_setup {
 }
 
 
-# ->begin_work and such have no effect with FreeTDS but we run them anyway to
-# let the DBD keep any state it needs to.
-#
-# If they ever do start working, the extra statements will do no harm (because
-# Sybase supports nested transactions.)
-
 sub _dbh_begin_work {
   my $self = shift;
 
@@ -898,29 +887,9 @@ sub _dbh_begin_work {
 
   $self->next::method(@_);
 
-  if ($self->using_freetds) {
-    $self->_get_dbh->do('BEGIN TRAN');
-  }
-
   $self->_began_bulk_work(1) if $self->_is_bulk_storage;
 }
 
-sub _dbh_commit {
-  my $self = shift;
-  if ($self->using_freetds) {
-    $self->_dbh->do('COMMIT');
-  }
-  return $self->next::method(@_);
-}
-
-sub _dbh_rollback {
-  my $self = shift;
-  if ($self->using_freetds) {
-    $self->_dbh->do('ROLLBACK');
-  }
-  return $self->next::method(@_);
-}
-
 # savepoint support using ASE syntax
 
 sub _svp_begin {
@@ -943,7 +912,7 @@ sub _svp_rollback {
 =head1 Schema::Loader Support
 
 As of version C<0.05000>, L<DBIx::Class::Schema::Loader> should work well with
-most (if not all) versions of Sybase ASE.
+most versions of Sybase ASE.
 
 =head1 FreeTDS
 
@@ -962,18 +931,22 @@ To see if you're using FreeTDS check C<< $schema->storage->using_freetds >>, or
 
   perl -MDBI -le 'my $dbh = DBI->connect($dsn, $user, $pass); print $dbh->{syb_oc_version}'
 
-Some versions of the libraries involved will not support placeholders, in which
-case the storage will be reblessed to
+It is recommended to set C<tds version> for your ASE server to C<5.0> in
+C</etc/freetds/freetds.conf>.
+
+Some versions or configurations of the libraries involved will not support
+placeholders, in which case the storage will be reblessed to
 L<DBIx::Class::Storage::DBI::Sybase::ASE::NoBindVars>.
 
 In some configurations, placeholders will work but will throw implicit type
 conversion errors for anything that's not expecting a string. In such a case,
 the C<auto_cast> option from L<DBIx::Class::Storage::DBI::AutoCast> is
 automatically set, which you may enable on connection with
-L<DBIx::Class::Storage::DBI::AutoCast/connect_call_set_auto_cast>. The type info
-for the C<CAST>s is taken from the L<DBIx::Class::ResultSource/data_type>
-definitions in your Result classes, and are mapped to a Sybase type (if it isn't
-already) using a mapping based on L<SQL::Translator>.
+L<connect_call_set_auto_cast|DBIx::Class::Storage::DBI::AutoCast/connect_call_set_auto_cast>.
+The type info for the C<CAST>s is taken from the
+L<DBIx::Class::ResultSource/data_type> definitions in your Result classes, and
+are mapped to a Sybase type (if it isn't already) using a mapping based on
+L<SQL::Translator>.
 
 In other configurations, placeholders will work just as they do with the Sybase
 Open Client libraries.
@@ -991,14 +964,14 @@ In addition, they are done on a separate connection so that it's possible to
 have active cursors when doing an insert.
 
 When using C<DBIx::Class::Storage::DBI::Sybase::ASE::NoBindVars> transactions
-are disabled, as there are no concurrency issues with C<SELECT @@IDENTITY> as
-it's a session variable.
+are unnecessary and not used, as there are no concurrency issues with C<SELECT
+@@IDENTITY> which is a session variable.
 
 =head1 TRANSACTIONS
 
-Due to limitations of the TDS protocol, L<DBD::Sybase>, or both, you cannot
-begin a transaction while there are active cursors, nor can you use multiple
-active cursors within a transaction. An active cursor is, for example, a
+Due to limitations of the TDS protocol and L<DBD::Sybase>, you cannot begin a
+transaction while there are active cursors, nor can you use multiple active
+cursors within a transaction. An active cursor is, for example, a
 L<ResultSet|DBIx::Class::ResultSet> that has been executed using C<next> or
 C<first> but has not been exhausted or L<reset|DBIx::Class::ResultSet/reset>.
 
@@ -1092,7 +1065,7 @@ L<populate|DBIx::Class::ResultSet/populate> call, eg.:
 B<NOTE:> the L<add_columns|DBIx::Class::ResultSource/add_columns>
 calls in your C<Result> classes B<must> list columns in database order for this
 to work. Also, you may have to unset the C<LANG> environment variable before
-loading your app, if it doesn't match the character set of your database.
+loading your app, as C<BCP -Y> is not yet supported in DBD::Sybase .
 
 When inserting IMAGE columns using this method, you'll need to use
 L</connect_call_blob_setup> as well.
@@ -1109,6 +1082,7 @@ represent them in your Result classes as:
     data_type => undef,
     default_value => \'getdate()',
     is_nullable => 0,
+    inflate_datetime => 1,
   }
 
 The C<data_type> must exist and must be C<undef>. Then empty inserts will work
@@ -1148,10 +1122,6 @@ Real limits and limited counts using stored procedures deployed on startup.
 
 =item *
 
-Adaptive Server Anywhere (ASA) support
-
-=item *
-
 Blob update with a LIKE query on a blob, without invalidating the WHERE condition.
 
 =item *
diff --git a/lib/DBIx/Class/Storage/DBI/Sybase/FreeTDS.pm b/lib/DBIx/Class/Storage/DBI/Sybase/FreeTDS.pm
new file mode 100644 (file)
index 0000000..feb50fe
--- /dev/null
@@ -0,0 +1,116 @@
+package DBIx::Class::Storage::DBI::Sybase::FreeTDS;
+
+use strict;
+use warnings;
+use base qw/DBIx::Class::Storage::DBI::Sybase/;
+use mro 'c3';
+use Try::Tiny;
+use namespace::clean;
+
+=head1 NAME
+
+DBIx::Class::Storage::DBI::Sybase - Base class for drivers using L<DBD::Sybase>
+over FreeTDS.
+
+=head1 DESCRIPTION
+
+This is the base class for Storages designed to work with L<DBD::Sybase> over
+FreeTDS.
+
+It is a subclass of L<DBIx::Class::Storage::DBI::Sybase>.
+
+=head1 METHODS
+
+=cut
+
+# The subclass storage driver defines _set_autocommit_stmt
+# for MsSQL it is SET IMPLICIT_TRANSACTIONS ON/OFF
+# for proper Sybase it's SET CHAINED ON/OFF
+sub _set_autocommit {
+  my $self = shift;
+
+  if ($self->_dbh_autocommit) {
+    $self->_dbh->do($self->_set_autocommit_stmt(1));
+  } else {
+    $self->_dbh->do($self->_set_autocommit_stmt(0));
+  }
+}
+
+# Handle AutoCommit and SET TEXTSIZE because LongReadLen doesn't work.
+#
+sub _run_connection_actions {
+  my $self = shift;
+
+  # based on LongReadLen in connect_info
+  $self->set_textsize;
+
+  $self->_set_autocommit;
+
+  $self->next::method(@_);
+}
+
+=head2 set_textsize
+
+When using DBD::Sybase with FreeTDS, C<< $dbh->{LongReadLen} >> is not available,
+use this function instead. It does:
+
+  $dbh->do("SET TEXTSIZE $bytes");
+
+Takes the number of bytes, or uses the C<LongReadLen> value from your
+L<connect_info|DBIx::Class::Storage::DBI/connect_info> if omitted, lastly falls
+back to the C<32768> which is the L<DBD::Sybase> default.
+
+=cut
+
+sub set_textsize {
+  my $self = shift;
+  my $text_size =
+    shift
+      ||
+    try { $self->_dbic_cinnect_attributes->{LongReadLen} }
+      ||
+    32768; # the DBD::Sybase default
+
+  $self->_dbh->do("SET TEXTSIZE $text_size");
+}
+
+sub _dbh_begin_work {
+  my $self = shift;
+
+  if ($self->{_in_dbh_do}) {
+    $self->_dbh->do('BEGIN TRAN');
+  }
+  else {
+    $self->dbh_do(sub { $_[1]->do('BEGIN TRAN') });
+  }
+}
+
+sub _dbh_commit {
+  my $self = shift;
+
+  my $dbh = $self->_dbh
+    or $self->throw_exception('cannot COMMIT on a disconnected handle');
+
+  $dbh->do('COMMIT');
+}
+
+sub _dbh_rollback {
+  my $self = shift;
+
+  my $dbh  = $self->_dbh
+    or $self->throw_exception('cannot ROLLBACK on a disconnected handle');
+
+  $dbh->do('ROLLBACK');
+}
+
+1;
+
+=head1 AUTHORS
+
+See L<DBIx::Class/CONTRIBUTORS>.
+
+=head1 LICENSE
+
+You may distribute this code under the same terms as Perl itself.
+
+=cut
index 068a1a2..f1ea17c 100644 (file)
@@ -11,6 +11,29 @@ use mro 'c3';
 
 use DBIx::Class::Carp;
 
+=head1 NAME
+
+DBIx::Class::Storage::DBI::Sybase::Microsoft_SQL_Server - Support for Microsoft
+SQL Server via DBD::Sybase
+
+=head1 SYNOPSIS
+
+This subclass supports MSSQL server connections via L<DBD::Sybase>.
+
+=head1 DESCRIPTION
+
+This driver tries to determine whether your version of L<DBD::Sybase> and
+supporting libraries (usually FreeTDS) support using placeholders, if not the
+storage will be reblessed to
+L<DBIx::Class::Storage::DBI::Sybase::Microsoft_SQL_Server::NoBindVars>.
+
+The MSSQL specific functionality is provided by
+L<DBIx::Class::Storage::DBI::MSSQL>.
+
+=head1 METHODS
+
+=cut
+
 __PACKAGE__->datetime_parser_type(
   'DBIx::Class::Storage::DBI::Sybase::Microsoft_SQL_Server::DateTime::Format'
 );
@@ -21,6 +44,19 @@ sub _rebless {
 
   return if ref $self ne __PACKAGE__;
   if (not $self->_use_typeless_placeholders) {
+    carp <<'EOF' unless $ENV{DBIC_MSSQL_FREETDS_LOWVER_NOWARN};
+Placeholders do not seem to be supported in your configuration of
+DBD::Sybase/FreeTDS.
+
+This means you are taking a large performance hit, as caching of prepared
+statements is disabled.
+
+Make sure to configure your server with "tds version" of 8.0 or 7.0 in
+/etc/freetds/freetds.conf .
+
+To turn off this warning, set the DBIC_MSSQL_FREETDS_LOWVER_NOWARN environment
+variable.
+EOF
     require
       DBIx::Class::Storage::DBI::Sybase::Microsoft_SQL_Server::NoBindVars;
     bless $self,
@@ -29,35 +65,11 @@ sub _rebless {
   }
 }
 
-sub _run_connection_actions {
-  my $self = shift;
-
-  # LongReadLen doesn't work with MSSQL through DBD::Sybase, and the default is
-  # huge on some versions of SQL server and can cause memory problems, so we
-  # fix it up here (see ::DBI::Sybase.pm)
-  $self->set_textsize;
-
-  $self->next::method(@_);
-}
-
-sub _dbh_begin_work {
-  my $self = shift;
-
-  $self->_get_dbh->do('BEGIN TRAN');
-}
-
-sub _dbh_commit {
-  my $self = shift;
-  my $dbh  = $self->_dbh
-    or $self->throw_exception('cannot COMMIT on a disconnected handle');
-  $dbh->do('COMMIT');
-}
+# invoked only if DBD::Sybase is compiled against FreeTDS
+sub _set_autocommit_stmt {
+  my ($self, $on) = @_;
 
-sub _dbh_rollback {
-  my $self = shift;
-  my $dbh  = $self->_dbh
-    or $self->throw_exception('cannot ROLLBACK on a disconnected handle');
-  $dbh->do('ROLLBACK');
+  return 'SET IMPLICIT_TRANSACTIONS ' . ($on ? 'OFF' : 'ON');
 }
 
 sub _get_server_version {
@@ -138,25 +150,6 @@ sub format_datetime {
 
 1;
 
-=head1 NAME
-
-DBIx::Class::Storage::DBI::Sybase::Microsoft_SQL_Server - Support for Microsoft
-SQL Server via DBD::Sybase
-
-=head1 SYNOPSIS
-
-This subclass supports MSSQL server connections via L<DBD::Sybase>.
-
-=head1 DESCRIPTION
-
-This driver tries to determine whether your version of L<DBD::Sybase> and
-supporting libraries (usually FreeTDS) support using placeholders, if not the
-storage will be reblessed to
-L<DBIx::Class::Storage::DBI::Sybase::Microsoft_SQL_Server::NoBindVars>.
-
-The MSSQL specific functionality is provided by
-L<DBIx::Class::Storage::DBI::MSSQL>.
-
 =head1 AUTHOR
 
 See L<DBIx::Class/CONTRIBUTORS>.
index 622cf1e..5d266bc 100644 (file)
@@ -12,6 +12,8 @@ use mro 'c3';
 sub _init {
   my $self = shift;
   $self->disable_sth_caching(1);
+
+  $self->next::method(@_);
 }
 
 1;
index 223709d..57d7c69 100644 (file)
@@ -42,6 +42,8 @@ for my $storage_type (@test_storages) {
     $schema->storage->_use_typeless_placeholders (0);
   }
 
+  local $ENV{DBIC_MSSQL_FREETDS_LOWVER_NOWARN} = 1; # disable nobindvars warning
+
   $schema->storage->ensure_connected;
 
   if ($storage_type =~ /NoBindVars\z/) {
@@ -192,6 +194,66 @@ SQL
     $rs->delete;
   }
 
+  # test transaction handling on a disconnected handle
+  my $wrappers = {
+    no_transaction => sub { shift->() },
+    txn_do => sub { my $code = shift; $schema->txn_do(sub { $code->() } ) },
+    txn_begin => sub { $schema->txn_begin; shift->(); $schema->txn_commit },
+    txn_guard => sub { my $g = $schema->txn_scope_guard; shift->(); $g->commit },
+  };
+  for my $wrapper (keys %$wrappers) {
+    $rs->delete;
+
+    # a reconnect should trigger on next action
+    $schema->storage->_get_dbh->disconnect;
+
+    lives_and {
+      $wrappers->{$wrapper}->( sub {
+        $rs->create({ amount => 900 + $_ }) for 1..3;
+      });
+      is $rs->count, 3;
+    } "transaction on disconnected handle with $wrapper wrapper";
+  }
+
+  TODO: {
+    local $TODO = 'Transaction handling with multiple active statements will '
+                 .'need eager cursor support.';
+
+    # test transaction handling on a disconnected handle with multiple active
+    # statements
+    my $wrappers = {
+      no_transaction => sub { shift->() },
+      txn_do => sub { my $code = shift; $schema->txn_do(sub { $code->() } ) },
+      txn_begin => sub { $schema->txn_begin; shift->(); $schema->txn_commit },
+      txn_guard => sub { my $g = $schema->txn_scope_guard; shift->(); $g->commit },
+    };
+    for my $wrapper (keys %$wrappers) {
+      $rs->reset;
+      $rs->delete;
+      $rs->create({ amount => 1000 + $_ }) for (1..3);
+
+      my $artist_rs = $schema->resultset('Artist')->search({
+        name => { -like => 'Artist %' }
+      });;
+
+      $rs->next;
+
+      my $map = [ ['Artist 1', '1002.00'], ['Artist 2', '1003.00'] ];
+
+      lives_and {
+        my @results;
+
+        $wrappers->{$wrapper}->( sub {
+          while (my $money = $rs->next) {
+            my $artist = $artist_rs->next;
+            push @results, [ $artist->name, $money->amount ];
+          };
+        });
+
+        is_deeply \@results, $map;
+      } "transactions with multiple active statement with $wrapper wrapper";
+    }
+  }
 
   # test RNO detection when version detection fails
   SKIP: {
@@ -231,6 +293,31 @@ lives_ok (sub {
   is ($artist->id, 1, 'Artist retrieved successfully');
 }, 'Query-induced autoconnect works');
 
+# test AutoCommit=0
+{
+  local $ENV{DBIC_UNSAFE_AUTOCOMMIT_OK} = 1;
+  my $schema2 = DBICTest::Schema->connect($dsn, $user, $pass, { AutoCommit => 0 });
+
+  my $rs = $schema2->resultset('Money');
+
+  $rs->delete;
+  $schema2->txn_commit;
+
+  is $rs->count, 0, 'initially empty'
+    || diag ('Found row with amount ' . $_->amount) for $rs->all;
+
+  $rs->create({ amount => 3000 });
+  $schema2->txn_rollback;
+
+  is $rs->count, 0, 'rolled back in AutoCommit=0'
+    || diag ('Found row with amount ' . $_->amount) for $rs->all;
+
+  $rs->create({ amount => 4000 });
+  $schema2->txn_commit;
+
+  cmp_ok $rs->first->amount, '==', 4000, 'committed in AutoCommit=0';
+}
+
 done_testing;
 
 # clean up our mess