dblink query interruptibility

From: Noah Misch <noah(at)leadboat(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: dblink query interruptibility
Date: 2023-11-22 01:29:45
Message-ID: 20231122012945.74@rfd.leadboat.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

=== 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

Attachment Content-Type Size
libpqsrv-exec-v1.patch text/plain 18.4 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2023-11-22 01:43:32 Re: [PATCH] fix race condition in libpq (related to ssl connections)
Previous Message Peter Smith 2023-11-22 01:17:30 Re: pg_upgrade and logical replication