Date: Wed, 22 Jan 2025 10:22:45 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Paul Ramsey <pramsey@cleverelephant.ca>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Converting pqsignal to void return
Message-ID: <Z5EbVXiPWHh2dR2G@nathan>
References: <4E796D56-79B5-4447-9DA7-5428BC1A6B53@cleverelephant.ca>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <4E796D56-79B5-4447-9DA7-5428BC1A6B53@cleverelephant.ca>
Archived-At: <https://www.postgresql.org/message-id/Z5EbVXiPWHh2dR2G%40nathan>
Precedence: bulk

On Wed, Jan 22, 2025 at 07:57:52AM -0800, Paul Ramsey wrote:
> I just ran across this change when a bunch of CI´s I run barfed.

Oops, sorry.

> The problem we are having in extension land is that we often run
> functions in external libraries that take a long time to return, and we
> would like to ensure that PgSQL users can cancel their queries, even when
> control has passed into those functions.
> 
> The way we have done it, historically, has been to take the return value
> of pqsignal(SIGINT, extension_signint_handler) and remember it, and then,
> inside extension_signint_handler, call the pgsql handler once we have
> done our own business..

I see a couple other projects that might be doing something similar [0].

> It is possible we have been Doing It Wrong all this time, and would love
> some pointers on the right way to do this.
> 
> Alternatively, if we have a valid use case, it would be nice to have the
> return value from pqsignal() back.

I don't think you were doing it wrong before, but commit 3b00fdb did add
some weird race conditions to the return value, but only in rare cases that
I don't think apply to the case you shared.

I was tempted to suggest that you try using sigaction() with the second
"act" argument set to NULL to retrieve the original signal handler, but I
think there's a good chance you'd get our wrapper_handler() function (we
store the actual handlers in a separate array), which is no good.  So, I
should probably just revert d4a43b2.  An alternative approach could be to
provide our own way to retrieve the current handler, but that comes with
its own race conditions, although perhaps those are more obvious than the
pqsignal() ones.  WDYT?

[0] https://codesearch.debian.net/search?q=%3D+pqsignal&literal=1

-- 
nathan