PostgreSQL recordset close fails when query id == -1

[Mike-Benoit]

driver is broken due to regression introduced by #834, which has changed _queryId checks to be something like:

if ( $this->_queryID === false ) { ... }

However _queryId can be false, but it is also -1 quite often. See Line 392 of adodb.inc.php and Line 79 of adodb-csvlib.inc.php as a few examples. There are also several places where -1 might be used as a dummy ID so it still passes checks like:

if ( $this->_queryID )

[Mike-Benoit]

The below IF statement in the _close() function at least works around a PHP fatal error (passing -1 to pg_free_result() ) in our testing, but more thought may need to go into how queryID handling is performed in regards to "-1" and being backwards compatible with PHP < v8.1

drivers/adodb-postgres64.inc.php

function _close()
	{
		if ($this->_queryID === false || $this->_queryID == -1 ) {
			return true;
		}
		return pg_free_result($this->_queryID);
	}

[dregad]

See Line 392 of adodb.inc.php

@Mike-Benoit I assume you actually meant 3932 here ?

ADOdb/adodb.inc.php

Lines 3931 to 3932 in a567820

	/** @var resource result link identifier */
	var $_queryID = -1;

For consistency, I think we should change that value to false, matching ADOConnection::$_queryID, although this is somewhat pointless since the property is always initialized in the class constructor anyway.

This normally happens after executing a query from the connection, when the recordset is created. The only 2 exceptions to this I can think of, are:

• the csvlib example you mentioned
• In _adodb_getinsertsql() (adodb-lib.inc.php) where we create a dummy recordset so we can call metaType()

Do you know about any other ?

In any case, considering that ADORecordSet::__destruct() calls close(), I think your suggestion is a correct way to fix this; alternatively we could perform the check in the destructor, and not attempt to close the RS if it's a "dummy" one

function __destruct() {
	if($this->_queryID != -1) {
		$this->Close();
	}
}

Also, to be clean and avoid future confusion with this use of -1 magic constant, I think we should define and use a DUMMY_QUERY_ID = -1 constant instead.

[dregad]

Please review and test PR #851.

[Mike-Benoit]

Your PR looks like a more thorough fix, and is passing our own internal unit tests.