Обсуждение: Segmentation fault on proc exit after dshash_find_or_insert

Hi Rahila,

On Fri, Nov 21, 2025 at 8:45 PM Rahila Syed <rahilasyed90@gmail.com> wrote:
>
> Hi,
>
> If a process encounters a FATAL error after acquiring a dshash lock but before releasing it,
> and it is not within a transaction, it can lead to a segmentation fault.
>
> The FATAL error causes the backend to exit, triggering proc_exit() and similar functions.
> In the absence of a transaction, LWLockReleaseAll() is delayed until ProcKill. ProcKill is
> an on_shmem_exit callback, and dsm_backend_shutdown() is called before any
> on_shmem_exit callbacks are invoked.
> Consequently, if a dshash lock was acquired before the FATAL error occurred, the lock
> will only be released after dsm_backend_shutdown() detaches the DSM segment containing
> the lock, resulting in a segmentation fault.

Thanks for the report.

I am not super familiar with this code path, but this raises a broader
question for me: are there other resources residing in DSM (besides
LWLocks) that might be accessed during the shutdown sequence?

We know dshash and dsa locks (LWLocks) face this risk because ProcKill
runs as an on_shmem_exit callback, which happens after
dsm_backend_shutdown() has already detached the memory.

This patch fixes the specific case for LWLocks, but if there are any
other on_shmem_exit callbacks that attempt to touch DSM memory, they
would still trigger a similar segfault. Do we need to worry about
other cleanup routines, or is ProcKill the only consumer of DSM data
at this stage?

> Please find a reproducer attached. I have modified the test_dsm_registry module to create
> a background worker that does nothing but throws a FATAL error after acquiring the dshash lock.
> The reason this must be executed in the background worker is to ensure it runs without a transaction.
>
> To trigger the segmentation fault, apply the 0001-Reproducer* patch, run make install in the
> test_dsm_registry module, specify test_dsm_registry as shared_preload_libraries in postgresql.conf,
> and start the server.
>
> Please find attached a fix to call LWLockReleaseAll() early in the shmem_exit() routine. This ensures
> that the dshash lock is released before dsm_backend_shutdown() is called. This will  also ensure that
> any subsequent callbacks invoked in shmem_exit() will not fail to acquire any lock.

@@ -229,6 +230,14 @@ shmem_exit(int code)
 {
    shmem_exit_inprogress = true;

+   /*
+    * Make sure we release any pending locks so that any callbacks called
+    * subsequently do not fail to acquire any locks. This also fixes a seg
+    * fault due to releasing a dshash lock after the dsm segment containing
+    * the lock has been detached by dsm_backend_shutdown().
+    */
+   LWLockReleaseAll();
+
    /*
     * Call before_shmem_exit callbacks.
     *

Again, not an expert, but I am concerned about placing
LWLockReleaseAll() at the very top, before before_shmem_exit()
callbacks run.

One of those callbacks might rely on locks being held or assume the
consistency of shared memory structures protected by those locks. It
seems safer to sandwich the release between the two callback lists:
after before_shmem_exit is done, but before dsm_backend_shutdown()
runs.

If we move it there, we should also reword the comment to focus on the
resource lifespan rather than just the symptom ("seg fault"):

    /*
     * Release all LWLocks before we tear down DSM segments.
     *
     * LWLocks that reside in dynamic shared memory (e.g., dshash partition
     * locks) must be released before the backing segment is detached by
     * dsm_backend_shutdown(). If we wait for ProcKill (via on_shmem_exit),
     * the memory will already be unmapped, leading to access violations.
     */

--
Thanks, Amit Langote

On Tue, Dec 2, 2025 at 9:40 AM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi Rahila,
>
> On Fri, Nov 21, 2025 at 8:45 PM Rahila Syed <rahilasyed90@gmail.com> wrote:
> >
> > Hi,
> >
> > If a process encounters a FATAL error after acquiring a dshash lock but before releasing it,
> > and it is not within a transaction, it can lead to a segmentation fault.
> >
> > The FATAL error causes the backend to exit, triggering proc_exit() and similar functions.
> > In the absence of a transaction, LWLockReleaseAll() is delayed until ProcKill. ProcKill is
> > an on_shmem_exit callback, and dsm_backend_shutdown() is called before any
> > on_shmem_exit callbacks are invoked.
> > Consequently, if a dshash lock was acquired before the FATAL error occurred, the lock
> > will only be released after dsm_backend_shutdown() detaches the DSM segment containing
> > the lock, resulting in a segmentation fault.
>
> Thanks for the report.
>
> I am not super familiar with this code path, but this raises a broader
> question for me: are there other resources residing in DSM (besides
> LWLocks) that might be accessed during the shutdown sequence?
>
> We know dshash and dsa locks (LWLocks) face this risk because ProcKill
> runs as an on_shmem_exit callback, which happens after
> dsm_backend_shutdown() has already detached the memory.
>
> This patch fixes the specific case for LWLocks, but if there are any
> other on_shmem_exit callbacks that attempt to touch DSM memory, they
> would still trigger a similar segfault. Do we need to worry about
> other cleanup routines, or is ProcKill the only consumer of DSM data
> at this stage?
>
> > Please find a reproducer attached. I have modified the test_dsm_registry module to create
> > a background worker that does nothing but throws a FATAL error after acquiring the dshash lock.
> > The reason this must be executed in the background worker is to ensure it runs without a transaction.
> >
> > To trigger the segmentation fault, apply the 0001-Reproducer* patch, run make install in the
> > test_dsm_registry module, specify test_dsm_registry as shared_preload_libraries in postgresql.conf,
> > and start the server.
> >
> > Please find attached a fix to call LWLockReleaseAll() early in the shmem_exit() routine. This ensures
> > that the dshash lock is released before dsm_backend_shutdown() is called. This will  also ensure that
> > any subsequent callbacks invoked in shmem_exit() will not fail to acquire any lock.
>
> @@ -229,6 +230,14 @@ shmem_exit(int code)
>  {
>     shmem_exit_inprogress = true;
>
> +   /*
> +    * Make sure we release any pending locks so that any callbacks called
> +    * subsequently do not fail to acquire any locks. This also fixes a seg
> +    * fault due to releasing a dshash lock after the dsm segment containing
> +    * the lock has been detached by dsm_backend_shutdown().
> +    */
> +   LWLockReleaseAll();
> +
>     /*
>      * Call before_shmem_exit callbacks.
>      *
>
> Again, not an expert, but I am concerned about placing
> LWLockReleaseAll() at the very top, before before_shmem_exit()
> callbacks run.
>
> One of those callbacks might rely on locks being held or assume the
> consistency of shared memory structures protected by those locks. It
> seems safer to sandwich the release between the two callback lists:
> after before_shmem_exit is done, but before dsm_backend_shutdown()
> runs.

I think it makes sense to place LWLockReleaseAll() right before the
dsm_backend_shutdown() which is detaching the process from the dsm
desgments.

--
Regards,
Dilip Kumar
Google

Hi,

On 2025-12-02 13:10:29 +0900, Amit Langote wrote:
> On Fri, Nov 21, 2025 at 8:45 PM Rahila Syed <rahilasyed90@gmail.com> wrote:
> >
> > Hi,
> >
> > If a process encounters a FATAL error after acquiring a dshash lock but before releasing it,
> > and it is not within a transaction, it can lead to a segmentation fault.
> >
> > The FATAL error causes the backend to exit, triggering proc_exit() and similar functions.
> > In the absence of a transaction, LWLockReleaseAll() is delayed until ProcKill. ProcKill is
> > an on_shmem_exit callback, and dsm_backend_shutdown() is called before any
> > on_shmem_exit callbacks are invoked.
> > Consequently, if a dshash lock was acquired before the FATAL error occurred, the lock
> > will only be released after dsm_backend_shutdown() detaches the DSM segment containing
> > the lock, resulting in a segmentation fault.
> 
> Thanks for the report.
> 
> I am not super familiar with this code path, but this raises a broader
> question for me: are there other resources residing in DSM (besides
> LWLocks) that might be accessed during the shutdown sequence?
> 
> We know dshash and dsa locks (LWLocks) face this risk because ProcKill
> runs as an on_shmem_exit callback, which happens after
> dsm_backend_shutdown() has already detached the memory.
> 
> This patch fixes the specific case for LWLocks, but if there are any
> other on_shmem_exit callbacks that attempt to touch DSM memory, they
> would still trigger a similar segfault. Do we need to worry about
> other cleanup routines, or is ProcKill the only consumer of DSM data
> at this stage?

I don't think it's really right to frame it as ProcKill() being a consumer of
DSM data - it's just releasing all held lwlocks, and we happen to hold an
lwlock inside a DSM in the problematic case...

There are many other places that do LWLockReleaseAll()...


> > Please find a reproducer attached. I have modified the test_dsm_registry module to create
> > a background worker that does nothing but throws a FATAL error after acquiring the dshash lock.
> > The reason this must be executed in the background worker is to ensure it runs without a transaction.
> >
> > To trigger the segmentation fault, apply the 0001-Reproducer* patch, run make install in the
> > test_dsm_registry module, specify test_dsm_registry as shared_preload_libraries in postgresql.conf,
> > and start the server.
> >
> > Please find attached a fix to call LWLockReleaseAll() early in the shmem_exit() routine. This ensures
> > that the dshash lock is released before dsm_backend_shutdown() is called. This will  also ensure that
> > any subsequent callbacks invoked in shmem_exit() will not fail to acquire any lock.
> 
> @@ -229,6 +230,14 @@ shmem_exit(int code)
>  {
>     shmem_exit_inprogress = true;
> 
> +   /*
> +    * Make sure we release any pending locks so that any callbacks called
> +    * subsequently do not fail to acquire any locks. This also fixes a seg
> +    * fault due to releasing a dshash lock after the dsm segment containing
> +    * the lock has been detached by dsm_backend_shutdown().
> +    */
> +   LWLockReleaseAll();
> +
>     /*
>      * Call before_shmem_exit callbacks.
>      *
> 
> Again, not an expert, but I am concerned about placing
> LWLockReleaseAll() at the very top, before before_shmem_exit()
> callbacks run.

I think it's actually kind of required for correctness, independent of this
crash.  If we errored out while holding an lwlock, we cannot reliably run
*any* code acquiring an lwlock, because lwlocks are not reentrant.


> One of those callbacks might rely on locks being held or assume the
> consistency of shared memory structures protected by those locks. It
> seems safer to sandwich the release between the two callback lists:
> after before_shmem_exit is done, but before dsm_backend_shutdown()
> runs.

I don't agree. You *cannot* rely on lwlocks working after an error before you
have called LWLockReleaseAll(). I.e. currently *any* use of lwlocks in
before_shmem_exit is unsafe. The only reason we haven't really noticed that is
that most of the top-level error handlers (i.e. sigsetjmp()s) do an
AbortCurrentTransaction(), which does an LWLockReleaseAll() if in a tx, and
most lwlock acquisitions happen within a transaction. But if you ever do stuff
outside of a transaction, the AbortCurrentTransaction() won't do
LWLockReleaseAll(), and you're in trouble, as the case here.

IOW, I think we need to do LWLockReleaseAll() at the top of proc_exit(), at
least in case of FATAL errors.

We probably should add a note to LWLockReleaseAll() indicating that we rely on
LWLockReleaseAll() working even if CreateLWLocks()/InitializeLWLocks() etc
hasn't yet been called...

Greetings,

Andres Freund

On Thu, Dec 4, 2025 at 12:33 AM Andres Freund <andres@anarazel.de> wrote:
> On 2025-12-02 13:10:29 +0900, Amit Langote wrote:
> > On Fri, Nov 21, 2025 at 8:45 PM Rahila Syed <rahilasyed90@gmail.com> wrote:
> > > If a process encounters a FATAL error after acquiring a dshash lock but before releasing it,
> > > and it is not within a transaction, it can lead to a segmentation fault.
> > >
> > > The FATAL error causes the backend to exit, triggering proc_exit() and similar functions.
> > > In the absence of a transaction, LWLockReleaseAll() is delayed until ProcKill. ProcKill is
> > > an on_shmem_exit callback, and dsm_backend_shutdown() is called before any
> > > on_shmem_exit callbacks are invoked.
> > > Consequently, if a dshash lock was acquired before the FATAL error occurred, the lock
> > > will only be released after dsm_backend_shutdown() detaches the DSM segment containing
> > > the lock, resulting in a segmentation fault.
> >
> > Thanks for the report.
> >
> > I am not super familiar with this code path, but this raises a broader
> > question for me: are there other resources residing in DSM (besides
> > LWLocks) that might be accessed during the shutdown sequence?
> >
> > We know dshash and dsa locks (LWLocks) face this risk because ProcKill
> > runs as an on_shmem_exit callback, which happens after
> > dsm_backend_shutdown() has already detached the memory.
> >
> > This patch fixes the specific case for LWLocks, but if there are any
> > other on_shmem_exit callbacks that attempt to touch DSM memory, they
> > would still trigger a similar segfault. Do we need to worry about
> > other cleanup routines, or is ProcKill the only consumer of DSM data
> > at this stage?
>
> I don't think it's really right to frame it as ProcKill() being a consumer of
> DSM data - it's just releasing all held lwlocks, and we happen to hold an
> lwlock inside a DSM in the problematic case...
>
> There are many other places that do LWLockReleaseAll()...

Sure, I was just wondering if there might be other stuff in these DSM
segment possibly being accessible from on_shmem_exit callbacks.  But,
maybe we don't have to address all such risks in this patch.

> > > Please find a reproducer attached. I have modified the test_dsm_registry module to create
> > > a background worker that does nothing but throws a FATAL error after acquiring the dshash lock.
> > > The reason this must be executed in the background worker is to ensure it runs without a transaction.
> > >
> > > To trigger the segmentation fault, apply the 0001-Reproducer* patch, run make install in the
> > > test_dsm_registry module, specify test_dsm_registry as shared_preload_libraries in postgresql.conf,
> > > and start the server.
> > >
> > > Please find attached a fix to call LWLockReleaseAll() early in the shmem_exit() routine. This ensures
> > > that the dshash lock is released before dsm_backend_shutdown() is called. This will  also ensure that
> > > any subsequent callbacks invoked in shmem_exit() will not fail to acquire any lock.
> >
> > @@ -229,6 +230,14 @@ shmem_exit(int code)
> >  {
> >     shmem_exit_inprogress = true;
> >
> > +   /*
> > +    * Make sure we release any pending locks so that any callbacks called
> > +    * subsequently do not fail to acquire any locks. This also fixes a seg
> > +    * fault due to releasing a dshash lock after the dsm segment containing
> > +    * the lock has been detached by dsm_backend_shutdown().
> > +    */
> > +   LWLockReleaseAll();
> > +
> >     /*
> >      * Call before_shmem_exit callbacks.
> >      *
> >
> > Again, not an expert, but I am concerned about placing
> > LWLockReleaseAll() at the very top, before before_shmem_exit()
> > callbacks run.
>
> I think it's actually kind of required for correctness, independent of this
> crash.  If we errored out while holding an lwlock, we cannot reliably run
> *any* code acquiring an lwlock, because lwlocks are not reentrant.
>
> > One of those callbacks might rely on locks being held or assume the
> > consistency of shared memory structures protected by those locks. It
> > seems safer to sandwich the release between the two callback lists:
> > after before_shmem_exit is done, but before dsm_backend_shutdown()
> > runs.
>
> I don't agree. You *cannot* rely on lwlocks working after an error before you
> have called LWLockReleaseAll(). I.e. currently *any* use of lwlocks in
> before_shmem_exit is unsafe. The only reason we haven't really noticed that is
> that most of the top-level error handlers (i.e. sigsetjmp()s) do an
> AbortCurrentTransaction(), which does an LWLockReleaseAll() if in a tx, and
> most lwlock acquisitions happen within a transaction. But if you ever do stuff
> outside of a transaction, the AbortCurrentTransaction() won't do
> LWLockReleaseAll(), and you're in trouble, as the case here.
>
> IOW, I think we need to do LWLockReleaseAll() at the top of proc_exit(), at
> least in case of FATAL errors.

Oh, so not at the top of not shmem_exit() as Rahila has proposed?

> We probably should add a note to LWLockReleaseAll() indicating that we rely on
> LWLockReleaseAll() working even if CreateLWLocks()/InitializeLWLocks() etc
> hasn't yet been called...

Makes sense.  AFAICS, num_held_lwlocks would be 0 in that case, so
LWLockReleaseAll() would be a no-op.

--
Thanks, Amit Langote

Hi,

On 2025-12-04 11:06:20 +0900, Amit Langote wrote:
> On Thu, Dec 4, 2025 at 12:33 AM Andres Freund <andres@anarazel.de> wrote:
> > I don't agree. You *cannot* rely on lwlocks working after an error before you
> > have called LWLockReleaseAll(). I.e. currently *any* use of lwlocks in
> > before_shmem_exit is unsafe. The only reason we haven't really noticed that is
> > that most of the top-level error handlers (i.e. sigsetjmp()s) do an
> > AbortCurrentTransaction(), which does an LWLockReleaseAll() if in a tx, and
> > most lwlock acquisitions happen within a transaction. But if you ever do stuff
> > outside of a transaction, the AbortCurrentTransaction() won't do
> > LWLockReleaseAll(), and you're in trouble, as the case here.
> >
> > IOW, I think we need to do LWLockReleaseAll() at the top of proc_exit(), at
> > least in case of FATAL errors.
> 
> Oh, so not at the top of not shmem_exit() as Rahila has proposed?

Oh, ontop of shmem_exit() seems fine, what I was trying to express was that it
should happen unconditionally at the start of exit processing, not just at the
tail.


> > We probably should add a note to LWLockReleaseAll() indicating that we rely on
> > LWLockReleaseAll() working even if CreateLWLocks()/InitializeLWLocks() etc
> > hasn't yet been called...
> 
> Makes sense.  AFAICS, num_held_lwlocks would be 0 in that case, so
> LWLockReleaseAll() would be a no-op.

Right. I just meant we should add a comment noting that we rely on that
fact...

Greetings,

Andres Freund

Hi,

On Fri, Dec 5, 2025 at 12:33 AM Andres Freund <andres@anarazel.de> wrote:
> On 2025-12-04 11:06:20 +0900, Amit Langote wrote:
> > On Thu, Dec 4, 2025 at 12:33 AM Andres Freund <andres@anarazel.de> wrote:
> > > I don't agree. You *cannot* rely on lwlocks working after an error before you
> > > have called LWLockReleaseAll(). I.e. currently *any* use of lwlocks in
> > > before_shmem_exit is unsafe. The only reason we haven't really noticed that is
> > > that most of the top-level error handlers (i.e. sigsetjmp()s) do an
> > > AbortCurrentTransaction(), which does an LWLockReleaseAll() if in a tx, and
> > > most lwlock acquisitions happen within a transaction. But if you ever do stuff
> > > outside of a transaction, the AbortCurrentTransaction() won't do
> > > LWLockReleaseAll(), and you're in trouble, as the case here.
> > >
> > > IOW, I think we need to do LWLockReleaseAll() at the top of proc_exit(), at
> > > least in case of FATAL errors.
> >
> > Oh, so not at the top of not shmem_exit() as Rahila has proposed?
>
> Oh, ontop of shmem_exit() seems fine, what I was trying to express was that it
> should happen unconditionally at the start of exit processing, not just at the
> tail.

Ah, ok.  I was talking about this with Rahila today and she pointed
out to me that whether we add it to the top of proc_exit() or
shmem_exit() doesn't really make any material difference to the fact
that it will get done at the start of exit processing as you say, at
least today.  So I think we can keep it like Rahila originally
proposed.

> > > We probably should add a note to LWLockReleaseAll() indicating that we rely on
> > > LWLockReleaseAll() working even if CreateLWLocks()/InitializeLWLocks() etc
> > > hasn't yet been called...
> >
> > Makes sense.  AFAICS, num_held_lwlocks would be 0 in that case, so
> > LWLockReleaseAll() would be a no-op.
>
> Right. I just meant we should add a comment noting that we rely on that
> fact...

Ok, got it.  Maybe like this:

@@ -1940,6 +1940,10 @@ LWLockReleaseClearVar(LWLock *lock,
pg_atomic_uint64 *valptr, uint64 val)
  * unchanged by this operation.  This is necessary since InterruptHoldoffCount
  * has been set to an appropriate level earlier in error recovery. We could
  * decrement it below zero if we allow it to drop for each released lock!
+ *
+ * Note that this function must be safe to call even if the LWLock subsystem
+ * hasn't been initialized (e.g., during early startup error recovery).
+ * In that case, num_held_lwlocks will be 0, and we'll do nothing.
  */
 void
 LWLockReleaseAll(void)

--
Thanks, Amit Langote