On Mon, Sep 09, 2002 at 10:33:18AM -0400, Steven N. Hirsch wrote:
> Tim, et al,
>
> Here is a patch againt DBI-1.30 which adds some important
> functionality and fixes a minor problem or two.
Ohhh, thanks. This is good stuff. I've a few assorted comments and
suggested tweaks...
> (Minor Highlights)
>
> The notion of Proxy <--> ProxyServer protocol level is introduced to
> maintain sanity and interoperability with older versions. There is
> almost certainly a better way to implement it, but I wanted to get
> something that worked out there for comments and testing.
This is a valuable concept and important for the future. In some places
in the code you've called the thing 'version' and in others 'proto'.
Please change all to include both words to make things unambiguous.
> Support is added to Proxy.pm for passing in PlRPC specific parameters;
> specifically I needed to set 'maxmessage' in the DBI_AUTOPROXY
> environment string. Anything prefixed with 'rpc_' has this trimmed
> off and passed along to the PlRPC object.
Handy, but lets make it anything prefixed with 'proxy_rpc_' ...
so we keep within the drivers own private namespace.
> A bug in the ProxyServer table_info() method was fixed. With Perl
> 5.8.0 and DBD::DB2 driver v1.26, calling $sth->fetch() in scalar
> context is a losing proposition, resulting in the dreaded "Can't use
> scalar '1' as an array ref.." exception.
Why? A DBD::DB2 driver bug?
> I'm not sure if anyone has ever used 'dbish' to work with a proxied
> database by setting DBI_AUTOPROXY, but as it stands the DBD::Sponge
> pseudo driver gets invoked on the far end unless measures are taken.
> This _should_ work, I suppose, but the dbish 'table_info' command
> causes a dramatic core dump on the client side when run in this
> manner. I suspect a bug in Storable, but have not been motivated to
> track it down. Should be repeatable if anyone else wants to pursue
> it. Forcibly clearing the environment variable after the initial
> connection is setup ensures that Sponge runs on the client. Life is
> then Good.
I think a better fix would be to change this line in DBI::connect()
to include 'Sponge':
if ($ENV{DBI_AUTOPROXY} && $driver ne 'Proxy' && $driver ne 'Switch') {
> The dbish semantics for table_info also require that a Proxy
> table_info call set the statement handle 'Active'. This now works
> properly.
Thanks. (Though I'm not clear on which bit of the patch addresses that.)
> (Major Highlights)
>
> Synchronous prepare / execute:
>
> I remain unconvinced that the original "lazy" method (deferring
> prepare until the first call to execute) was truly saving any time.
For applications not using placeholders and doing lots of non-selects
it can significantly reduce the (expensive) round-trips to the proxy server.
> Further, although the DBI spec may legalize deferred prepare,
> enforcing this on the proxy session when the underlying remote
> database has meaningful and distinct prepare behavior violates the
> principle of least surprise.
Adding support for non-lazy prepare is a valuable addition - but
it needs to be made configurable at the dbh level via an attribute.
> I make liberal use of positioned updates in my applications, and DB2
> requires that a statement be prepared by the database before a valid
> cursor name can be obtained. Also, "fetch-ahead" wreaks havoc in this
> situation since we want the (remote) database cursor to remain on the
> row being read on the client.
Thanks.
> Propagation of remote database errors:
>
> It is often desirable to suppress DBI exceptions and have the program
> logic deal with common (or expected) cases. For this to work
> transparently, the application must see the actual error information
> generated by the remote database. My proposal is to always turn off
> 'RaiseError' on the ProxyServer and pass back the database-specific
> codes on failure.
The code as patched treats execute() returning 0 as an error.
It's should check for undef instead.
But I don't see why execute() should be made into a special case.
I'd much rather see RaiseError always used on the server (for
simplicity, consistency, and reliability) and have the code
fixed to pass back err & errstr & state properly.
The basic pattern, oft repeated, in the DBI::Proxy code is:
... = eval { $remote_h->foo(...) };
return DBI::set_err($local_h, 1, $@) if $@;
which is just plain insufficient and needs fixing. The easiest
approach I can see is probably to use a HandleError => sub { ... }
in the server to edit the error message so it contains the err and
state values in a way that can be reliably extracted later, but still
human readable and compatible with ShowErrorStatement and ParamValues etc.
A bit of a hack but very workable. Something roughly like:
HandleError => sub {
my $err = $_[1]->err;
my $state = $_[1]->state || '';
$_[0] .= " [err=$err,state=$state]";
return 0;
}
the DBI::Proxy mantra could be changed to abtract out the handling into a sub:
... = eval { $remote_h->foo(...) };
return DBD::Proxy::proxy_set_err($local_h, $@) if $@;
sub DBD::Proxy::proxy_set_err {
my ($h, $errmsg) = @_;
my ($err, $state) = ($errmsg =~ s/ \[err=(.*?),state=(.*?)\]//) ? ($1,$2)
: (1,'');
return DBI::set_err($h, $err, $errmsg, $state);
}
That would be backwards compatible and would fix *every* remote call
to pass back err & errstr & state properly.
> All this has been tested in a pre-production environment for a bit
> over a week, using DB2 V7.2 as the remote database. I would greatly
> appreciate any and all comments, criticisms and suggestions pertaining
> to this code.
I'm very glad to see the proxy code getting more use and I'd love
to see it improved. Thanks for helping out.
Tim.
