Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Insert_ID is not working when using mysqli #166

Closed
stccorp opened this issue Dec 5, 2015 · 21 comments
Closed

Insert_ID is not working when using mysqli #166

stccorp opened this issue Dec 5, 2015 · 21 comments
Labels
bug mysqli MySQL and compatible drivers (MariaDB, etc) (Tier 1)
Milestone

Comments

@stccorp
Copy link

stccorp commented Dec 5, 2015

I just upgraded to php 5.5.30 and I am using the latest adodb 520. When I use mysqli, my id is always 0. When I use mysql I get the expected id. Is it not supported in mysqli? Is it a bug? Thank you

$cn = ADONewConnection('mysqli');
$cn->SetFetchMode(ADODB_FETCH_ASSOC);
$sql = "CALL sp_test(?,?,?)";
$stmt = $cn->Prepare($sql);
$datamap = array($p1, $p2, $p3);
$rs = $cn->Execute($stmt,$datamap);
$id= $cn->Insert_ID(); //************* ERROR wih 0 when using mysqli

@mnewnham
Copy link
Contributor

mnewnham commented Dec 5, 2015

Is this issue reproducable In 5.19?

@stccorp
Copy link
Author

stccorp commented Dec 5, 2015

Yes , same thing.

Just in case, I am using
Server version: 5.5.16-log MySQL Community Server (GPL) on windows

@stccorp
Copy link
Author

stccorp commented Dec 5, 2015

I just noticed something with mysql (debug on)

(mysql): CALL sp_insert_taxid(224,'482',1,'23744','')
(mysql): SELECT LAST_INSERT_ID()

with mysqli
(mysqli): CALL sp_insert_taxid(224,'482',1,'23744','')

The LAST_INSERT_ID is just NOT EXECUTED using mysqli internally

@stccorp
Copy link
Author

stccorp commented Dec 5, 2015

I am also running on a bsd server, and the same thing happens

@mnewnham
Copy link
Contributor

mnewnham commented Dec 5, 2015

Just wanted to make sure this was not a regression error with the new release

@stccorp
Copy link
Author

stccorp commented Dec 5, 2015

Is the first time for me using mysqli. I never had any problems with the mysql extension. But now that is being deprecated, more people will move to mysqli

@stccorp
Copy link
Author

stccorp commented Dec 5, 2015

I use LAST_INSERT_ID ALL the time. If it is a bug , I am very surprise. I will think it is used a lot.

@mnewnham
Copy link
Contributor

mnewnham commented Dec 5, 2015

This is what I think is the problem:

There is a method in each driver called in _insertid(). The one in the mysql driver executes 'SELECT LAST_INSERT_ID', whilst the mysqli driver uses the driver function mysqli_insert_id(). It's possible that the second one does not work correctly with stored procedures, or needs further configuration. As the mysqli driver extends the mysql driver, you could easily test this by renaming the method in the mysqli driver (adodb/drivers/adodb-mysqli.inc.php) and seeing if that fixes the problem.

Looking at the code in the mysql driver, it looks like an attempt to use mysql_insert_id() there has been abandoned.

@mnewnham
Copy link
Contributor

mnewnham commented Dec 9, 2015

The issue is as I thought, replacing the call to mysqli_insert_id with SELECT_LAST_INSERT_ID resolves the problem. You can find a hotfix for this here.

There is a bigger problem, however, as the statement is not actually being prepared by the call. It is being handled in the ADOdb compatibility mode, i.e the call to prepare simply returns the SQL statement. This means that prepare is not returning a handle to the stored procedure, and unless you are writing it in a way to be compatible with databases that do handle stored procedures, you might as well write:

$sql  = "CALL sp_test(?,?,?)";
$datamap = array($p1, $p2, $p3);
$rs  = $cn->Execute($sql,$datamap);

So you are not actually receiving any of the benefits of SP preparation, (Nor were you in the original driver). I'm not sure what it would take to implement this correctly, @dregad , perhaps you know mysql stored procedures better than I?

[updated]
There's also some notes in mysql::_query() that suggests the statement isn't even being prepared there. Maybe this needs documenting

@stccorp
Copy link
Author

stccorp commented Dec 9, 2015

What is your suggestion? Is it better to keep using the mysql extension?

@dregad
Copy link
Member

dregad commented Dec 9, 2015

@mnewnham Just to clarify in terms of roadmap, I see you qualify this as "hotfix", does this mean you think it should be targeted at 5.20.2, or is it just a regular bugfix that goes in next release (5.21) ? I would say the latter, since the issue was already present in 5.19.

perhaps you know mysql stored procedures better than I?

Probably not, I hardly ever use them ;-)

@dregad dregad added bug mysqli MySQL and compatible drivers (MariaDB, etc) (Tier 1) labels Dec 9, 2015
@mnewnham
Copy link
Contributor

mnewnham commented Dec 9, 2015

@stccorp , no you should use the hotfix and migrate your applications
@dregad I had not applied much thought to the roadmap, but considering the important issue of users needing to migrate from mysql to mysqli drivers, I believe that any mysql/mysqli driver incompatibility issues should be handled as hotfixes (5.20.2 in this case)

@dregad
Copy link
Member

dregad commented Dec 16, 2015

Please see my note in bfd0e0a#commitcomment-14988787 - the commit introduces dead code.

@mnewnham
Copy link
Contributor

From the documentation that you linked:

mysql_insert_id() returns 0 following a CALL statement for a stored procedure that generates an AUTO_INCREMENT value because in this case mysql_insert_id() applies to CALL and not the statement within the procedure. Within the procedure, you can use LAST_INSERT_ID() at the SQL level to obtain the AUTO_INCREMENT value.

Suggestions

  1. we use mysql_insert_id, and leave it to the programmer how to ascertain the last insert id for stored procedures. I believe that this is completely acceptable because it is a fully documented feature of the driver,
  2. We continue to use LAST_INSERT_ID(). I can't quite work our from the documentation why we should not, and I've used it for years on the mysqlt driver with no problems,
  3. We mix the two, as you suggest.
  4. We add some kind of option "$mysqldriver->useLastInsertFunction so that it uses mysql_insert_id normally but LAST_INSERT_ID after calling an SP
  5. We add something in the Stored procedure execution to retrieve last insert id. I am convinced we do something similar with another driver but I can't find it at the moment.

@stccorp
Copy link
Author

stccorp commented Dec 16, 2015

Please dont go with option 1. I have been using adodb for more than 7 years. One of the things I like is the ability to switch databases with just the connection string. If you use option 1, it will mean that people have to go through the code to see if they are using a store procedure or not. For large applications, it is just not practical. Option 4 or 5 seems more practical and 0 impact to the programmer

@mnewnham
Copy link
Contributor

Can anyone shine a light on this statement in the documentation:

The reason for the differences between LAST_INSERT_ID() and mysql_insert_id() is that LAST_INSERT_ID() is made easy to use in scripts while mysql_insert_id() tries to provide more exact information about what happens to the AUTO_INCREMENT column.

What 'exact information' can be provided that we would care about?

@dregad
Copy link
Member

dregad commented Dec 17, 2015

@stccorp

I agree that if we can provide a way to retrieve the last insert ID, then we should do it. It has to be clear however, that we may not be able to cover all cases depending on what the stored procedure does.

@mnewnham

What 'exact information' can be provided that we would care about?

From http://dev.mysql.com/doc/refman/5.7/en/mysql-insert-id.html :

The return value of mysql_insert_id() can be simplified to the following sequence:

  1. If there is an AUTO_INCREMENT column, and an automatically generated value was successfully inserted, return the first such value.
  2. If LAST_INSERT_ID(expr) occurred in the statement, return expr, even if there was an AUTO_INCREMENT column in the affected table.
  3. The return value varies depending on the statement used. When called after an INSERT statement:
  • If there is an AUTO_INCREMENT column in the table, and there were some explicit values for this column that were successfully inserted into the table, return the last of the explicit values.

    When called after an INSERT ... ON DUPLICATE KEY UPDATE statement:

  • If there is an AUTO_INCREMENT column in the table and there were some explicit successfully inserted values or some updated values, return the last of the inserted or updated values.

The key here is point 2:

  • As long as we let the DB assign the auto_increment field's value, there is no problem as both methods return the same result:

    php > $r = mysqli_query($db, 'insert into t (id) values (0)');
    php > print_r(mysqli_fetch_assoc( mysqli_query($db, 'select last_insert_id()')));
    Array
    (
    [last_insert_id()] => 142
    )
    php > var_dump(mysqli_insert_id($db));
    int(142)
    
  • On the other hand, if we assign a specific value, e.g. statement INSERT INTO table (id) VALUES (999), LAST_INSERT_ID() returns 0.

    php > $r = mysqli_query($db, 'insert into t (id) values (999)');
    php > print_r(mysqli_fetch_assoc( mysqli_query($db, 'select last_insert_id()')));
    Array
    (
    [last_insert_id()] => 142
    )
    php > var_dump(mysqli_insert_id($db));
    int(999)
    

This is the reason why I was suggesting to use a hybrid approach (first try mysqli_insert_id() and fallback to last_insert_id() if we get 0).

One more thing about the LAST_INSERT_ID() function (from http://dev.mysql.com/doc/refman/5.7/en/information-functions.html#function_last-insert-id)

The effect of a stored routine or trigger upon the value of LAST_INSERT_ID() that is seen by following statements depends on the kind of routine:

  • If a stored procedure executes statements that change the value of LAST_INSERT_ID(), the changed value is seen by statements that follow the procedure call.
  • For stored functions and triggers that change the value, the value is restored when the function or trigger ends, so following statements will not see a changed value.

[...]
The value of LAST_INSERT_ID() is not changed if you set the AUTO_INCREMENT column of a row to a non-“magic” value (that is, a value that is not NULL and not 0).

@dregad
Copy link
Member

dregad commented Dec 20, 2015

Note from @mnewnham over chat:

Is it true to say that whenever mysql_insert_id is updated then LAST_INSERT_ID is also updated, I'n trying to think of a loop with a non-procedure insert is followed by a procedure-based insert and at some point the procedure succeeds, then the non-procedure fails but the insert_id returns the insert id for the last successful procedure instead of false for the failed mysql_insert_id

No, see my note #166 (comment)
If user is assigning a value to the autoincrement field, then LAST_INSERT_ID is not updated

Regarding the loop scenario, I guess it's possible. In that case, maybe we can circumvent that by checking for an error prior to doing the fallback (didn't test)

@mnewnham
Copy link
Contributor

I've rethought this and I believe the following simplifies the procedure.

  1. We define a class variable $usePreparedStatement, default false.
  2. If the prepare method is used, this sets the value to true.
  3. If we use _query() in an appropriate manner, and $usePreparedStatement == true, set class variable $useLastInsertStatement = true.
  4. If we call insert_id(), use the internal function unless $useLastInsertStatement == true, then use LAST_INSERT_ID.
  5. reset $useLastInsertStatement = false
  6. Any other use of _query resets $usePreparedStatement and $useLastInsertStatement to false.

@dregad
Copy link
Member

dregad commented Jan 5, 2016

Sounds OK to me.

mnewnham added a commit that referenced this issue Feb 1, 2016
When using prepared statements, the last insert id was not
retrieved correctly. New feature optimally uses the suggested
mysql method for obtaining the value based on whether it was 
generated by a prepared statement or not.
dregad pushed a commit that referenced this issue Feb 4, 2016
When using prepared statements, the last insert id was not
retrieved correctly. New feature optimally uses the suggested
mysql method for obtaining the value based on whether it was
generated by a prepared statement or not.
@dregad dregad added this to the v5.21 milestone Feb 4, 2016
@dregad dregad closed this as completed Feb 4, 2016
@dargre
Copy link

dargre commented Dec 29, 2017

For mysqli and adodb this simple function should work:
$id = $db->_insertid();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mysqli MySQL and compatible drivers (MariaDB, etc) (Tier 1)
Projects
None yet
Development

No branches or pull requests

4 participants