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

pgsql: Insert_ID() - PostgreSQL8 driver assumes that lastval() will be set. #978

Closed
1 task done
NathanGibbs3 opened this issue Jun 9, 2023 · 10 comments · Fixed by #979
Closed
1 task done

pgsql: Insert_ID() - PostgreSQL8 driver assumes that lastval() will be set. #978

NathanGibbs3 opened this issue Jun 9, 2023 · 10 comments · Fixed by #979
Labels
bug pgsql PostgreSQL (Tier 1)
Milestone

Comments

@NathanGibbs3
Copy link
Contributor

NathanGibbs3 commented Jun 9, 2023

Description

Getting the error and stack trace below in CI when calling Insert_ID() on a newly initialized ADOdb object connected to a PostgreSQL DB.

1) dbTest::testbaseInsertIDReturnsExpected
postgres9 error: [-1: ERROR:  lastval is not yet defined in this session] in EXECUTE("SELECT lastval()")

/home/travis/build/NathanGibbs3/BASE/build/adodb/ADOdb-5.22.5/adodb-errorhandler.inc.php:84
/home/travis/build/NathanGibbs3/BASE/build/adodb/ADOdb-5.22.5/adodb.inc.php:1624
/home/travis/build/NathanGibbs3/BASE/build/adodb/ADOdb-5.22.5/adodb.inc.php:1590
/home/travis/build/NathanGibbs3/BASE/build/adodb/ADOdb-5.22.5/adodb.inc.php:2142
/home/travis/build/NathanGibbs3/BASE/build/adodb/ADOdb-5.22.5/drivers/adodb-postgres8.inc.php:51
/home/travis/build/NathanGibbs3/BASE/build/adodb/ADOdb-5.22.5/adodb.inc.php:1741
/home/travis/build/NathanGibbs3/BASE/includes/base_db.inc.php:467
/home/travis/build/NathanGibbs3/BASE/tests/PhpUnit/idbTest.php:353

Environment

  • ADOdb version: 5.20.0 - 5.22.5
  • Driver or Module: postgres
  • Database type and version: PostgreSQL 9.2 - 9.6
  • PHP version: 7.0.25 - 8.1.12
  • Platform: Linux
  • I have tested that the problem is reproducible in the latest release

Steps to reproduce

Detailed, step-by-step instructions to reproduce the behavior, including:

  • Initialize an ADOdb object connected to a PostgreSQL DB.
  • Call Insert_ID().

Expected behavior

Would expect Insert_ID() to return false as per the documentation and not throw an error.

Additional context

Link to CI build were we can see the breakage on PostgreSQL for PHP versions using ADOdb 5.20+
https://app.travis-ci.com/github/NathanGibbs3/BASE/builds/263709789

@NathanGibbs3 NathanGibbs3 added the triage New issues not yet reviewed by ADOdb developers label Jun 9, 2023
@NathanGibbs3 NathanGibbs3 changed the title Insert_ID() - PostgreSQL driver assumes that lastval() will be set. pgsql: Insert_ID() - PostgreSQL8 driver assumes that lastval() will be set. Jun 9, 2023
NathanGibbs3 added a commit to NathanGibbs3/ADOdb that referenced this issue Jun 9, 2023
@NathanGibbs3
Copy link
Contributor Author

Link to CI build with this fixed.
https://app.travis-ci.com/github/NathanGibbs3/BASE/builds/263713300
PHP 7+ is using my ADOdb fork with this fixed.
Will PR it shortly.

FYI, something between 5.22.5 & present broke the return from calls to metacolumns() in the mysqli driver.

@dregad
Copy link
Member

dregad commented Jun 10, 2023

I just tried this and it seems to return false as expected - am I missing something ?

include 'adodb.inc.php'; 
$db = ADONewConnection('pgsql');
$db->connect(...);
var_dump($db->insert_id()); // false

@dregad
Copy link
Member

dregad commented Jun 10, 2023

Ah I get it - the problem occurs when using exceptions, i.e. with include 'adodb-exceptions.inc.php'; I get

Fatal error: Uncaught ADODB_Exception: postgres9 error: [-1: ERROR: lastval is not yet defined in this session] in EXECUTE("SELECT lastval()") in /Users/dregad/dev/adodb/adodb-exceptions.inc.php on line 97

which matches the error reported in your CI.

I'll have a look at your PR.

@dregad dregad added bug pgsql PostgreSQL (Tier 1) and removed triage New issues not yet reviewed by ADOdb developers labels Jun 10, 2023
@dregad dregad added this to the v5.22.6 milestone Jun 10, 2023
@dregad dregad linked a pull request Jun 10, 2023 that will close this issue
NathanGibbs3 added a commit to NathanGibbs3/ADOdb that referenced this issue Jun 10, 2023
@dregad dregad closed this as completed in b819440 Jun 10, 2023
@NathanGibbs3
Copy link
Contributor Author

NathanGibbs3 commented Jun 11, 2023

Is it possible to backport this fix into the 5.21 and/or the 5.20 lines.
The code responsible for this issue was introduced in ADOdb 5.18 postgres9 driver.
Moved down to the postgres8 driver in 5.19

At the moment, it may be necessary to downgrade my project's maximum supported ADOdb versions to 5.17 on older PHPs.
https://github.com/NathanGibbs3/BASE/wiki/ADOdb-version-requirements-by-PHP-Version

I CI test against the minimum supported versions, If I had tested against the maximum supported versions, this issue would have been present in the PHP 5 tests as well.

I'll take into account users running ADOdb 5.18 - 5.22.5 and compensate for this error in my project's code, when the installed version of ADOdb does not. Giving the end uses who for whatever reasons are using older PHP's an upgrade path for older ADOdb's would be greatly appreciated if possible.

NathanGibbs3 added a commit to NathanGibbs3/ADOdb that referenced this issue Jun 11, 2023
NathanGibbs3 added a commit to NathanGibbs3/ADOdb that referenced this issue Jun 11, 2023
@NathanGibbs3
Copy link
Contributor Author

NathanGibbs3 commented Jun 11, 2023

I have PR #983 for the 5.21 codebase.

I have https://github.com/NathanGibbs3/ADOdb/tree/NathanGibbs3-Issue%23978for5.20 in my fork for the 5.20 codebase.
for the 5.20 codebase.

Hopefully that will avoid adding any 5.21+ code into it, if you choose to use it.

@NathanGibbs3
Copy link
Contributor Author

NathanGibbs3 commented Jun 11, 2023

Ah I get it - the problem occurs when using exceptions, i.e. with include 'adodb-exceptions.inc.php'; I get

For what it's worth, I'm nor including the exception handler, just the error handler.
include 'adodb-errorhandler.inc.php #981 may still exist there.
You may want to look at adodb-errorpear.inc.php as well.

@NathanGibbs3
Copy link
Contributor Author

ADOdb 5.22x #984
ADOdb 5.21x #985
ADOdb 5.20.x https://github.com/NathanGibbs3/ADOdb/tree/NathanGibbs3-Issue%23981for5.20

@dregad
Copy link
Member

dregad commented Jun 12, 2023

Is it possible to backport this fix into the 5.21 and/or the 5.20 lines.

I'm sorry, but I need to take a strong stance here and reject this request.

Our support policy and the reason for it are quite clearly stated on our website (and we even discussed that last week) - only stable (5.22.x as of this writing) and legacy (5.21.x) releases are supported, and the latter only gets security and critical bug fixes.

Unfortunately, this bug hardly qualifies as critical.

I suggest you patch your code to work around the problem, or alternatively feel free to maintain a modified copy of ADOdb.

@NathanGibbs3
Copy link
Contributor Author

I understand.
As direct result of our conversation about open source development last week I went from my original position of.

Is it possible to backport ...

to the position of
Doing the actual work & submitting PR's containing what I was originally asking you for.

If you want them in the future., they are there.
Either way, I am happy to have provided them. 😄
That is the idea & values behind Open Source after all.

As I've stated and in alignment with your suggestion.
I'll take into account users running ADOdb 5.18 - 5.22.6 and compensate for this error in my project's code.
I would have to do that anyways, as end users don't always update to the latest ADOdb.

Thank you again for your consideration .

@dregad
Copy link
Member

dregad commented Jun 13, 2023

Thanks for your understanding 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug pgsql PostgreSQL (Tier 1)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants