Feedback-ID: id4a34324:Fastmail
Date: Mon, 24 Jul 2023 20:39:46 -0700
From: Andres Freund <andres@anarazel.de>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18031: Segmentation fault after deadlock within VACUUM's
 parallel worker
Message-ID: <20230725033946.mstudugbav7yghui@awork3.anarazel.de>
References: <18031-ebe2d08cb405f6cc@postgresql.org>
 <CAD21AoDs7vzK7NErse7xTruqY-FXmM+3K00SdBtMcQhiRNkoeQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAD21AoDs7vzK7NErse7xTruqY-FXmM+3K00SdBtMcQhiRNkoeQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20230725033946.mstudugbav7yghui%40awork3.anarazel.de>
Precedence: bulk

Hi,

Thanks for finding / debugging this issue!

On 2023-07-21 17:01:11 +0900, Masahiko Sawada wrote:
> I've reproduced the issue in my environment with the provided script.
> The crashed process is not a parallel vacuum worker actually but a
> parallel worker for rebuilding the index. The scenario seems that when
> detecting a deadlock, the process removes itself from the wait queue
> by RemoveFromWaitQueue() (called by CheckDeadLock()), and then
> RemoveFromWaitQueue() is called again by LockErrorCleanup() while
> aborting the transaction. With commit 5764f611e, in
> RemoveFromWaitQueue() we remove the process from the wait queue using
> dclist_delete_from():
> 
>     /* Remove proc from lock's wait queue */
>     dclist_delete_from(&waitLock->waitProcs, &proc->links);
> :
>     /* Clean up the proc's own state, and pass it the ok/fail signal */
>     proc->waitLock = NULL;
>     proc->waitProcLock = NULL;
>     proc->waitStatus = PROC_WAIT_STATUS_ERROR;
> 
>  However, since dclist_delete_from() doesn't clear proc->links, in
> LockErrorCleanup(), dlist_node_is_detached() still returns false:
> 
>     if (!dlist_node_is_detached(&MyProc->links))
>     {
>         /* We could not have been granted the lock yet */
>         RemoveFromWaitQueue(MyProc, lockAwaited->hashcode);
>     }

Indeed :(


> leading to calling RemoveFromWaitQueue() again. I think we should use
> dclist_delete_from_thoroughly() instead. With the attached patch, the
> issue doesn't happen in my environment.

Yep. Do you want to push that fix, or should I?


> Another thing I noticed is that the Assert(waitLock) in
> RemoveFromWaitQueue() is useless actually, since we access *waitLock
> before that:
> 
> I think we should fix it as well. This fix is also included in the
> attached patch.

Don't really have an opinion on that. It's been this way for longer, afaict.


Greetings,

Andres Freund