Feedback-ID: id4a34324:Fastmail
Date: Thu, 29 Dec 2022 09:23:30 -0800
From: Andres Freund <andres@anarazel.de>
To: pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>, Peter Geoghegan <pg@bowt.ie>,
	Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: postgres_fdw uninterruptible during connection establishment /
 ProcSignalBarrier
Message-ID: <20221229172330.jpuoljhtvivci3cd@awork3.anarazel.de>
References: <20220925232237.p6uskba2dw6fnwj2@awork3.anarazel.de>
 <20221209020815.wsoixct72c6hst5l@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221209020815.wsoixct72c6hst5l@awork3.anarazel.de>
Archived-At: 
 <https://www.postgresql.org/message-id/20221229172330.jpuoljhtvivci3cd%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2022-12-08 18:08:15 -0800, Andres Freund wrote:
> I just re-discovered this issue, via
> https://www.postgresql.org/message-id/20221209003607.bz2zdznvfnkq4zz3%40awork3.anarazel.de
> 
> 
> On 2022-09-25 16:22:37 -0700, Andres Freund wrote:
> > Maybe I am missing something, but I don't think it's OK for
> > connect_pg_server() to connect in a blocking manner, without accepting
> > interrupts?
> 
> It's definitely not. This basically means network issues or such can lead to
> connections being unkillable...
> 
> We know how to do better, c.f. libpqrcv_connect(). I hacked that up for
> postgres_fdw, and got through quite a few runs without related issues ([1]).
> 
> The same problem is present in two places in dblink.c. Obviously we can copy
> and paste the code to dblink.c as well. But that means we have the same not
> quite trivial code in three different c files. There's already a fair bit of
> duplicated code around AcquireExternalFD().
> 
> It seems we should find a place to put backend specific libpq wrapper code
> somewhere. Unless we want to relax the rule about not linking libpq into the
> backend we can't just put it in the backend directly, though.
> 
> The only alternative way to provide a wrapper that I can think of are to
> a) introduce a new static library that can be linked to by libpqwalreceiver,
>    postgres_fdw, dblink
> b) add a header with static inline functions implementing interrupt-processing
>    connection establishment for libpq
> 
> Neither really has precedent.
> 
> The attached quick patch just adds and uses libpq_connect_interruptible() in
> postgres_fdw. If we wanted to move this somewhere generic, at least part of
> the external FD handling should also be moved into it.
> 
> 
> dblink.c uses a lot of other blocking libpq functions, which obviously also
> isn't ok.
> 
> 
> Perhaps we could somehow make this easier from within libpq? My first thought
> was a connection parameter that'd provide a different implementation of
> pqSocketCheck() or such - but I don't think that'd work, because we might
> throw errors when processing interrupts, and that'd not be ok from deep within
> libpq.

Any opinions?  Due to the simplicity I'm currently leaning to a header-only
helper, but I don't feel confident about it.

The rate of cfbot failures is high enough that it'd be good to do something
about this.

Greetings,

Andres Freund