BIGINT handling

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.

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.

When did a similar fix get into Drupal 7 and what was the issue URL?

It's ugly, but it might be required unless we want to extend the API with a new placeholder.

It's ugly, but it might be required unless we want to extend the API with a new placeholder.

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.

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.

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

Doesn't look like CREATE CAST is 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.

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

marking this issue for postgreSQL code sprint

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 ]

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.

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.

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.

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.

I could be ready to accept that, but we need to document it, at the minimum.

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?

Patch re-rolled to include support for unsigned integers. Also checks for negative BIGINTs.

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.

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.

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?

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

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

I consider this a bug, so I support adding %u.

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?

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

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.

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.

Exactly. Both issues share the same considerations.

Aside from hook_requirements() stuff, if you need BIGINTs or Unsigned Ints, you can do:

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

:(

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

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.

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.

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.

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

BCMath / GMP aren't required if you use a pure-PHP BigInteger implementation. Like this one:

http://phpseclib.sourceforge.net/

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