Date: Wed, 18 Sep 2024 19:53:48 -0700
From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>, Peter Geoghegan <pg@bowt.ie>
Cc: Francesco Degrassi <francesco.degrassi@optionfactory.net>,
	pgsql-bugs@lists.postgresql.org
Subject: Re: Leader backend hang on IPC/ParallelFinish when LWLock held at
 parallel query start
Message-ID: <20240919025348.c5.nmisch@google.com>
References: 
 <CAC-SaSzHUKT=vZJ8MPxYdC_URPfax+yoA1hKTcF4ROz_Q6z0_Q@mail.gmail.com>
 <20240918030159.2a.nmisch@google.com>
 <1737570.1726633422@sss.pgh.pa.us>
 <20240918050434.13.nmisch@google.com>
 <1744648.1726636728@sss.pgh.pa.us>
 <CAH2-Wz=aNjJzOwoA1TjBvSUdqJ9uzO9L4LqcjapMz+zD0HA-+g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAH2-Wz=aNjJzOwoA1TjBvSUdqJ9uzO9L4LqcjapMz+zD0HA-+g@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240919025348.c5.nmisch%40google.com>
Precedence: bulk

On Wed, Sep 18, 2024 at 01:45:31AM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > What if we just ignored the plancache when uninterruptible?  The new planner
> > check would then suffice.
> 
> Only if you believe that parallel-query is the only problem,
> which is something I seriously doubt.  I fear that the
> committed patch is just a band-aid over one symptom of the
> general problem that we can't permit arbitrary operations
> to be invoked from a btree comparison function.  It's late
> here so I'm not sufficiently awake to think of examples,
> but I'm sure there are some.

I bet one can cause an (undetected) lwlock deadlock, but I don't know of a way
to do that "accidentally".

> However ... clearly a maliciously-coded btree support function can
> do arbitrarily bad stuff.  We restrict creation of opclasses to
> superusers for exactly that reason.  If our ambitions are only
> to prevent support functions from *accidentally* causing problems,
> is disabling parallel query enough?  I'm still pretty uncomfortable
> about it, but it's less obviously insufficient than in the general
> case.

I'm okay stopping at blocking some known accidents.  I'm not aware of an
approach that would block 100% of unwanted outcomes here without also removing
a feature folks would miss.  Nice things about blocking parallel query:

- Cheap to block
- Always hangs if attempted, so blocking it doesn't take away something of value
- Query still completes, without parallelism
- Writers of opclass functions probably aren't thinking of parallel query

For what it's worth, I tried making standard_ExecutorStart() warn if
!INTERRUPTS_CAN_BE_PROCESSED().  Only this thread's new test and
004_verify_nbtree_unique reached the warning.  (That's not a surprise.)

On Wed, Sep 18, 2024 at 08:59:22AM -0400, Peter Geoghegan wrote:
> The test case provided was intended to be illustrative of a problem
> that some foreign data wrapper ran into, when it used SPI.

Ideally, we'd block those or at least warn under assertions so FDW authors
don't accidentally run the executor with an LWLock held.  Unlike the opclass
case, we so far don't have a valid use case for holding an LWLock there.  In
other words, if the opclass use case is the only known-valid one, it would be
nice to have checks so new use cases don't creep in.