RE: subscription/015_stream sometimes breaks

> -----Original Message-----
> From: Zhijie Hou (Fujitsu) <houzj(dot)fnst(at)fujitsu(dot)com>
> Sent: Wednesday, August 23, 2023 10:27 AM
> To: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
> Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>; pgsql-hackers
> <pgsql-hackers(at)postgresql(dot)org>
> Subject: RE: subscription/015_stream sometimes breaks
>
> On Wednesday, August 23, 2023 4:55 AM Thomas Munro
> <thomas(dot)munro(at)gmail(dot)com> wrote:
> >
> > On Wed, Aug 23, 2023 at 8:21 AM Thomas Munro
> <thomas(dot)munro(at)gmail(dot)com>
> > wrote:
> > > I didn't study it closely but it looks like there might be a second
> > > deadlock, after the one that is expected by the test? Examples from
> > > the past couple of weeks:
> >
> > I should add, it's not correlated with the patches that cfbot is
> > testing, and it's the most frequent failure for which that is the case.
> >
> > suite | name | distinct_patches | errors
> > --------------+------------+------------------+--------
> > subscription | 015_stream | 47 | 61
> >
>
> Thanks for reporting !
> I am researching the failure and will share my analysis.

Hi,

After an off-list discussion with Amit, we figured out the reason.
From the crash log, I can see the apply worker crashed when accessing the
worker->proc, so I think it's because the work->proc has been released.

577: /* Now terminate the worker ... */
> 578: kill(worker->proc->pid, signo);
579:
580: /* ... and wait for it to die. */

Normally, this should not happen because we take a lock on LogicalRepWorkerLock
when shutting all the parallel workers[1] which can prevent concurrent worker
to free the worker info. But in logicalrep_worker_stop_internal(), when
stopping parallel worker #1, we will release the lock shortly. and at this
timing it's possible that another parallel worker #2 which reported an ERROR
will shutdown by itself and free the worker->proc. So when we try to stop that
parallel worker #2 in next round, we didn't realize it has been closed,
thus accessing invalid memory(worker->proc).

[1]--
LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);

workers = logicalrep_workers_find(MyLogicalRepWorker->subid, true);
foreach(lc, workers)
{
LogicalRepWorker *w = (LogicalRepWorker *) lfirst(lc);

** if (isParallelApplyWorker(w))
logicalrep_worker_stop_internal(w, SIGTERM);
}
--

The bug happens after commit 2a8b40e where isParallelApplyWorker() start to use
the worker->type to check but we forgot to reset the worker type at worker exit
time. So, even if the worker #2 has shutdown, the worker_type is still valid
and we try to stop it again.

Previously, the isParallelApplyWorker() used the worker->leader_pid which will
be reset when the worker exits, so the "if (isParallelApplyWorker(w))" won't pass
in this case and we don't try to stop the worker #2.

To fix it I think we need to reset the worker type at exit as well.
Attach the patch which does the same. I am also testing it locally
to see if there are other issues here.

Best Regards,
Hou Zhijie