Re: BUG #18031: Segmentation fault after deadlock within VACUUM's parallel worker

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