Re: DBI::DBD: attribute handling (outer/inner handles, $h->STORE vs. $h->{})

[Tim Bunce]

On Thu, Dec 16, 2004 at 10:58:35AM +0100, Steffen Goeldner wrote:
> However, it's placed below the connect() method:
> 
>   
> 
> 
> where we have an outer handle by way of an exception!
> Instead of changing the nice description, I'd rather change the
> example code (see attached patch). Advantages that accrued:
> 
>  - Now, we have the inner handle the text speaks about.
>  - The example is shorter.
>  - The example shows both, the STORE method and direct hash access.
>  - The example shows a realistic DBI attribute ('Active') instead of
>whatever gets extracted from DSN (and may cause troubles in STORE).

(STORE should always be able to handle any kind of attribute,
avoiding it is purely an optimization. Less relevant in these days
of gigahertz processors.)

>  - The example shows a realistic driver specific attribute.
>  - The examples are more consistent ($dbh is always the inner handle).

Thanks, applied.

Tim.

Re: DBI::DBD: Other statement methods

[Tim Bunce]

On Fri, Dec 17, 2004 at 02:15:57PM +0100, Steffen Goeldner wrote:
> 
> P.S.: This was my last patch this year.

Thanks, applied.

>   I wish all a Merry Christmas and a Happy New Year!

Thanks Steffen. And many thanks for all your well-considered
and well-presented patches.

Tim.

Re: DBD::Pg vs Pg

[David Wheeler]

On Dec 17, 2004, at 12:29 PM, Jochen Wiedmann wrote:
Good heavens, is that a difference?
Yes. perldoc perlunicode.
Regards,
David

Re: DBD::Pg vs Pg

[Jochen Wiedmann]

David Wheeler wrote:
I assure you that we'll be happy to deprecate it and eventually dump it 
once DBI has full UTF-8 (and, specifically, utf8) support.
Good heavens, is that a difference?
Shaking my head,
Jochen

Re: DBD::Pg vs Pg

[David Wheeler]

On Dec 17, 2004, at 3:20 AM, Tim Bunce wrote:
imp_dbh->pg_enable_utf8 defaults to off.
Ah, good.
I assure you that we'll be happy to deprecate it and eventually dump it 
once DBI has full UTF-8 (and, specifically, utf8) support.

Cheers,
David

DBI::DBD: Other statement methods

[Steffen Goeldner]

"The Perl DBI Database Driver Writer's Guide" mentions
table_info() and type_info_all() as "Other statement
methods":
  
Because the Metadata methods are treated in their own
section:
  
we can (IMO) discard the paragraph without substitution.
Steffen
P.S.: This was my last patch this year.
  I wish all a Merry Christmas and a Happy New Year!
Index: lib/DBI/DBD.pm
===
--- lib/DBI/DBD.pm  (revision 634)
+++ lib/DBI/DBD.pm  (working copy)
@@ -1091,10 +1091,6 @@
 A trivial C method to discard the stored data and do
 $sth->SUPER::finish;
 
-A C method to return details of available tables.
-
-A C method to return details of supported types.
-
 If you've defined a parse_trace_flag() method in ::db you'll also want
 it in ::st, so just alias it in:
 

Re: DBI::DBD: attribute handling (outer/inner handles, $h->STORE vs. $h->{})

[Tim Bunce]

Thanks, applied.

On Fri, Dec 17, 2004 at 10:30:18AM +0100, Steffen Goeldner wrote:
>  
> -  sub rows { shift->FETCH('drv_rows') }
> +  sub rows { shift->{drv_rows} }

I've added this note:

The rows method for this driver can be implemented like this:

  sub rows { shift->{drv_rows} }

because it knows in advance how many rows it has fetched.
Alternatively you could delete that method and so fallback
to the DBI's own method which does the right thing based
on the number of calls to _set_fbav().

Tim.

Re: DBD::ExampleP: attribute handling (outer/inner handles, $h->STORE vs. $h->{})

[Tim Bunce]

Thanks, applied.

Tim.

On Fri, Dec 17, 2004 at 11:33:24AM +0100, Steffen Goeldner wrote:
> Changed DBD::ExampleP to reflect the recent changes to DBI::DBD.
> 
> 
> Steffen

> Index: lib/DBD/ExampleP.pm
> ===
> --- lib/DBD/ExampleP.pm   (revision 633)
> +++ lib/DBD/ExampleP.pm   (working copy)
> @@ -58,13 +58,10 @@
>  
>  sub connect { # normally overridden, but a handy default
>  my($drh, $dbname, $user, $auth)= @_;
> -my($this) = DBI::_new_dbh($drh, {
> - 'Name' => $dbname,
> - 'User' => $user,
> - examplep_get_info => {},
> - });
> - $this->STORE(Active => 1);
> -$this;
> +my ($outer, $dbh) = DBI::_new_dbh($drh, { Name => $dbname });
> +$dbh->STORE('Active', 1);
> +$dbh->{examplep_get_info} = {};
> +return $outer;
>  }
>  
>  sub data_sources {
> @@ -97,7 +94,7 @@
>   # No we have DBI::DBM etc ExampleP should be deprecated
>   }
>  
> - my ($outer, $inner) = DBI::_new_sth($dbh, {
> + my ($outer, $sth) = DBI::_new_sth($dbh, {
>   'Statement' => $statement,
>   }, ['example implementors private data '.__PACKAGE__]);
>  
> @@ -109,7 +106,7 @@
>  
>   $outer->STORE('NUM_OF_FIELDS' => scalar(@fields));
>  
> - $inner->{'dbd_ex_dir'} = $dir if defined($dir) && $dir !~ /\?/;
> + $sth->{dbd_ex_dir} = $dir if defined($dir) && $dir !~ /\?/;
>   $outer->STORE('NUM_OF_PARAMS' => ($dir) ? $dir =~ tr/?/?/ : 0);
>  
>   if (@fields) {

Re: Re: Re: DBD::Pg vs Pg

[Tim Bunce]

On Thu, Dec 16, 2004 at 07:25:13PM -0500, Rudy Lippan wrote:
> On Tue, 14 Dec 2004, Tim Bunce wrote:
> 
> > On Tue, Dec 14, 2004 at 02:54:39PM +0900, [EMAIL PROTECTED] wrote:
> > > Hi.
> 
> > if (PQgetisnull(imp_sth->result, imp_sth->cur_tuple, i)) {
> > sv_setsv(sv, &sv_undef);
> > 
> > That would be significantly faster as:
> > SvROK(sv) ? sv_unref(sv) : SvOK_off(sv);
> 
> Intersting. 1) How significant,

Significant is a wonderful word. It carries weight without much
absolute meaning :-)

In this case "significant" might mean 10 times as fast (guess),
but the effect on overall performance of DBD::Pg would be hard
to measure - even for result sets with many nulls.

Both SvROK and SvOK_off are simple macros expanding to very little code.
No function calls at all. sv_setsv, on the other hand, calls
sv_setsv_flags() which has a whole much of macros and tests before
it then short-circuits to doing SvOK_off.

> and 2) Do you think it would be worth the reduction in legability?

I don't see a reduction in legability :)

> > } else {
> > value = (char*)PQgetvalue(imp_sth->result, imp_sth->cur_tuple, 
> > i); 
> > 
> > pg_type = PQftype(imp_sth->result, i);
> > type_info = pg_type_data(pg_type);
> > 
> > Types don't vary between rows so the PQftype() and pg_type_data()
> > calls could be factored out of this inner-inner loop.
> 
> I see your point, but how much do you loose, 20 or so pushes and pops, a few 
> mvs
> per loop iteration, and how does that compare to pulling the data off the 
> wire?  

Doesn't pg fetch and cache all the rows off the wire in advance? 
So then the application fetching has a much shorter code path and
you stand a better chance of fitting more of it into the level2
data and instruction cache of the cpu. It all adds up either way.

> And then if you factor it out, you have to replace the above code with 
> something
> that indexes into array and that would probably make the code not quite as
> stright forward, but I am not totally against it; I just don't know if it 
> would 
> shave that much time off of the fetch?

You won't know unless you try :) I'm just pointing out that there's
possibly room for improvement. I've no idea how much - but you do have
the PG extension to "complete" with.

> imp_dbh->pg_enable_utf8 defaults to off.

Ah, good.

Tim.

DBD::ExampleP: attribute handling (outer/inner handles, $h->STORE vs. $h->{})

[Steffen Goeldner]

Changed DBD::ExampleP to reflect the recent changes to DBI::DBD.
Steffen
Index: lib/DBD/ExampleP.pm
===
--- lib/DBD/ExampleP.pm (revision 633)
+++ lib/DBD/ExampleP.pm (working copy)
@@ -58,13 +58,10 @@
 
 sub connect { # normally overridden, but a handy default
 my($drh, $dbname, $user, $auth)= @_;
-my($this) = DBI::_new_dbh($drh, {
-   'Name' => $dbname,
-   'User' => $user,
-   examplep_get_info => {},
-   });
-   $this->STORE(Active => 1);
-$this;
+my ($outer, $dbh) = DBI::_new_dbh($drh, { Name => $dbname });
+$dbh->STORE('Active', 1);
+$dbh->{examplep_get_info} = {};
+return $outer;
 }
 
 sub data_sources {
@@ -97,7 +94,7 @@
# No we have DBI::DBM etc ExampleP should be deprecated
}
 
-   my ($outer, $inner) = DBI::_new_sth($dbh, {
+   my ($outer, $sth) = DBI::_new_sth($dbh, {
'Statement' => $statement,
}, ['example implementors private data '.__PACKAGE__]);
 
@@ -109,7 +106,7 @@
 
$outer->STORE('NUM_OF_FIELDS' => scalar(@fields));
 
-   $inner->{'dbd_ex_dir'} = $dir if defined($dir) && $dir !~ /\?/;
+   $sth->{dbd_ex_dir} = $dir if defined($dir) && $dir !~ /\?/;
$outer->STORE('NUM_OF_PARAMS' => ($dir) ? $dir =~ tr/?/?/ : 0);
 
if (@fields) {

Re: DBI::DBD: attribute handling (outer/inner handles, $h->STORE vs. $h->{})

[Steffen Goeldner]

Tim Bunce wrote:
I've added this:
[...]
Comments, or patches against subversion, most welcome.
Fixed this daVinci bug:
  -  my ($drh, $dbname, $user, $auth, $attr) = @_;
  +  my ($drh, $dr_dsn, $user, $auth, $attr) = @_;
as well as a few typos and the like.
Steffen
Index: lib/DBI/DBD.pm
===
--- lib/DBI/DBD.pm  (revision 633)
+++ lib/DBI/DBD.pm  (working copy)
@@ -622,7 +622,7 @@
 The I method is the driver handle constructor. It's a
 reasonable example of how DBI implements its handles. There are three
 kinds: B (typically stored in C<$drh>; from now on
-called C of C<$drh>), B (from now on called C or
+called C or C<$drh>), B (from now on called C or
 C<$dbh>) and B (from now on called C or
 C<$sth>).
 
@@ -664,7 +664,7 @@
 interpreter:
 
   sub CLONE {
-undef $rdh;
+undef $drh;
   }
 
 =head3 The DBD::Driver::dr package
@@ -700,7 +700,7 @@
 
   # Process attributes from the DSN; we assume ODBC syntax
   # here, that is, the DSN looks like var1=val1;...;varN=valN
-  foreach my $var ( split /;/, $dbname ) {
+  foreach my $var ( split /;/, $dr_dsn ) {
   my ($attr_name, $attr_value) = split '=', $var, 2;
  return $drh->set_err(1, "Can't parse DSN part '$var'")
   unless defined $attr_value;
@@ -723,10 +723,10 @@
 
   # Assume you can attach to your database via drv_connect:
   my $connection = drv_connect($db, $host, $port, $user, $auth)
-  or return $drh->set_err(1, "Can't connect to $dbname: ...");
+  or return $drh->set_err(1, "Can't connect to $dr_dsn: ...");
 
   # create a 'blank' dbh (call superclass constructor)
-  my ($outer, $dbh) = DBI::_new_dbh($drh, { Name => $dbname });
+  my ($outer, $dbh) = DBI::_new_dbh($drh, { Name => $dr_dsn });
 
   $dbh->STORE('Active', 1 );
   $dbh->{drv_connection} = $connection;
@@ -775,7 +775,7 @@
 
   sub data_sources
   {
-  my($srh, $attr) = @_;
+  my($drh, $attr) = @_;
   my(@list) = ();
   # You need more sophisticated code than this to set @list...
   push @list, "dbi:Driver:abc";
@@ -996,7 +996,7 @@
   my $dbh = $sth->{Database};
   $val = $dbh->quote($sth, $type);
   }
-  my $params = $sth->FETCH('drv_params');
+  my $params = $sth->{drv_params};
   $params->[$pNum-1] = $val;
   1;
   }
@@ -1009,7 +1009,7 @@
   $sth->finish if $sth->FETCH('Active');
 
   my $params = (@bind_values) ?
-  [EMAIL PROTECTED] : $sth->FETCH('drv_params');
+  [EMAIL PROTECTED] : $sth->{drv_params};
   my $numParam = $sth->FETCH('NUM_OF_PARAMS');
   return $sth->set_err(1, "Wrong number of parameters")
   if @$params != $numParam;
@@ -1028,8 +1028,8 @@
 There are a number of things you should note here.
 We setup the NUM_OF_FIELDS attribute
 here, because this is essential for I to work.
-We use attribute I<$sth->{'Statement'}> which we created
-within I. The attribute I<$sth->{'Database'}>, which is
+We use attribute C<$sth-E{Statement}> which we created
+within I. The attribute C<$sth-E{Database}>, which is
 nothing else than the I, was automatically created by DBI.
 
 Finally note that (as specified in the DBI specification) we return the
@@ -1048,10 +1048,10 @@
   sub fetchrow_arrayref
   {
   my ($sth) = @_;
-  my $data = $sth->FETCH('drv_data');
+  my $data = $sth->{drv_data};
   my $row = shift @$data;
   if (!$row) {
-  $sth->STORE(Active => 0); # mark as no longer active 
+  $sth->STORE(Active => 0); # mark as no longer active
   return undef;
   }
   if ($sth->FETCH('ChopBlanks')) {
@@ -1061,7 +1061,7 @@
   }
   *fetch = \&fetchrow_arrayref; # required alias for fetchrow_arrayref
 
-  sub rows { shift->FETCH('drv_rows') }
+  sub rows { shift->{drv_rows} }
 
 Note the use of the method I<_set_fbav>: This is required so that
 I and I work.

Re: DBI::DBD: attribute handling (outer/inner handles, $h->STORE vs. $h->{})

[Steffen Goeldner]

Tim Bunce wrote:
Actually it's only the first letter that matters (so foo_Bar is okay,
if you like that kind of thing :)
I've changed the text to read:
: Note the prefix I in the attribute names: it is required that
: all your private attributes use a lowercase prefix unique to your driver.
: The DBI contains a registry of known driver prefixes and may one day
: warn about unknown attributes that don't have a registered prefix.
O.k., that's clear now.
Is STORE() responsible for checking/changing this or should the
connect() code above handle this?
I'm not quite sure what you're asking there. I presume you asking
about any invalid/misspelt driver-private attribute not just mixed
case ones.
Exactly! With the clarification above, my question is answered.
Steffen

Parse/Compose DSNs

[Tim Bunce]

On Thu, Dec 16, 2004 at 10:39:17PM +0100, Jochen Wiedmann wrote:
> Jeff Zucker wrote:
> 
> >>After the "dbi:AnyData:"; everything should be in "name=value;" format.
> >>So "dbi:AnyData:format=XML;table=Test;file=test.xml";
> 
> Btw, Tim, how about the following suggestion: The discussion on the DSN 
> format is, IMO, mainly caused by the fact, that the DBI doesn't support 
> parsing the DSN.

Not quite true. See DBI->parse_dsn($dsn)

> Would it be possible, that a driver requests, that he doesn't receive 
> the DSN as a string, but as a hash ref? IMO, this would terminate such 
> discussions once and forever. (Of course, the driver should still be 
> free to parse the DSN for itself.)

Nothing will terminate such discussions once and forever :)

Anyway, connect() *already does* allow attributes to be passed as
a hash ref! It's called the \%attr parameter.

Drivers could, and probably should, allow any attribute that can
be specified in the driver-dsn string to also be specified via \%attr.


I've no plans to change the parameters to DBI->connect. But...

The DBI->parse_dsn() method could be extended to also return a hash
parsed from the driver-dsn - assuming the driver-dsn uses "n=v;" format.

A DBI->compose_dsn() method could be added to compose a DBI DSN string
from a bunch of parameters.

If someone wants to send me a patch to implement those changes (with
docs and tests) then I'd be interested. Best to write the docs first
though so we can agree on the (devils in the) details.

Tim.