Posted by chx on June 23, 2009 at 1:41am
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | chx |
| Status: | closed (fixed) |
| Issue tags: | PostgreSQL Code Sprint |
Issue Summary
http://www.mysqlperformanceblog.com/2008/01/10/php-vs-bigint-vs-float-co... http://www.lullabot.com/articles/drupocalypse-now-or-dangerous-integer-h...
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| bigint.patch | 932 bytes | Ignored: Check issue status. | None | None |
Comments
#1
Just verified that this does, in fact, fix the bigint problem for modules like Twitter that import larger-than-signed-int values and use drupal_write_record() to insert them. It also only incurs a speed hit *when bigints are actually being used*. Big thumbs up.
#2
Also, as chx noted, the 'heavier' case will never fire on 64-bit systems for actual ints. This is pretty much the best-case scenario for a fix unless we're willing to change the developer interface to require a new placeholder character.
#3
When did a similar fix get into Drupal 7 and what was the issue URL?
#4
It's called DBTNG. #225450: Database Layer: The Next Generation
#5
<?php$p = new PDO('mysql:host=localhost;dbname=drupal_7', 'root', '', array( PDO::ATTR_EMULATE_PREPARES => TRUE));
$p->prepare('INSERT INTO a VALUES (?)')
->execute(array(PHP_INT_MAX + 1));
?>
just ran this with mysql and sqlite on a 32 bit machine. works as expected. Edit: tried on SQLite without the emulate prepares option.
#6
postgresql has an issue here however that's a completely different issue and whether and how it can be fixed is a dubious matter and we do not even hold up D7 patches with postgresql results, why would we do it with Drupal 6 ?
#7
It's ugly, but it might be required unless we want to extend the API with a new placeholder.
#8
Having spent some quality time with it, I think it's worth the ugly. It's isolated, invisible to developers, and only triggers when the 'problem' scenario is encountered. If better options come down the line in the future, we can implement them.
#9
<?php
define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE);
db_query('CREATE TABLE test (id BIGINT)');
db_insert('test')->fields(array('id' => PHP_INT_MAX +1))->execute();
?>
This throws an exception when running on PostgreSQL:
PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer: "2147483648.000000"PHP is passing in TEXT, not a float.
#10
So it seems PHP/PDO is failing us here but on the bright side, there will be a work around. One option could be to retrive the datatype from the information schema, since we already do this for blobs and sequences, it shouldn't be much more work to just get all datatypes and pass the cast type to the ->bindParam() function. The Second option which will require more research could be using PostgreSQL auto casting which needs to be setup:
CREATE CAST(TEXT AS INT) WITHOUT FUNCTION AS ASSIGNMENT#11
Doesn't look like
CREATE CASTis going to work:drupal7=# CREATE FUNCTION drupal_to_number(drupal_text TEXT) RETURNS bigint AS $$
DECLARE new_int FLOAT;
BEGIN
SELECT to_number(drupal_text, '9999999999D000000') INTO new_int;
RETURN CAST(new_int AS bigint);
END;
$$ LANGUAGE plpgsql;
CREATE FUNCTION
drupal7=# select drupal_to_number('2147483648.000000');
drupal_to_number
------------------
2147483648
(1 row)
drupal7=# CREATE CAST(text AS bigint) WITH FUNCTION drupal_to_number(text) AS ASSIGNMENT;
CREATE CAST
drupal7=# insert into test values ('2147483648.000000');
ERROR: invalid input syntax for integer: "2147483648.000000"
So driver/schema casting looks like the only hope.
#12
See that's a D7 pgsql issue. The D6 issue is still RTBC.
#13
The reason I am reluctant to commit this is that it admittedly introduces a regression in Drupal 7. Chx says we should not care, but I am responsible to care, so seeing that it was not considered initially, I've asked to verify whether it applies as a bug to Drupal 7. It was proven to apply. All bugs in Drupal 6 first need to be fixed in Drupal 7, even if they result in different patches. I've asked whether a Drupal 7 fix will involve a patch against Drupal. If not, then we can obviously not delay committing this patch to Drupal 6. If the Drupal 7 problem is in PDO, and Drupal will not be patched against it, then Drupal will need to require a fresh PDO install, which might have this bug fixed, so that limits the platforms on where Drupal is supported. Therefore it was logical, that a Drupal 7 core patch will emerge. Thus I set out to follow the standard procedure and not commit this until the bug is fixed in Drupal 7, just like with any other issue. (Again, the fact that there is a different patch required for Drupal 7 applies to a lot of bugs, since there is lots changed in Drupal 7, so that does not make this issue a special case).
#14
Gabor, different patches is one thing but a different bug is another. The symptoms might be similar but they are not the same for in D6 the INSERT succeeds with wrong data because the our typecast is wrong and in D7 postgresql the INSERT fails. Yes, both are related to the 32 bit overflow to float misfeature of PHP but they are not the same bugs by any measure.
#15
marking this issue for postgreSQL code sprint
#16
It looks to me like this is sorted in Php 6.0. I trawled around in the code to find what had been changed, and have what I Think is the relevant diff attached to:
http://bugs.php.net/bug.php?id=48924
This seems to fix the issue cleanly. Its worth noting that Mysql users will experience the same sort of error as Postgres if STRICT_ALL_TABLES or similar is switched on (Actually they won't, I was looking at a the 64-bit case by mistake, sorry).
[further comments in http://drupal.org/node/515310 ]
#17
I'm with chx on the matter that the D6 patch should not be upheld because of a D7 bug,while they are the same problem, they are caused in different ways, I've created to #515310: Inserting >PHP_INT_MAX into BIGINT fails on PostgreSQL as pointed out by @markir. Lets resolve the D7 bug over there and get this D6 patch commited.
#18
That will not flight. 64-bit IEEE floating points have a maximum of 14 digits precision.
Gabor ran the following test:
@ini_set('precision', 16);print sprintf('%.0f', 9223372036854775807);
>> 9223372036854775808
Trying to represent 64 bit integers by 64 bit floats is a no-go.
The only way is to consider them as strings and whitelist characters.
#19
Frankly don't know what to mark duplicate, this one or #333788: Cannot insert/update unsigned integers or numbers bigger than an integer. Marked that one duplicate on the grounds of this one having a patch at least under discussion.
#20
While you are at it, please also add support for unsigned intergers (this would required a different placeholder %u). See the issue mentioned by Gábor in #19.
Proper handling of BIGINT in PHP may require BCMath/GMP extensions.
#21
We are good until 53 bits. Better than what we have now.
#22
I could be ready to accept that, but we need to document it, at the minimum.
#23
May we explore here the possibility to also add support unsigned ints? Is it just lack of time to write the patch?
Here's another comment about the patch: what happens with negative BIGINTs?
#24
Patch re-rolled to include support for unsigned integers. Also checks for negative BIGINTs.
#25
Re-rolled as I forgot to scan core for call to db_placeholders() where 'unsigned' was necessary to deal correctly with the corresponding DB columns.
#26
Thanks for adding negative bigint support. However, adding a new feature to Drupal 6 is not a possibility anymore, so the %u placeholder is not going to be added.
#27
Gábor: unsigned ints are supported by schema api, but you cannot use database api to insert unsigned ints, the same way bigints are supported by schema api, but you cannot use database api to insert bigints. Where's the difference?
Could we add %u to database api, but leave core modules that insert unsigned ints untouched? That way, there will be no side effect to this change. But contrib/custom code could take advantage of this fix, the same way contrib/custom code could take advantage of the ability to insert bigints?
#28
@markus_petrux: It is not the core modules. The reason we do not change APIs is so that contrib modules compatible with Drupal 6.x are compatible regardless of x. If %u is added in some x, then some contrib modules will only be compatible with x > 14 for example.
#29
@Gábor: Isn't this something to consider for BIGINT support too? Modules that need BIGINT support will also depend on D6.x and above.
It is actually a bug that Schema API allows BIGINTs and Unsigned Ints, but this is not supported by Database API.
#30
I consider this a bug, so I support adding %u.
#31
My patch just makes things work. Without adding %u.
#32
Huh. Just the bigint changes.
#33
The fact that Schema API supports BIGINTs and Unsigned ints, but this is not supported by Database API is a bug. Otherwise, this is a new feature. In any case, both issues have the same right to be fixed, unless there's a reason to fix just for positive BIGINT. Where is the problem in adding %u? Where's the problem for adding support for negative BIGINTs?
#34
Markus, those are valid points however as Drupal 6 has no version dependency, you would end up with modules that use a placeholder that are broken on an older core. That's not acceptable. My patch does something good within the confines of a minor release.
#35
%u could also be added to the Database API without any other impact in Drupal core and contrib/custom modules.
Only those that need this *Drupal bug fixed* will need to perform some kind of version checking, the same will happen for modules that really need BIGINT support. There's no difference here. If a module that needs BIGINTs does not perform version checking, it is simply broken.
Version checking is as easy as implementing hook_requirements('install') and refuse to install if VERSION constant is lower than (say) 6.14. If the module is already installed, then this is a bit more complex, but it is also possible, for example using a hook_update_N() that checks for Drupal VERSION constant and aborts the update step if lower than 6.14.
I'm attaching a patch that adds %u placeholder to Database API (but leaves core modules that use unsigneds untouched), and also adds a check for negative BIGINTs.
#36
This bears repeating. If a module actually needs to use the %u placeholder, and would thus become dependant on Drupal 6.14, there's no way for it to work properly under Drupal 6.13 right now. So the worst case scenario is that the module would stay broken until the admin upgraded core to 6.14.
#37
Exactly. Both issues share the same considerations.
Aside from hook_requirements() stuff, if you need BIGINTs or Unsigned Ints, you can do:
<?phpif (version_compare(VERSION, '6.14') < 0) {
// Bypass Database API because support for BIGINT and Unsigned Ints
// has not been fixed until Drupal (say) 6.14.
$precision = ini_get('precision');
@ini_set('precision', 16);
db_query('INSERT INTO {mytable} (mybigint, myunsigned) VALUES ('. sprintf('%.0f', $mybigint) .', '. sprintf('%u', $myunsigned) .')');
@ini_set('precision', $precision);
}
else {
db_query('INSERT INTO {mytable} (mybigint, myunsigned) VALUES (%d, %u)', $mybigint, $myunsigned);
}
?>
But that's not very nice.
If your application needs BIGINTs, or unsigned ints, then this is a possible way that does not require you to live with a patched Drupal core. Well, if your application really needs BIGINTs, or unsigned ints, then you need to do something about it.
On the other hand, as soon as these issues are fixed in Drupal core, you can simply do it right, and then use hook_requirements() stuff to ensure your module cannot be installed if Drupal core is lower then X.
Nobody else is affected by these issues unless it really needs BIGINTs, or unsigned ints.
#38
There are no polite words to describe the frustration I feel. First the issue is derailed by a different postgresql bug in Drupal 7, now it is derailed by unsigned integers. I won't it because this will never get into Drupal 6 and I do not want to see this in my issue tracker constantly frustrating myself.
#39
:(
If we need to, we can write SQL as in #37, but if this is not fixed in core, then that means we cannot write a CCK field that needs BIGINT and/or Unsigned Integers because CCK writes the SQL statements for us, so we cannot tell CCK to use sprintf's. :(
#40
In #26 above, I've explained that it was good to get Markus on the issue, since he pointed out we'd miss fixing negative bigint support. Looks like thanks to his review, chx's patch got better. Except, that I've also pointed out that adding %u would be a new feature IMHO in Drupal 6 (which chx seems to agree, at least in that it is derailing this issue). Not sure why chx inferred we should not fix any of the bugs anymore then. I never disputed we have a bug, I've asked for more reviews and we got valuable feedback and contribution thanks to that. I'd still love to get the bigint issue fixed, but did not see a patch to that effect incorporating Markus' feedback, so at best this is a needs work.
#41
Once more: my patch does fix what's need to be fixed. It never changed a bit nor it will. The sole reason for BIGINTs are to integrate with 3rd party systems with 64 bit identifiers. Currently, the prime example is twitter.
Negative BIGINTs are again a completely different bug that again needs proper research and noone came up with a use case for it so far... let's fix what we can and what we clearly need to fix.
#42
Note that slapping || $value < -(PHP_INT_MAX on this is just that: slapping. I am fairly sure a -1 or a +1 is required, one needs to check where exactly the overflow happens, whether the patch works with negative overflow etc.
#43
Well, someone marked the other issue as a duplicate of this one, so I assumed both things were going to be addressed here. @chx: I was not trying to "One of the worst things you can do is elaborate on other, equivalent approaches and suggest complex extensions to the patch". If the other issue is marked a dup of this, then this one needed, at least, to discuss about unsigned ints.
I do not have any personal interest in fixing this, I just tried to be helpful. IMHO, adding %u will only affect those that need this fixed. Not adding %u means those that need this fixed cannot do certain things in D6, just because DB API forgot to add support for something that's supported by Schema API.
If D6 is patched to accept positive BIGINTs, then those that need this will depend on the particular version of Drupal core this is fixed. If those that need this do nothing about previous versions, then they are buggy for those versions. And these are the same considerations for fixing Unsigned Ints. Another different thing is that it is decided not to fix unsigneds, which is fine, but it would also be fine if a good explanation on why is given. Well, this is just my opinion.
#44
Ok, in the interest of fixing the positive big integers problem at least, I've committed this patch from #0. Thanks.
@Markus: looks like the rest of the issues are indeed better discussed in #333788: Cannot insert/update unsigned integers or numbers bigger than an integer, where more people are already on the unsigned problem.
#45
Nope. I think that issue is ok as it is. You have crearly stated that unsigned int issue is not going to be fixed. Congrats. You have patched Drupal core just to fix a contrib module issue, while other modules that need BIGINT will have to check for Drupal core version to provide this feature. And other modules that need unsigned int fixed don't have a change to run in Drupal 6. :)
#46
Automatically closed -- issue fixed for 2 weeks with no activity.
#47
BCMath / GMP aren't required if you use a pure-PHP BigInteger implementation. Like this one:
http://phpseclib.sourceforge.net/
#48
Apologies for commenting on a closed issue.
I'm trying to solve a third-party module issue involving big ints on D6. #1362222: Users with 64-bit IDs not being registered.
Someone suggested using %s instead of %d. I don't particularly like the sound of that. Earlier in this thread, someone wrote about using %u, but I can't find that documented anywhere.
Can anyone recommend the right way to fix the problem in D6? Thanks!
[Update... I think I see in _db_query_callback() that %d should work for ints and big ints. I now think the issue reported against my module would be fixed by upgrading drupal core. Still I'd appreciate someone in the know confirming that.]