DBD::mysql Re: Why is selectrow_hashref complaining about a fetch without execute?
On Thu, Jul 16, 2015 at 10:46:35AM -0700, David E. Wheeler wrote:
> On Jul 16, 2015, at 6:40 AM, Tim Bunce wrote:
>
> > Well, this contains lots more light! ...
> >
> >>> -> dbd_st_execute for 03fdf4e0
> parse_params statement
> >>>SELECT c.change_id ...
> >
> >>> Binding parameters: SELECT c.change_id
> >
> >>>--> do_error
> >>> Out of sort memory, consider increasing server sort buffer size error
> >>> 1038 recorded: Out of sort memory, consider increasing server sort buffer
> >>> size
> >>><-- do_error
> >>> <- dbd_st_execute returning imp_sth->row_num 18446744073709551615
> >>>!! ERROR: 1038 'Out of sort memory, consider increasing server sort
> >>> buffer size' (err#0)
> >>><- execute= ( -1 ) [1 items] at /usr/lib/perl5/DBI.pm line 1632 via
> >>> at /usr/local/share/perl/5.18.2/App/Sqitch/Role/DBIEngine.pm line 149
> >
> > So execute failed. Note the crazy row_num. Execute seems to have
> > returned -1, which is a true value.
> >
> >>>!! The ERROR '1038' was CLEARED by call to fetchrow_hashref method
> >>>-> fetchrow_hashref for DBD::mysql::st
> >>> (DBI::st=HASH(0x42cfcc0)~0x4231cf8) thr#2603010
> >
> > Then the higher-level code called fetchrow_hashref, which cleared the
> > error recorded by execute().
>
> FWIW, the database handle is created like this:
>
> my $dbh = DBI->connect($uri->dbi_dsn, scalar $self->username, $pass, {
> PrintError => 0,
> RaiseError => 0,
> AutoCommit => 1,
> mysql_enable_utf8=> 1,
> mysql_auto_reconnect => 0,
> mysql_use_result => 0, # Prevent "Commands out of sync" error.
> HandleError => sub {
> my ($err, $dbh) = @_;
> $@ = $err;
> @_ = ($dbh->state || 'DEV' => $dbh->errstr);
> goto &hurl;
> },
>
> Context:
> https://github.com/theory/sqitch/blob/master/lib/App/Sqitch/Engine/mysql.pm#L59
>
> So I’m a little confused as to why the execute failure was ignored. Is this
> an issue with DBD::mysql?
Note the "row_num 18446744073709551615" above, that's -1 as an unsigned 64 bit
long.
DBD::mysql's handling of row_num seems less than ideal (prompted in part by
baggage of the DBI's ancient driver API).
int dbd_st_execute(SV* sth, imp_sth_t* imp_sth) <== XXX int (forced by DBI API)
{
...
imp_sth->row_num= mysql_st_internal_execute(...) <== row_num is declared as
my_ulonglong
...
if (imp_sth->row_num+1 != (my_ulonglong)-1) { ... } <== XXX
...
...
sprintf(actual_row_num, "%llu", imp_sth->row_num);
PerlIO_printf(DBIc_LOGPIO(imp_xxh), " <- dbd_st_execute returning
imp_sth->row_num %s\n", actual_row_num);
}
return (int)imp_sth->row_num; # <== XXX
}
my_ulonglong mysql_st_internal_execute(...) <== unsigned
{
my_ulonglong rows= 0; <== unsigned
if (!slen) {
do_error(h, JW_ERR_QUERY, "Missing table name" ,NULL);
return -2; <== signed
}
if (!(table= malloc(slen+1))) {
do_error(h, JW_ERR_MEM, "Out of memory" ,NULL);
return -2; <== signed
}
if (!(*result)) {
do_error(h, mysql_errno(svsock),
mysql_error(svsock),mysql_sqlstate(svsock));
return -2; <== signed
}
if(rows == -2) { <== signed
do_error(h, mysql_errno(svsock), mysql_error(svsock),
mysql_sqlstate(svsock));
if (DBIc_TRACE_LEVEL(imp_xxh) >= 2)
PerlIO_printf(DBIc_LOGPIO(imp_xxh), "IGNORING ERROR errno %d\n", errno);
rows = -2; <== signed
}
return(rows);
}
mysql_st_internal_execute41(...) has very similar issues
Looks to me like you've hit some latent bugs in the DBD::mysql code (e.g., it's
not safe/reliable to throw negative numbers around in unsigned types) compounded
by the limitations of the ancient DBI driver API:
https://github.com/perl5-dbi/dbi/blob/1486773ec0bf357661d756cf37ff2988b5eaf24d/Driver.xst#L585-L601
Seems like there's a need to separate "row count" from "execute return value".
Internally the DBI has a DBIc_ROW_COUNT(sth) macro that has an IV type.
That's a signed int that would be 64 bits on most modern systems.
On many of those systems the plain int type might be 32 bits.
I've just pushed an experimental change that might help in general
https://github.com/perl5-dbi/dbi/commit/29f6b9b76e9c637be31cb80f1a262ff68b42ef43#diff-cb6af96fe009d6f8d9d682415e1ab755
but probably wouldn't in your case.
At the moment I'd view this as a DBD::mysql bug.
Tim.
p.s. These open DBD::mysql issues might also be more or less related:
https://rt.cpan.org/Public/Bug/Display.html?id=48158
https://rt.cpan.org/Public/Bug/Display.html?id=80394
https://rt.cpan.org/Public/Bug/Display.html?id=75570
Re: DBD::mysql Re: Why is selectrow_hashref complaining about a fetch without execute?
On Jul 19, 2015, at 7:41 AM, Tim Bunce wrote: > Internally the DBI has a DBIc_ROW_COUNT(sth) macro that has an IV type. > That's a signed int that would be 64 bits on most modern systems. > On many of those systems the plain int type might be 32 bits. > > I've just pushed an experimental change that might help in general > https://github.com/perl5-dbi/dbi/commit/29f6b9b76e9c637be31cb80f1a262ff68b42ef43#diff-cb6af96fe009d6f8d9d682415e1ab755 > but probably wouldn't in your case. Huh. Why not? > At the moment I'd view this as a DBD::mysql bug. > > Tim. > > p.s. These open DBD::mysql issues might also be more or less related: > https://rt.cpan.org/Public/Bug/Display.html?id=48158 > https://rt.cpan.org/Public/Bug/Display.html?id=80394 > https://rt.cpan.org/Public/Bug/Display.html?id=75570 Given that these have had exactly 0 activity in three years, how should we go about getting thins on the maintaners’ radar? Also, is there something I can do in Sqitch to work around this issue? Thanks, David
Re: DBD::mysql Re: Why is selectrow_hashref complaining about a fetch without execute?
On 19/07/15 15:41, Tim Bunce wrote:
On Thu, Jul 16, 2015 at 10:46:35AM -0700, David E. Wheeler wrote:
On Jul 16, 2015, at 6:40 AM, Tim Bunce wrote:
Well, this contains lots more light! ...
-> dbd_st_execute for 03fdf4e0
parse_params statement
SELECT c.change_id ...
Binding parameters: SELECT c.change_id
--> do_error
Out of sort memory, consider increasing server sort buffer size error 1038
recorded: Out of sort memory, consider increasing server sort buffer size
<-- do_error
<- dbd_st_execute returning imp_sth->row_num 18446744073709551615
!! ERROR: 1038 'Out of sort memory, consider increasing server sort buffer
size' (err#0)
<- execute= ( -1 ) [1 items] at /usr/lib/perl5/DBI.pm line 1632 via at
/usr/local/share/perl/5.18.2/App/Sqitch/Role/DBIEngine.pm line 149
So execute failed. Note the crazy row_num. Execute seems to have
returned -1, which is a true value.
!! The ERROR '1038' was CLEARED by call to fetchrow_hashref method
-> fetchrow_hashref for DBD::mysql::st (DBI::st=HASH(0x42cfcc0)~0x4231cf8)
thr#2603010
Then the higher-level code called fetchrow_hashref, which cleared the
error recorded by execute().
FWIW, the database handle is created like this:
my $dbh = DBI->connect($uri->dbi_dsn, scalar $self->username, $pass, {
PrintError => 0,
RaiseError => 0,
AutoCommit => 1,
mysql_enable_utf8=> 1,
mysql_auto_reconnect => 0,
mysql_use_result => 0, # Prevent "Commands out of sync" error.
HandleError => sub {
my ($err, $dbh) = @_;
$@ = $err;
@_ = ($dbh->state || 'DEV' => $dbh->errstr);
goto &hurl;
},
Context:
https://github.com/theory/sqitch/blob/master/lib/App/Sqitch/Engine/mysql.pm#L59
So I’m a little confused as to why the execute failure was ignored. Is this an
issue with DBD::mysql?
Note the "row_num 18446744073709551615" above, that's -1 as an unsigned 64 bit
long.
DBD::mysql's handling of row_num seems less than ideal (prompted in part by
baggage of the DBI's ancient driver API).
int dbd_st_execute(SV* sth, imp_sth_t* imp_sth) <== XXX int (forced by DBI API)
{
...
imp_sth->row_num= mysql_st_internal_execute(...) <== row_num is declared as
my_ulonglong
...
if (imp_sth->row_num+1 != (my_ulonglong)-1) { ... } <== XXX
...
...
sprintf(actual_row_num, "%llu", imp_sth->row_num);
PerlIO_printf(DBIc_LOGPIO(imp_xxh), " <- dbd_st_execute returning
imp_sth->row_num %s\n", actual_row_num);
}
return (int)imp_sth->row_num; # <== XXX
}
my_ulonglong mysql_st_internal_execute(...) <== unsigned
{
my_ulonglong rows= 0; <== unsigned
if (!slen) {
do_error(h, JW_ERR_QUERY, "Missing table name" ,NULL);
return -2; <== signed
}
if (!(table= malloc(slen+1))) {
do_error(h, JW_ERR_MEM, "Out of memory" ,NULL);
return -2; <== signed
}
if (!(*result)) {
do_error(h, mysql_errno(svsock),
mysql_error(svsock),mysql_sqlstate(svsock));
return -2; <== signed
}
if(rows == -2) { <== signed
do_error(h, mysql_errno(svsock), mysql_error(svsock),
mysql_sqlstate(svsock));
if (DBIc_TRACE_LEVEL(imp_xxh) >= 2)
PerlIO_printf(DBIc_LOGPIO(imp_xxh), "IGNORING ERROR errno %d\n", errno);
rows = -2; <== signed
}
return(rows);
}
mysql_st_internal_execute41(...) has very similar issues
Looks to me like you've hit some latent bugs in the DBD::mysql code (e.g., it's
not safe/reliable to throw negative numbers around in unsigned types) compounded
by the limitations of the ancient DBI driver API:
https://github.com/perl5-dbi/dbi/blob/1486773ec0bf357661d756cf37ff2988b5eaf24d/Driver.xst#L585-L601
Seems like there's a need to separate "row count" from "execute return value".
Internally the DBI has a DBIc_ROW_COUNT(sth) macro that has an IV type.
That's a signed int that would be 64 bits on most modern systems.
On many of those systems the plain int type might be 32 bits.
I've just pushed an experimental change that might help in general
https://github.com/perl5-dbi/dbi/commit/29f6b9b76e9c637be31cb80f1a262ff68b42ef43#diff-cb6af96fe009d6f8d9d682415e1ab755
but probably wouldn't in your case.
At the moment I'd view this as a DBD::mysql bug.
Tim.
p.s. These open DBD::mysql issues might also be more or less related:
https://rt.cpan.org/Public/Bug/Display.html?id=48158
https://rt.cpan.org/Public/Bug/Display.html?id=80394
https://rt.cpan.org/Public/Bug/Display.html?id=75570
Please also see the issue I reported in DBI back in 2012:
https://rt.cpan.org/Ticket/Display.html?id=81911
I had to add various workarounds and a warning to DBD::ODBC.
Martin
Re: DBD::mysql Re: Why is selectrow_hashref complaining about a fetch without execute?
On Sun, Jul 19, 2015 at 06:39:59PM -0700, David E. Wheeler wrote: > On Jul 19, 2015, at 7:41 AM, Tim Bunce wrote: > > > Internally the DBI has a DBIc_ROW_COUNT(sth) macro that has an IV type. > > That's a signed int that would be 64 bits on most modern systems. > > On many of those systems the plain int type might be 32 bits. > > > > I've just pushed an experimental change that might help in general > > https://github.com/perl5-dbi/dbi/commit/29f6b9b76e9c637be31cb80f1a262ff68b42ef43#diff-cb6af96fe009d6f8d9d682415e1ab755 > > but probably wouldn't in your case. > > Huh. Why not? That change just adds a warning. > > At the moment I'd view this as a DBD::mysql bug. The assignment of signed values to unsigned types in DBD::mysql ought to be fixed. > > p.s. These open DBD::mysql issues might also be more or less related: > > https://rt.cpan.org/Public/Bug/Display.html?id=48158 > > https://rt.cpan.org/Public/Bug/Display.html?id=80394 > > https://rt.cpan.org/Public/Bug/Display.html?id=75570 > > Given that these have had exactly 0 activity in three years, how should we go > about getting thins on the maintaners’ radar? I don't know. The repo is managed by the perl5-dbi org on github. https://github.com/perl5-dbi/DBD-mysql so in theory any of the team members https://github.com/orgs/perl5-dbi/people could commit patches. Patrick Galbraith, the primary maintainer (CC'd) seems fairly active at the moment so I'd start by asking Patrick for his thoughts. > Also, is there something I can do in Sqitch to work around this issue? I'm not sure. It's possible that a HandleSetError handler could help https://metacpan.org/pod/DBI#HandleSetErr (It's also possible that I'm misreading the cause as I'm not clear how the -2 becomes a -1 but I gave up looking further after seeing the problems with the current DBD::mysql code.) Tim.
Re: DBD::mysql Re: Why is selectrow_hashref complaining about a fetch without execute?
On Mon, Jul 20, 2015 at 08:55:40AM +0100, Martin J. Evans wrote: > On 19/07/15 15:41, Tim Bunce wrote: > > Please also see the issue I reported in DBI back in 2012: > > https://rt.cpan.org/Ticket/Display.html?id=81911 > > I had to add various workarounds and a warning to DBD::ODBC. Ah, thanks for the reminder Martin! I'll add a comment on that case. Any thoughts about the general principle of changing the XS execute to return the value of the DBIc_ROW_COUNT IV if the int returned by dbd_st_execute is > 0 and DBIc_ROW_COUNT > 0? Tim.
Re: DBD::mysql Re: Why is selectrow_hashref complaining about a fetch without execute?
On 20/07/15 14:15, Tim Bunce wrote: On Mon, Jul 20, 2015 at 08:55:40AM +0100, Martin J. Evans wrote: On 19/07/15 15:41, Tim Bunce wrote: Please also see the issue I reported in DBI back in 2012: https://rt.cpan.org/Ticket/Display.html?id=81911 I had to add various workarounds and a warning to DBD::ODBC. Ah, thanks for the reminder Martin! I'll add a comment on that case. Any thoughts about the general principle of changing the XS execute to return the value of the DBIc_ROW_COUNT IV if the int returned by dbd_st_execute is > 0 and DBIc_ROW_COUNT > 0? Tim. 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. 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? 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. Is there some docs on that or perhaps you could just tell me or point me at a driver that does it correctly. 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 Also, maybe a little picky but the comment "and DBIc_ROW_COUNT>0" does not match the code. 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. Martin
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.
Re: DBD::mysql Re: Why is selectrow_hashref complaining about a fetch without execute?
On Mon, Jul 20, 2015 at 06:00:53PM +0100, Tim Bunce wrote: > On Mon, Jul 20, 2015 at 02:54:53PM +0100, Martin J. Evans wrote: > > 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)); Ah. The same logic also means I could change the return type of dbd_st_execute and dbd_db_do4, which also would help. Tim.
Re: DBD::mysql Re: Why is selectrow_hashref complaining about a fetch without execute?
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 iss
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_r
Re: DBD::mysql Re: Why is selectrow_hashref complaining about a fetch without execute?
On 21/07/15 15:03, Tim Bunce wrote:
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.
ok, so I'll try removing dbd_st_rows and whenever I call SQLRowCount I'll use
the DBIc_ROW_COUNT macro.
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.)
I will add assertion.
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?
Yes, I believe so.
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 imp
