Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier

From: Andres Freund <andres(at)anarazel(dot)de>
To: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org, Robert Haas <robertmhaas(at)gmail(dot)com>, Peter Geoghegan <pg(at)bowt(dot)ie>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Fujii Masao <fujii(at)postgresql(dot)org>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>
Subject: Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
Date: 2023-01-21 03:00:08
Message-ID: 20230121030008.acoon7nk7gnuslx4@awork3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On 2023-01-14 14:45:06 +1300, Thomas Munro wrote:
> On Tue, Jan 10, 2023 at 3:05 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2023-01-03 12:05:20 -0800, Andres Freund wrote:
> > > The attached patch adds libpq-be-fe-helpers.h and uses it in libpqwalreceiver,
> > > dblink, postgres_fdw.
> >
> > > As I made libpq-be-fe-helpers.h handle reserving external fds,
> > > libpqwalreceiver now does so. I briefly looked through its users without
> > > seeing cases of leaking in case of errors - which would already have been bad,
> > > since we'd already have leaked a libpq connection/socket.
> > >
> > >
> > > Given the lack of field complaints and the size of the required changes, I
> > > don't think we should backpatch this, even though it's pretty clearly buggy
> > > as-is.
> >
> > Any comments on this? Otherwise I think I'll go with this approach.
>
> +1. Not totally convinced about the location but we are free to
> re-organise it any time, and the random CI failures are bad.

Cool.

Updated patch attached. I split it into multiple pieces.
1) A fix for [1], included here because I encountered it while testing
2) Introduction of libpq-be-fe-helpers.h
3) Convert dblink and postgres_fdw to the helper
4) Convert libpqwalreceiver.c to the helper

Even if we eventually decide to backpatch 3), we'd likely not backpatch 4), as
there's no bug (although perhaps the lack of FD handling could be called a
bug?).

There's also some light polishing, improving commit message, comments and
moving some internal helper functions to later in the file.

Greetings,

Andres Freund

[1] https://postgr.es/m/20230121011237.q52apbvlarfv6jm6%40awork3.anarazel.de

Attachment Content-Type Size
v3-0001-Fix-error-handling-in-libpqrcv_connect.patch text/x-diff 2.3 KB
v3-0002-Add-helper-library-for-use-of-libpq-inside-the-se.patch text/x-diff 8.3 KB
v3-0003-dblink-postgres_fdw-Handle-interrupts-during-conn.patch text/x-diff 8.1 KB
v3-0004-libpqwalreceiver-Convert-to-libpq-be-fe-helpers.h.patch text/x-diff 3.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2023-01-21 03:27:07 Re: Reduce timing overhead of EXPLAIN ANALYZE using rdtsc?
Previous Message Andres Freund 2023-01-21 02:50:37 Re: libpqrcv_connect() leaks PGconn