Please port to the Framework as well.

@mbabker I'm on my phone at the moment but I don't recall the framework having a getErrorNumber() method in the driver. I think the framework handles these error requests directly and puts the error in a log while in the method that has a possible error condition like $this->cursor being false.

It doesn't have getErrorNumber() methods, the errors are handled inline to the execute() methods (the CMS abstracted it out in part to mask the table prefixes which we aren't doing on the Framework; actually there the SQL query is no longer part of the Exception message).

So nothing to port to the framework?

Do we need to quotename, escape or remove the sql from the exception here?

The check if $this->cursor is false needs to be added for the two calls to pg_result_error_field() for the same reason you're PR'ing it here. Except instead of throwing an Exception directly I'd just give it a default 0 value since the Exception gets thrown a couple lines down there.

@mbabker do we need to add additional if ($this->cursor === false) when the framework is already checking with if (!$this->cursor) ? I believe this should already catch the false condition for $this->cursor.

Eh, fair point. But if pg_result_error_field() requires a valid resource and that entire conditional operates with a boolean value then that whole branch is bugged anyway and we probably need to do something with it.

Sorry, but it seems to me to rather be an issue that we are using the Postgres functions incorrectly. pg_result_error_field() has a specific comment that you should use different functions to get a resource instead of either resource or boolean. Seems to me as if the code would have to look somewhat more like

pg_send_query($this->connection, $query);

$this->cusor = pg_get_result($this->connection);
if (pg_result_error($this->cursor) != '')
{
    // Get the error number and message before we execute any more queries.
    $errorNum = $this->getErrorNumber();
    $errorMsg = $this->getErrorMessage($query);

    // Check if the server was disconnected.
    if (!$this->connected())
    {
        try
        {
            // Attempt to reconnect.
            $this->connection = null;
            $this->connect();
        }
        // If connect fails, ignore that exception and throw the normal exception.
        catch (RuntimeException $e)
        {
            $this->errorNum = $this->getErrorNumber();
            $this->errorMsg = $this->getErrorMessage($query);
            // Throw the normal query exception.
            JLog::add(JText::sprintf('JLIB_DATABASE_QUERY_FAILED', $this->errorNum, $this->errorMsg), JLog::ERROR, 'database-error');
            throw new JDatabaseExceptionExecuting($query, $this->errorMsg, null, $e);
        }
        // Since we were able to reconnect, run the query again.
        return $this->execute();
    }
    // The server was not disconnected.
    else
    {
        // Get the error number and message from before we tried to reconnect.
        $this->errorNum = $errorNum;
        $this->errorMsg = $errorMsg;
        // Throw the normal query exception.
        JLog::add(JText::sprintf('JLIB_DATABASE_QUERY_FAILED', $this->errorNum, $this->errorMsg), JLog::ERROR, 'database-error');
        throw new JDatabaseExceptionExecuting($query, $this->errorMsg);
    }
}
return $this->cursor;

Can't say I've looked through the pgsql_* docs closely but if we need to fix that stuff let's do it.

Ok, I tried the code that I posted above and it does NOT work. The problem is still, that we are using pg_query() and later, in case of an error, expect a resource where there can't be one. I don't see a case where getErrorNumber() could not run into the JDatabaseExceptionExecuting exception of this PR. If there is an error in pg_query(), we don't have a resource. But if there is no resource, we can not get the error number. I have no idea what to do.

When I started trying to solve this I looked a little at trying to follow the recommendation for pg_result_error_field() that "if the query fails, you must use pg_send_query() and pg_get_result() to get the result handle" but I read something about pg_send_query() not being able to be used with trasactions. Which is one reason I moved towards throwing an Exception when the query fails.

I can't find the reference regarding pg_send_query() and transaction issues at the moment.

@Hackwar two issues with the code you posted, you need to "call pg_get_result() one or more times to obtain the results" and we also have to take into consideration "pg_send_query() cannot be called again (on the same connection) until pg_get_result() has returned a null pointer"

I think we are likely opening a big rework if we go down the road of implementing Asynchronous Command Processing just to handle a single error condition from a query failing.

I followed the comment in the docs of pg_result_error_field() where it indeed says to use pg_send_query() and pg_get_result(). But after testing that code and then reading the docs carefully of pg_get_result(), I've seen that that one, too, returns false upon error. So the comment is simply wrong. Seems like pg_last_error() or something like that would actually be the way to go?

I agree, Let's move forward on merging this PR.

Bump for merge consideration