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.
