Returns wrong BigInt insertIds

[candypink2]

DB Schema

CREATE TABLE `users` (
  `id` bigint unsigned NOT NULL AUTO_INCREMENT,
  `name` varchar(20) NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB AUTO_INCREMENT=12345678901234567945 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci

MYSQL Config

  {
    host: 'localhost',
    user: 'develop',
    password: 'develop',
    database: 'develop',
    waitForConnections: true,
    namedPlaceholders: true,
    connectionLimit: 1,
    bigNumberStrings: true,
    supportBigNumbers: true,
  };

QUERY

INSERT INTO users(name) VALUES('test')

Response

[
  ResultSetHeader {
    fieldCount: 0,
    affectedRows: 1,
    insertId: '-6101065172474983670',
    info: '',
    serverStatus: 2,
    warningStatus: 0
  },
  undefined
]

Version: ^2.3.0

PS: Works as expected with https://github.com/mysqljs/mysql

[sidorares]

I think we have a bug in the https://github.com/sidorares/node-mysql2/blame/3c300a874d8d739338b7ce4954a39cef5a392541/lib/packets/packet.js#L200

in my test raw buffer for the number is 254, 11, 11, 31, 235, 140, 169, 84, 171

readLengthCodedNumberExt(tag, bigNumberStrings, signed) is called with 254, true, true then reads 2 int32: 3944680203, 2874452364

and (new Long(3944680203, 2874452364, false)).toString() gives incorrect "-6101065172474983669"

if I call it as (new Long(3944680203, 2874452364, true)).toString() the result is correct 12345678901234567947 - possible word0 and word1 in previous step are calculated incorrectly and need to be read as signed

[sidorares]

I think the problem is that we always assume insertId signed but in the schema its defined as signed `id` bigint unsigned NOT NULL AUTO_INCREMENT, and serialised as such

related #336

[sidorares]

hm, I don't know if there is a good solution here. We don't get flag about sign of insertId as part of ok packet and depending on how the table is defined it can be both signed and unsigned, and that affects how we must read binary "length coded integer" ( 64 bit int in your example )

Am I missing something? Might need your help @ruiquelhas . If this is an omission in the protocol - what is other clients behaviour?

[sidorares]

Note that while you can't have ENGINE=InnoDB AUTO_INCREMENT=[negative number] it's possible to have negative insertId by inserting INSERT INTO bigint_table(id, name) VALUES(-12345678934567945, 'test') if id has signed type.

[testn]

This is what PHP document said

Caution mysql_insert_id() will convert the return type of the native MySQL C API function mysql_insert_id() to a type of long (named int in PHP). If your AUTO_INCREMENT column has a column type of BIGINT (64 bits) the conversion may result in an incorrect value. Instead, use the internal MySQL SQL function LAST_INSERT_ID() in an SQL query. For more information about PHP's maximum integer values, please see the integer documentation.

[testn]

This was a MySQL bug 10 years ago https://bugs.mysql.com/bug.php?id=20964

[sidorares]

thanks @testn , TIL

maybe we could add some error checking to have better early warning about undefined behaviour and point to a solution ( LAST_INSERT_ID() etc )

[testn]

Sound like based on this documentation, last_insert_id should be treated as unsigned by defaut?

[sidorares]

yeah, thats probably "least surprises" default.

in that case INSERT INTO bigint_table(id, name) VALUES(-12345678934567945, 'test') would return incorrect result but I'm afraid it's not possible to cover all scenarios here

[ruiquelhas]

@sidorares the fact that a number is signed or not should not be a factor here. AUTO_INCREMENT in this case will start counting from the value defined by AUTO_INCREMENT= up to the upper limit of the data type.

BIGINT SIGNED	BIGINT UNSIGNED
9223372036854775807	18446744073709551615

If you try to set a negative number as the starting point with AUTO_INCREMENT= it will yield an error. If you reach the upper limit, it also yields an error (duplicate key because since it can't further increment the value, it tries to use the highest one it can - the last one).

If, on the other hand, you INSERT a negative value, it does not matter because the LAST_INSERT_ID() only relates to whatever is tracked by AUTO_INCREMENT. You should not assume it contains the actual value you inserted.

So, in the end, you should treat the value of LAST_INSERT_ID() or whatever comes bundled in the OK_Packet as an unsigned number.

[sidorares]

The problem here is that we don't know if integer we receive is signed or not ( insertId is "length coded int" and ok packet does not give us sign hints ). If you INSERT a negative value, not only that operation insertId is incorrect but all subsequent ( until its positive again )

I guess the advise here is 1) never insert negative AUTO_INCREMENT field ( can we detect this at the protocol level? can the server be configured to help detecting that and treat as an error? ) and 2) don't trust insertId and always use LAST_INSERT_ID() instead ( from a second query or stored procedure )

[ruiquelhas]

I was mislead by my own tests. I assumed that the AUTO_INCREMENT value was not influenced by values we explicitly INSERT, but that's only the case for negative values, where although the value in the column is negative, AUTO_INCREMENT keeps the previous (and the highest positive) one.

In any case, that value is never negative, and thus, you should always treat it as UNSIGNED (even though the column data type might be SIGNED).

The advice, from what I can tell is, don't trust neither LAST_INSERT_ID() nor insertId whenever you start sourcing your own custom values (in particular, negative ones). The point in both is to simply keep track of valid AUTO_INCREMENT values. Valid in this case only means positive, because although we can INSERT negative values, explicitly changing AUTO_INCREMENT to a negative value yields an error:

ALTER TABLE <table> AUTO_INCREMENT=-10
-- ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-10' at line 1

I'll ask around to see if this has been a concern for other clients, but from what I can tell, this is something we simply have to accept as a fact. A possible non-breaking solution would be, for instance, having the INSERT statement generate a warning whenever you fed negative values for AUTO_INCREMENT columns, but it would only help to set the same expectation for the values returned by LAST_INSERT_ID() and insertId that we now have established, which is that we should not trust those values for some cases.

[sidorares]

hm, interesting I haven't tried inserting anything after inserting negative id, here is what happens for me:

first insert returns insertId equal to what was used in the insert ( '-100' ) second insert fails with ER_AUTOINC_READ_FAILED: Failed to read auto-increment value from storage engine if id is not set LAST_INSERT_ID() returns 0

In my view INSERT INTO bigint_table(id, name) VALUES(-12345678934567945, 'test') should result in an error to prevent all above scenarios with something like ER_AUTOINC_WRITE_FAILED

[ruiquelhas]

Are you setting an initial AUTO_INCREMENT value when creating the table? Is it a BIGINT UNSIGNED column like the OP example?

That error should not be related to the fact that you insert a negative number. I think that only happens when the AUTO_INCREMENT value is already out of range for the given column data type.

For instance, using OP's example but instead making the id column a BIGINT SIGNED (mind that by omission, numeric columns are SIGNED), it will fail with that same error because the initial AUTO_INCREMENT value is already out-of-range. So, if you rely on it whilst inserting anything and not providing a specific id value, it will yield that error.

CREATE TABLE test (id BIGINT AUTO_INCREMENT NOT NULL, name VARCHAR(25), PRIMARY KEY(id)) AUTO_INCREMENT=12345678901234567945;
INSERT INTO test (name) VALUES ('foo');
-- ERROR 1467 (HY000) at line 2: Failed to read auto-increment value from storage engine

The problem with your example is that, if the column data type is SIGNED, negative numbers are valid, so there's no reason to not allow that.

Of course if the column data type is UNSIGNED then you already get an error if you try to INSERT negative values.

CREATE TABLE test (id BIGINT UNSIGNED AUTO_INCREMENT NOT NULL, name VARCHAR(25), PRIMARY KEY(id)) INSERT INTO test (id, name) VALUES (-10, 'foo');
-- ERROR 1264 (22003) at line 2: Out of range value for column 'id' at row 1

The only problem here is that insertId creates false expectations about what it returns. To make matters worse, LAST_INSERT_ID() also does not help. Both those values are only reliable if you don't source your own values while INSERTing.

I've talked with a few people (working with client implementations) about it and some of them were not even aware of this kind of behaviour, which means it probably hasn't been an issue. Maybe that's because the behaviour is documented somewhere (couldn't find anything).

So, the idea that I got was that we probably need to live with it. And to be honest, I don't see any kind of limitation here as well. When your INSERT relies on values "generated" by AUTO_INCREMENT, the insertId is fine. When you provide your own values, you already know them, so there's no point in looking for whatever is returned by insertId. It might be a bit more tricky for bulk inserts, but the reasoning still stands.