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

Php 7.0.9 - Fix the get last id when no sequence is informed using lastval() #2014

Closed
wants to merge 26 commits into from

Conversation

6 participants
@phackwer
Copy link
Contributor

phackwer commented Jul 20, 2016

===My first contribution on C code. Sorry about the lot of builds to reach perfection.===

This PR fixes a very disturbing bug that happens when lastInsertId is used without the sequence name on PostgreSQL. When seq_name is informed, now it assumes you just want the very last inserted id. This makes the code more compatible with others databases, and also implements the use of postgre's lastval internal function. Before this fix, when no sequence name was informed, you always gotta a boolean false response. Very annoying, since not always the sequence names were know by the programmer.

For postgres versions prior to server_version_num 080100, the only option was to use currval informing the sequence name.

The problem occurred when an ORM lib had no idea of what was the sequence name. Currval always required the sequence name, and returned null when no sequence name was informed. This change, to use lastval when no sequence name was informed (or known), will fix a lot of ORMs problems around the world. All they need to do is stop trying to guess sequence names for Postgres.

phackwer added some commits Jul 19, 2016

[BUG] - When PostgreSQL version is higher than 8.1, fixes the message…
… "Object not in prerequisite state: 7 ERROR: currval of sequence sequence_name is not yet defined in this session"
Update bug_last_insert_id.phpt
[UPD] - I tried to use the warning messages on the EXPECTREGEX but got no lucky, so, the test now won't try to force the fail, since I couldn't figure out how to expect a warning on the test
Update pgsql_driver.c
Fix memory leak detected by QA
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jul 20, 2016

Please don't create a new PR for every commit -- you can simply push to your existing branch, the PR will be updated an Travis will run a new build.

@phackwer

This comment has been minimized.

Copy link
Contributor Author

phackwer commented Jul 20, 2016

Can someone help me?

Travis is accusing possible memory leak, but if I try to free *version or *res myself, I run into the following error on my machine:

*** Error in `/var/www/html/php-src/sapi/cli/php': double free or corruption (out): 0x00007f01ac402030 ***

So, if it's not about freeing version, or res, where is the "memory leak"? On the id return? If so, how it's possible that this code worked before? I haven't changed this part of the code. Apparently something is wrong, but I can't find the fix for it! Any thoughts?

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jul 20, 2016

@phackwer res does not need the free and version should use efree instead of free (as it is allocated with estrdup).

Though I'm not sure why you're copying version to a new string there at all?

@phackwer

This comment has been minimized.

Copy link
Contributor Author

phackwer commented Jul 20, 2016

@nikic , I copied to make a number comparison. server_version_num returns a number, but PQexec returns a char. So, I needed to compare if version equal or higher than 80100 (8.1.0). atoi was needed for this. I thought I was going to use it again, but I didn't, so I can move it to the if and remove int_version.

Thanks for the efree tip. I will try right now again.

And sorry about the PR. I worked on a place where we should remove PRs that had any problam on CI.

FIX memory leak - passed all local tests
This commit seems to fix any QA problem pointed out by Travis.

Fixes a very disturbing bug that happens when lastInsertId is used without the sequence name on PostgreSQL. When seq_name is informed, now it assumes you just want the very last inserted id. This makes the code more compatible with others databases, and also implements the use of postgre's lastval internal function. Before this fix, when no sequence name was informed, you always gotta a boolean false response. Very annoying, since not always the sequence names were know by the programmer.

For postgres versions prior to server_version_num 080100, the only option was to use currval informing the sequence name. Sorry.
@phackwer

This comment has been minimized.

Copy link
Contributor Author

phackwer commented Jul 20, 2016

@nikic - now it worked like a charm. I also removed int_version and used the atoi return instead. Local tests are ok, lastval is returning the last insert id for whatever was inserted, and if you need an specific sequence, than you just can inform (like it was before).
Thanks for all the help.

If it pass, should I aply it for all latest branches and ask for the PR, or is there any other process easier than this?

@phackwer

This comment has been minimized.

Copy link
Contributor Author

phackwer commented Jul 20, 2016

@nikic , really really thanks!!!! It passed!!! Now I must check how to go further with the PR for all the current dev branches.

@@ -15,6 +15,7 @@
| Authors: Edin Kadribasic <edink@emini.dk> |
| Ilia Alshanestsky <ilia@prohost.org> |
| Wez Furlong <wez@php.net> |
| Pablo Santiago Sánchez <phackwer@gmail.com> |

This comment has been minimized.

Copy link
@laruence

laruence Jul 21, 2016

Member

hmm, we usually won't edit copyright fields in this case...

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 21, 2016

Author Contributor

Why not? I've almost rewritten an entire method.

This comment has been minimized.

Copy link
@laruence

laruence Jul 21, 2016

Member

it's 1200+ lines file... anyway, I just show my concern here.

This comment has been minimized.

Copy link
@weltling

weltling Jul 21, 2016

Contributor

@phackwer authors mentioned there are usually either original authors or someone who rewrote the significant part of the file. We usually don't put names there just for one fix, but mention them in the NEWS file.

Thanks.

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 21, 2016

Author Contributor

Ok, I will remove it.

@@ -18,7 +18,7 @@ if (false !== getenv('PDO_PGSQL_TEST_DSN')) {
$config['ENV']['PDOTEST_ATTR'] = getenv('PDO_PGSQL_TEST_ATTR');
}
} else {
$config['ENV']['PDOTEST_DSN'] = 'pgsql:host=localhost port=5432 dbname=test user= password=';
$config['ENV']['PDOTEST_DSN'] = 'pgsql:host=localhost port=5432 dbname=test user=postgres password=postgres';

This comment has been minimized.

Copy link
@laruence

laruence Jul 21, 2016

Member

this should not be changed.

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 21, 2016

Author Contributor

Agreed, but I had to change because for some reason tests wasn't running because the PDOTEST complaint about no username and password informed. Why? I've actually didn't go further. I was worried more about the test not running than this line.

My mistake. I've committed accidentally.

I've probably did something wrong on my environment.

Really, I don't know when I committed it.

This comment has been minimized.

Copy link
@weltling

weltling Jul 21, 2016

Contributor

@phackwer you can use PDO_PGSQL_TEST_DSN to set the connection string, so you can revert this piece just withith the same PR.

Thanks.

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 21, 2016

Author Contributor

Ok, I will revert it.

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 22, 2016

Author Contributor

Reverted and passed tests. Everything ok now?

q[0] = name;

res = PQexec(H->server, "SHOW server_version_num");
version = estrdup((char *)PQgetvalue(res, 0, 0));

This comment has been minimized.

Copy link
@nikic

nikic Jul 21, 2016

Member

Looking at libpq it looks like there is a PQserverVersion function that directly returns the version as an integer.

However I'm not sure if there's some kind of problem with this function as it is not used here either: https://github.com/phackwer/php-src/blob/58ea58609ea292273ba8c8e91a52c8a04c82ee01/ext/pdo_pgsql/pgsql_driver.c#L416

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 22, 2016

Author Contributor

I will change and test it

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 22, 2016

Author Contributor

Changed.

Everything Ok now?

phackwer added some commits Jul 21, 2016

Using PQserverVersion - less queries, no atoi
Using PQserverVersion - less queries, no need for atoi
@jerrygrey

This comment has been minimized.

Copy link

jerrygrey commented Jul 22, 2016

To make it easy to read and understand, the test code:

 +--EXPECTREGEX--
+string\([0-9]*\)\ \"[0-9]*\"
+string\([0-9]*\)\ \"[0-9]*\"

can be just:

--EXPECTF--
string(%d) "%d"
string(%d) "%d"
@phackwer

This comment has been minimized.

Copy link
Contributor Author

phackwer commented Jul 22, 2016

It's a regular expression. Easy to read anyway. Won't change it.

--TEST--
currval() vs lastval()
PDO PgSQL Bug #1134 [BUG] New record, PostgreSQL and the Primary key https://github.com/phalcon/cphalcon/issues/1134
Fix PHP original PDO_PGSQL::lastInsertId https://github.com/doctrine/dbal/pull/2329

This comment has been minimized.

Copy link
@weltling

weltling Jul 25, 2016

Contributor

This section should contain just one line, usually. As when multiple tests run, everyone shows up as one line in the out. The other info can be added as a comment to the script in the --FILE-- section.

Thanks.

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 26, 2016

Author Contributor

I fixed it.

$config['ENV']['PDOTEST_USER'] = 'postgres';
$config['ENV']['PDOTEST_PASS'] = 'postgres';
$db = PDOTest::factory();

This comment has been minimized.

Copy link
@weltling

weltling Jul 25, 2016

Contributor

Please use $db = PDOTest::test_factory(dirname(__FILE__) . '/common.phpt'); so all the tests can be run on the same DB instance.

Thanks.

const char *q[1];
q[0] = name;

if (PHP_PDO_PGSQL_LASTVAL_PG_VERSION <= PQserverVersion && name == NULL) {

This comment has been minimized.

Copy link
@weltling

weltling Jul 25, 2016

Contributor

PQserverVersion is a function, you currently compare teh function pointer.

Thanks.

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 26, 2016

Author Contributor

Readi it too fast (too much to do at work). WIll fix it. 10 kill.

*len = PQgetlength(res, 0, 0);
} else {
pdo_pgsql_error(dbh, status, pdo_pgsql_sqlstate(res));
*len = spprintf(&id, 0, ZEND_LONG_FMT, (zend_long) H->pgoid);

This comment has been minimized.

Copy link
@weltling

weltling Jul 25, 2016

Contributor

Indent :)

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 26, 2016

Author Contributor

Oh, well, my editor was setup to space instead of tabs. Will fix it. Sorry.

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 26, 2016

Author Contributor

The identation is crrect according to the blocks coded in the function. You can check that now. ;-)

phackwer added some commits Jul 26, 2016

res = PQexec(H->server, "SHOW server_version_num");
version = estrdup((char *)PQgetvalue(res, 0, 0));

if (PHP_PDO_PGSQL_LASTVAL_PG_VERSION <= atoi(version) && name == NULL) {

This comment has been minimized.

Copy link
@weltling

weltling Jul 26, 2016

Contributor

Hmm, why not like PQserverVersion(H->servir) ?

And, seems too much with the space, I only meant that one line, but now the header and several other places have some brokend ws :)

Thanks

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 26, 2016

Author Contributor

Damn, this eclipse editor is breaking my balls! Each time it does something I didn't configured it to.

Will fix right now.

I selected the block I wanted it to search and replace, made sure that "Selected text" (or "Selection", can't remember exactly) was checked, and hit replace.

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 26, 2016

Author Contributor

@weltling
I added what you said and look the error now:

/home/travis/build/php/php-src/ext/pdo_pgsql/pgsql_driver.c:370:59: error: ‘pdo_pgsql_db_handle’ has no member named ‘servir’
make: *** [ext/pdo_pgsql/pgsql_driver.lo] Error 1

This comment has been minimized.

Copy link
@phackwer

phackwer Jul 26, 2016

Author Contributor

@weltling
It was server, not servir. Just fixed. Sorry, I couldn't find documentation about the handler. :-/ My IDE went crazy and documentation is pretty difficult to find.

This comment has been minimized.

Copy link
@weltling

weltling Jul 27, 2016

Contributor

Oh, my typo, yep. And PDO is OFC a lot of stuff. Usually just learning from the code. I were not aware of some API docs existing for PDO. But regarding IDE - that's why vim justifies itself :)

Thanks.

phackwer and others added some commits Jul 26, 2016

[UPD] - spaces, tabs, and everything nice.
Switched back to PQserverVersion(H->servir), fixed a mess eclipse did for me (or I did using it).
Pablo Santiago Sánchez
@phackwer

This comment has been minimized.

Copy link
Contributor Author

phackwer commented Jul 26, 2016

I figured out what was the real bug that some ORMs had. It was when they had no idea of what was the sequence name, or asked from one that didn't respect the SequenceNamingStrategy (Doctrine, for instance). Currval always required the sequence name. This change, to use lastval when no sequence name was informed (or known), will fix a lot of ORMs problems around the world. All they need to do is stop trying to guess sequence names for Postgres.

phackwer added some commits Jul 26, 2016

[FIX] - Ident!
Still had a small identation problem.
@weltling

This comment has been minimized.

Copy link
Contributor

weltling commented Jul 27, 2016

Merged with 12628e9.

Thanks.

@weltling weltling closed this Jul 27, 2016

@phackwer

This comment has been minimized.

Copy link
Contributor Author

phackwer commented Jul 27, 2016

@weltling : Should I do it in the 7.1.0 branch as well or it will be ported later?

@weltling

This comment has been minimized.

Copy link
Contributor

weltling commented Jul 27, 2016

@phackwer it's already merged into 7.0+ dev trees. See more about how it's handled in PHP here https://wiki.php.net/vcs/gitworkflow

Thanks.

@lcligny

This comment has been minimized.

Copy link

lcligny commented Mar 15, 2017

After installing php5-pgsql 5.6.30 which includes this fix, my PosgreSQL 9.5.6 server logs an error after each INSERT statement, as follow:

2017-03-15 17:30:18.982 CET [25960] user@db ERROR: lastval is not yet defined in this session
2017-03-15 17:30:18.982 CET [25960] user@db STATEMENT: SELECT LASTVAL()
2017-03-15 17:30:18.986 CET [25961] user@db ERROR: lastval is not yet defined in this session
2017-03-15 17:30:18.986 CET [25961] user@db STATEMENT: SELECT LASTVAL()
2017-03-15 17:30:19.975 CET [25962] user@db ERROR: lastval is not yet defined in this session
2017-03-15 17:30:19.975 CET [25962] user@db STATEMENT: SELECT LASTVAL()

The INSERT is proprely handled by postgresql, but the pdo issue a SELECT LASTVAL(); each time, logging the error.

If I read the PosgreSQL docs correctly at www.postgresql.org, it is said that "It is an error to call lastval if nextval has not yet been called in the current session."

That's also the opinion of some PostgreSQL professionals, as seen here: stackoverflow.com

I didn't have the problem with the very same application with php5-pgsql 5.6.25. My application didn't change in between.

I think the use of LASTVAL() in this context is a regression.

@weltling

This comment has been minimized.

Copy link
Contributor

weltling commented Mar 15, 2017

@lcligny until i miss something, this PR was about ext/pdo_pgsql, not ext/pgsql. What you say is right, however - the PG server will issue an error, if the sequence is not yet started but you call LASTVAL(). So likely it's in your application, as pgsql might handle it differently. FYI - in 7.1 pdo_pgsql will also throw an error in this case, 5.6 and 7.0 use savepoints for BC. If you still think there's a bug on PHP's side, please prepare a reproduce snippet and file a ticket on bugs.php.net.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.