Re: subscription/015_stream sometimes breaks

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>
Cc: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: subscription/015_stream sometimes breaks
Date: 2023-08-23 08:00:56
Message-ID: 20230823080056.tbesqtabjcghzdfq@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2023-Aug-23, Zhijie Hou (Fujitsu) wrote:

> [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);
> }

Hmm, I think if worker->in_use is false, we shouldn't consult the rest
of the struct at all, so I propose to add the attached 0001 as a minimal
fix.

In fact, I'd go further and propose that if we do take that stance, then
we don't need clear out the contents of this struct at all, so let's
not. That's 0002.

And the reason 0002 does not remove the zeroing of ->proc is that the
tests gets stuck when I do that, and the reason for that looks to be
some shoddy coding in WaitForReplicationWorkerAttach, so I propose we
change that too, as in 0003.

Thoughts?

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

Attachment Content-Type Size
0001-Don-t-use-LogicalRepWorker-until-in_use-is-verified.patch text/x-diff 2.3 KB
0002-don-t-clean-up-unnecessarily.patch text/x-diff 868 bytes
0003-Don-t-rely-on-proc-being-NULL-either.patch text/x-diff 1.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2023-08-23 08:10:31 Re: initdb caching during tests
Previous Message Daniel Gustafsson 2023-08-23 08:00:13 Re: pg_upgrade - a function parameter shadows global 'new_cluster'