On Tue, Jun 24, 2025 at 9:53 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > BTW, it strikes me that if we're going to leave > process_syncing_tables_for_apply() ignoring the result of > logicalrep_worker_launch, it'd be smart to insert an explicit > (void) cast to show that that's intentional. Otherwise Coverity > is likely to complain about how we're ignoring the result in > one place and not the other.
+1. On Tue, Jun 24, 2025 at 10:03 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Ashutosh Bapat <ashutosh.bapat....@gmail.com> writes: > > On Tue, Jun 24, 2025 at 5:26 AM Tom Lane <t...@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(). Makes sense to fix it proactively. -- Best Wishes, Ashutosh Bapat
