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

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?

> 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.

> 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));

> 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?

> 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.