Date: Wed, 24 Jan 2024 12:45:32 -0800
From: Noah Misch <noah@leadboat.com>
To: Fujii Masao <masao.fujii@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: dblink query interruptibility
Message-ID: <20240124204532.1b.nmisch@google.com>
References: <20231122012945.74@rfd.leadboat.com>
 <CAHGQGwGpDTXeg8K1oTmDv9nankaKTrCD-XW-tnkzo6=E9p5=uw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAHGQGwGpDTXeg8K1oTmDv9nankaKTrCD-XW-tnkzo6=E9p5=uw@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240124204532.1b.nmisch%40google.com>
Precedence: bulk

On Thu, Jan 25, 2024 at 04:23:39AM +0900, Fujii Masao wrote:
> I found that this patch was committed at d3c5f37dd5 and changed the
> error message in postgres_fdw slightly. Here's an example:
> 
> #1. Begin a new transaction.
> #2. Execute a query accessing to a foreign table, like SELECT * FROM
> <foreign table>
> #3. Terminate the *remote* session corresponding to the foreign table.
> #4. Commit the transaction, and then currently the following error
> message is output.
> 
>     ERROR:  FATAL:  terminating connection due to administrator command
>     server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
>    invalid socket
> 
> Previously, before commit d3c5f37dd5, the error message at #4 did not
> include "invalid socket." Now, after the commit, it does. Is this
> change intentional?

No.  It's a consequence of an intentional change in libpq call sequence, but I
was unaware I was changing an error message.

> + /* Consume whatever data is available from the socket */
> + if (PQconsumeInput(conn) == 0)
> + {
> + /* trouble; expect PQgetResult() to return NULL */
> + break;
> + }
> + }
> +
> + /* Now we can collect and return the next PGresult */
> + return PQgetResult(conn);
> 
> This code appears to cause the change. When the remote session ends,
> PQconsumeInput() returns 0 and marks conn->socket as invalid.
> Subsequent PQgetResult() calls pqWait(), detecting the invalid socket
> and appending "invalid socket" to the error message.
> 
> I think the "invalid socket" message is unsuitable in this scenario,
> and PQgetResult() should not be called after PQconsumeInput() returns
> 0. Thought?

The documentation is absolute about the necessity of PQgetResult():

  PQsendQuery cannot be called again (on the same connection) until
  PQgetResult has returned a null pointer, indicating that the command is
  done.

  PQgetResult must be called repeatedly until it returns a null pointer,
  indicating that the command is done. (If called when no command is active,
  PQgetResult will just return a null pointer at once.)

Similar statements also appear in libpq-pipeline-results,
libpq-pipeline-errors, and libpq-copy.


So, unless the documentation or my reading of it is wrong there, I think the
answer is something other than skipping PQgetResult().  Perhaps PQgetResult()
should not append "invalid socket" in this case?  The extra line is a net
negative, though it's not wrong and not awful.

Thanks for reporting the change.