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 tim.bu...@pobox.com 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 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 Sun, Jul 19, 2015 at 06:39:59PM -0700, David E. Wheeler wrote: On Jul 19, 2015, at 7:41 AM, Tim Bunce tim.bu...@pobox.com 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 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 retval0 (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_COUNT0 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 retval0 (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_COUNT0 does not match the code. Yeah, I was in two minds about that. I'll use DBIc_ROW_COUNT0 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.
