Re: DBD::mysql Re: Why is selectrow_hashref complaining about a fetch without execute?

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.