Long, sorry.
On 20/07/15 18:00, Tim Bunce wrote:
On Mon, Jul 20, 2015 at 02:54:53PM +0100, Martin J. Evans wrote:
On 20/07/15 14:15, Tim Bunce wrote:
I think that would work for me - I'm happy to test it our here if you want to
give it a go.
IIRC, when this was last discussed the problem is that some drivers
might not set DBIc_ROW_COUNT so you can't just use DBIc_ROW_COUNT.
Hence the check that DBIc_ROW_COUNT is not zero. Since the DBI code sets
it to zero before the call, if it's non-zero after the call we can be
sure that the driver has set it.
In fact, I just checked, and DBD::ODBC does not seem to call
DBIc_ROW_COUNT other than to set it to 0 in ODBC.xsi (which is code
from DBI anyway). Does that sound right?
Nope. Is it setting the underlying structure member directly?
no. All it does is it has a RowCount member in its own imp_sth_st structure
which is a SQLLEN (64 bits on 64 bit machines and 32 on 32). Then it:
o dbd_db_execute returns the number of rows or -1 or -2 (error)
At the end of dbd_st_execute if the affected rows is bigger than INT_MAX and
warnings are
on, it warns the rowcount has been truncated and changes the row count to
INT_MAX.
o has odbc_st_rows (because it is defined in dbd_xsh.h and I believed you
needed to implement most of these in the DBD) which casts the internal RowCount
to an int as odbc_st_rows is defined as returning an int.
DBD::ODBC also has its own odbc_rows which returns an IV to workaround this
issue in DBI when I found it back in 2012.
Note dbd_xsh.h defines dbd_st_rows and dbd_st_execute as returning ints.
Looking at 'do' in DBI.pm it just does:
sub do {
my($dbh, $statement, $attr, @params) = @_;
my $sth = $dbh->prepare($statement, $attr) or return undef;
$sth->execute(@params) or return undef;
my $rows = $sth->rows;
($rows == 0) ? "0E0" : $rows;
}
so doesn't that just end up in dbd_st_rows?
If a driver is supposed to set DBIc_ROW_COUNT I'd rather change the
drivers I maintain to do that especially since in ODBC and 64bit
SQLRowCount already returns a 64 bit value.
Yeap. That's best.
See above, I don't see how that fits in right now.
I tried to check my assumptions and this is what I found:
o DBD::ODBC has its own 'do' method because it can use SQLExecDirect instead of
prepare/execute. This returns the rows affected correctly as it returns an SV
created from the SQLLEN RowCount. So, the do method in DBI (shown above) is
neither here nor there for DBD::ODBC.
o DBD::ODBC has a dbd_st_rows which seems to get called if someone calls the
rows method.
dbd_st_rows is defined in dbd_xsh.h as returning an int so this is wrong.
o 'execute' or dbd_st_execute returns the rows and is again defined in dbd_xsh
as returning an int.
I don't see where DBIc_ROW_COUNT comes in unless you are saying every time a
DBD discovers the row count it should call DBIc_ROW_COUNT macro.
Is there some docs on that or perhaps you could just tell me or point
me at a driver that does it correctly.
No docs, sadly. And I'm not aware of any drivers that do.
I took a look at DBD:Pg and that uses it's own 'rows' structure
member which is defined as an int, and int is used in the code.
I also noticed something I should have seen before: dbd_st_rows() is
defined as returning an int. I _think_ it would be safe to change the
definition to returning an IV since it's only used internally by drivers
via the Driver.xst template file that does:
XST_mIV(0, dbd_st_rows(sth, imp_sth));
Unless I'm missing something I think that will break most drivers as when I
grepped cpan I found most drivers implement dbd_st_rows as:
int dbd_st_rows {
code
}
I'm having a frustrating day so far so perhaps have lost the ability to read
diffs and C but in your change at
https://github.com/perl5-dbi/dbi/commit/29f6b9b76e9c637be31cb80f1a262ff68b42ef43#diff-cb6af96fe009d6f8d9d682415e1ab755
"if retval>0 (checked above) " I don't see where the "checked above" bit is.
it looks like:
if (retval == 0)
..
else if (retval == -1)
..
else if (retval <= -2)
..
else
new stuff here
retval could still be negative just not -1 or -2
The "else if (retval <= -2)" covers other negative values, doesn't it?
my mistake, as I said, I was not having a good day.
Also, maybe a little picky but the comment "and DBIc_ROW_COUNT>0" does not
match the code.
Yeah, I was in two minds about that. I'll use DBIc_ROW_COUNT>0 in
practice, but !=0 seemed a better fit for the experimental warning.
If no DBDs use DBIc_ROW_COUNT then that warning you've put in will do
nothing. I'd like to see a driver which does use DBIc_ROW_COUNT and if
there are none I'm happy to change DBD::ODBC initially to a) test the
diff you just applied and b) test the suggested fix.
That would be great. Thank you Martin!
Tim.
I'll happily make any changes you suggest and can test any changes you want to
try out in DBI but I think there are still some issues to discuss above.
Martin
