On Tue, Jul 21, 2015 at 01:33:34PM +0100, Martin J. Evans wrote:
> Long, sorry.
No problem. The whole topic is a bit of a mess.
> 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.
That's reasonable. Hopefully we can do better though.
> 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.
The DBI provides a default rows method, in C, that returns DBIc_ROW_COUNT.
So a driver that stores the row count in DBIc_ROW_COUNT doesn't need to
provide a rows method at all (if all it needs to do is return the count).
That translates into not defining the dbd_st_rows macro. If that's not
defined then the rows method in Driver.xst won't get compiled in so
there'll be no call to a driver-provided dbd_st_rows.
> 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.
If DBD::ODBC switched to using DBIc_ROW_COUNT then you could remove
dbd_st_rows/odbc_rows. (It seems unlikely that sizeof(IV) would ever me
less than sizeof(SQLLEN) but that might be worth an assertion anyway.)
> 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?
Assuming the driver is using that default do() method, then it'll
end up in dbd_st_rows if the driver has defined a dbd_st_rows macro,
else it'll end up in the DBI's default rows() method.
> >>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.
Is the only outstanding issue now the 'int' return type on some various
dbd_st_* functions?
> 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.
And can simply be removed, per the above.
> 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.
DBIc_ROW_COUNT is just a macro for an IV in the imp_sth structure. Most,
if not all, compiled drivers that aren't using DBIc_ROW_COUNT are simply
using their own integer element in the imp_sth structure. In the case of
DBD::Pg that's declared as a plain int type.
So I'd hope and expect a driver can simply use DBIc_ROW_COUNT _instead of_
whatever it's currently using.
> >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
> }
[Sigh] I'm getting a bit rusty at C. I'd forgotten that hurdle.
The int return type affects dbd_st_execute, dbd_st_rows, and dbd_db_do4.
The fix is to do what we've done before when the signature of
dbd_db_login and dbd_db_do changed. E.g., first we added a dbd_db_login6
macro and then a dbd_db_login6_sv macro. Drivers just define the one
they want to use.
So I propose adding #ifdef's for dbd_st_execute_iv, dbd_st_rows_iv, and
dbd_db_do4_iv. Driver can then define those if they want to use them
(and add a prereq on the appropriate version of DBI.)
Sound ok?
Tim.
