ReportRemoteError() changes can lead to PurgeConnection() being missed.

[anarazel]

Previously calling code could do a PurgeConnection() after a command failed. But with the changed API that's not really possible anymore. As the result and connection needs to be passed to ReraiseRemoteError(), result/connection can't be cleared afterwards. Due to raised error it can't be cleared afterwards either.

That's problematic because at that stage not all data from the remote site might have been consumed, we might be in an incomplete transactional state, or various variations like that. For #578 I've temporarily hacked things together by putting

    if (errorLevel >= ERROR)
    {
        PurgeConnection(connection);
    }

just before the ereport()s in ReportRemoteError(), but ...

[jasonmp85]

Good catch. I suggest we look at what postgres_fdw does for this (I believe it wraps its own throw in a try/catch in order to have a block where it can reliably clear things).

[anarazel]

That's a possibility, yes. I'm actually kind of inclined to rather go the route of the PurgeConnection inside ReraiseRemoteError(). Not pretty, but a lot easier. Writing correct code around PG_TRY/CATCH isn't easy (cf. SETJMP(3)'s NOTES section).

[metdos]

We also need to clear PGresult in the error case, right?

[anarazel]

On 2016-06-10 07:16:30 -0700, Metin Döşlü wrote:

We also need to clear PGresult in the error case, right?

Closing a connection should take care of that:

static void
closePGconn(PGconn *conn)
{
...
    pqClearAsyncResult(conn);   /* deallocate result */
}
...

void
pqClearAsyncResult(PGconn *conn)
{
    if (conn->result)
        PQclear(conn->result);
    conn->result = NULL;
    if (conn->next_result)
        PQclear(conn->next_result);
    conn->next_result = NULL;
}