From: Tom Lane <tgl@sss.pgh.pa.us>
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Logrep launcher race conditions leading to slow tests
In-reply-to: 
 <CAExHW5s3=KJ9-J34dsiK-uXV9v5y7F=SD1-fBA8dekOF-0BbKw@mail.gmail.com>
References: <817604.1750723007@sss.pgh.pa.us>
 <CAExHW5s3=KJ9-J34dsiK-uXV9v5y7F=SD1-fBA8dekOF-0BbKw@mail.gmail.com>
Comments: In-reply-to Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
	message dated "Tue, 24 Jun 2025 17:20:10 +0530"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <957696.1750782780.1@sss.pgh.pa.us>
Content-Transfer-Encoding: 8bit
Date: Tue, 24 Jun 2025 12:33:00 -0400
Message-ID: <957697.1750782780@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/957697.1750782780%40sss.pgh.pa.us>
Precedence: bulk

Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
> On Tue, Jun 24, 2025 at 5:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 4. In process_syncing_tables_for_apply (the other caller of
>> logicalrep_worker_launch), it seems okay to ignore the
>> result of logicalrep_worker_launch, but I think it should
>> fill hentry->last_start_time before not after the call.
>> Otherwise we might be changing a hashtable entry that's
>> no longer relevant to this worker.

> A hash entry is associated with a table, not the worker. In case the
> worker fails to launch it records the time when worker launch for that
> table was attempted so that next attempt could be well-spaced in time.
> I am not able your last statement, what is the entry's relevance to
> the worker.

> But your change makes this code similar to ApplyLauncherMain(), which
> deals with subscriptions. +1 for the consistency.

Yeah, mainly I want to make it look more like ApplyLauncherMain().
It's true right now that nothing outside this process will touch that
hash table, so it doesn't matter which way we do it.  But if we were
to switch that table to being shared state, this'd be an unsafe order
of operations for the same reasons it'd be wrong to do it like that in
ApplyLauncherMain().

			regards, tom lane