Thread: dblink query interruptibility

=== Background

Something as simple as the following doesn't respond to cancellation.  In
v15+, any DROP DATABASE will hang as long as it's running:

  SELECT dblink_exec(
    $$dbname='$$||current_database()||$$' port=$$||current_setting('port'),
    'SELECT pg_sleep(15)');

https://postgr.es/m/4B584C99.8090004@enterprisedb.com proposed a fix back in
2010.  Latches and the libpqsrv facility have changed the server programming
environment since that patch.  The problem statement also came up here:

On Thu, Dec 08, 2022 at 06:08:15PM -0800, Andres Freund wrote:
> dblink.c uses a lot of other blocking libpq functions, which obviously also
> isn't ok.


=== Key decisions

This patch adds to libpqsrv facility.  It dutifully follows the existing
naming scheme.  For greppability, I am favoring renaming new and old functions
such that the libpq name is a substring of this facility's name.  That is,
rename libpqsrv_disconnect to srvPQfinish or maybe libpqsrv_PQfinish().  Now
is better than later, while pgxn contains no references to libpqsrv.  Does
anyone else have a preference between naming schemes?  If nobody does, I'll
keep today's libpqsrv_disconnect() style.

I was tempted to add a timeout argument to each libpqsrv function, which would
allow libpqsrv_get_result_last() to replace pgfdw_get_cleanup_result().  We
can always add a timeout-accepting function later and make this thread's
function name a thin wrapper around it.  Does anyone feel a mandatory timeout
argument, accepting -1 for no timeout, would be the right thing?


=== Minor topics

It would be nice to replace libpqrcv_PQgetResult() and friends with the new
functions.  I refrained since they use ProcessWalRcvInterrupts(), not
CHECK_FOR_INTERRUPTS().  Since walreceiver already reaches
CHECK_FOR_INTERRUPTS() via libpqsrv_connect_params(), things might just work.

This patch contains a PQexecParams() wrapper, called nowhere in
postgresql.git.  It's inessential, but twelve pgxn modules do mention
PQexecParams.  Just one mentions PQexecPrepared, and none mention PQdescribe*.

The patch makes postgres_fdw adopt its functions, as part of confirming the
functions are general enough.  postgres_fdw create_cursor() has been passing
the "DECLARE CURSOR FOR inner_query" string for some error messages and just
inner_query for others.  I almost standardized on the longer one, but the test
suite checks that.  Hence, I standardized on just inner_query.

I wrote this because pglogical needs these functions to cooperate with v15+
DROP DATABASE (https://github.com/2ndQuadrant/pglogical/issues/418).

Thanks,
nm

On Wed, Nov 22, 2023 at 10:29 AM Noah Misch <noah@leadboat.com> wrote:
>
> === Background
>
> Something as simple as the following doesn't respond to cancellation.  In
> v15+, any DROP DATABASE will hang as long as it's running:
>
>   SELECT dblink_exec(
>     $$dbname='$$||current_database()||$$' port=$$||current_setting('port'),
>     'SELECT pg_sleep(15)');
>
> https://postgr.es/m/4B584C99.8090004@enterprisedb.com proposed a fix back in
> 2010.  Latches and the libpqsrv facility have changed the server programming
> environment since that patch.  The problem statement also came up here:
>
> On Thu, Dec 08, 2022 at 06:08:15PM -0800, Andres Freund wrote:
> > dblink.c uses a lot of other blocking libpq functions, which obviously also
> > isn't ok.
>
>
> === Key decisions
>
> This patch adds to libpqsrv facility.

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?

+ /* 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?

Regards,

--
Fujii Masao

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.

On Thu, Jan 25, 2024 at 5:45 AM Noah Misch <noah@leadboat.com> wrote:
>
> 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():

The documentation looks unclear to me regarding what should be done
when PQconsumeInput() returns 0. So I'm not sure if PQgetResult()
must be called even in that case.

As far as I read some functions like libpqrcv_PQgetResult() that use
PQconsumeInput(), it appears that they basically report the error message
using PQerrorMessage(), without calling PQgetResult(),
when PQconsumeInput() returns 0.

Regards,

--
Fujii Masao

On Thu, Jan 25, 2024 at 12:28:43PM +0900, Fujii Masao wrote:
> On Thu, Jan 25, 2024 at 5:45 AM Noah Misch <noah@leadboat.com> wrote:
> > 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.

Other clients have witnessed the extra "invalid socket" message:
https://dba.stackexchange.com/questions/335081/how-to-investigate-intermittent-postgres-connection-errors
https://stackoverflow.com/questions/77781358/pgbackrest-backup-error-with-exit-code-57
https://github.com/timescale/timescaledb/issues/102

> > > + /* 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.

What do you think of making PQconsumeInput() set PGASYNC_READY and
CONNECTION_BAD in this case?  Since libpq appended "server closed the
connection unexpectedly", it knows those indicators are correct.  That way,
PQgetResult() won't issue a pointless pqWait() call.

> > > 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():
> 
> The documentation looks unclear to me regarding what should be done
> when PQconsumeInput() returns 0. So I'm not sure if PQgetResult()
> must be called even in that case.

I agree PQconsumeInput() docs don't specify how to interpret it returning 0.

> As far as I read some functions like libpqrcv_PQgetResult() that use
> PQconsumeInput(), it appears that they basically report the error message
> using PQerrorMessage(), without calling PQgetResult(),
> when PQconsumeInput() returns 0.

libpqrcv_PQgetResult() is part of walreceiver, where any ERROR becomes FATAL.
Hence, it can't hurt anything by eagerly skipping to ERROR.  I designed
libpqsrv_exec() to mimic PQexec() as closely as possible, so it would be a
drop-in replacement for arbitrary callers.  Ideally, accepting interrupts
would be the only caller-visible difference.

I know of three ways PQconsumeInput() can return 0, along with my untested
estimates of how they work:

a. Protocol violation.  handleSyncLoss() sets PGASYNC_READY and
   CONNECTION_BAD.  PQgetResult() is optional.

b. Connection broken.  PQgetResult() is optional.

c. ENOMEM.  PGASYNC_ and CONNECTION_ status don't change.  Applications choose
   among (c1) free memory and retry, (c2) close the connection, or (c3) call
   PQgetResult() to break protocol sync and set PGASYNC_IDLE:

Comparing PQconsumeInput() with the PQgetResult() block under "while
(conn->asyncStatus == PGASYNC_BUSY)", there's a key difference that
PQgetResult() sets PGASYNC_IDLE on most errors, including ENOMEM.  That
prevents PQexec() subroutine PQexecFinish() from busy looping on ENOMEM, but I
suspect that also silently breaks protocol sync.  While we could change it
from (c3) to (c2) by dropping the connection via handleSyncLoss() or
equivalent, I'm not confident about that being better.

libpqsrv_exec() implements (c3) by way of calling PQgetResult() after
PQconsumeInput() fails.  If PQisBusy(), the same ENOMEM typically will repeat,
yielding (c3).  If memory became available in that brief time, PQgetResult()
may instead block.  That blocking is unwanted but unimportant.