On Thu, Jan 5, 2023 at 6:19 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Wed, Jan 04, 2023 at 10:12:19AM -0800, Nathan Bossart wrote:
> > From the discussion thus far, it sounds like the alternatives are to 1) add
> > a global flag that causes wal_retrieve_retry_interval to be bypassed for
> > all workers or to 2) add a hash map in the launcher and a
> > restart_immediately flag in each worker slot. I'll go ahead and create a
> > patch for 2 since it seems like the most complete solution, and we can
> > evaluate whether the complexity seems appropriate.
>
> Here is a first attempt at adding a hash table to the launcher and a
> restart_immediately flag in each worker slot. This provides a similar
> speedup to lowering wal_retrieve_retry_interval to 1ms. I've noted a
> couple of possible race conditions in comments, but none of them seemed
> particularly egregious. Ideally, we'd put the hash table in shared memory
> so that other backends could adjust it directly, but IIUC that requires it
> to be a fixed size, and the number of subscriptions is virtually unbounded.
>
True, if we want we can use dshash for this. The garbage collection
mechanism used in the patch seems odd to me as that will remove/add
entries to the hash table even when the corresponding subscription is
never dropped. Also, adding this garbage collection each time seems
like an overhead, especially for small values of
wal_retrieve_retry_interval and a large number of subscriptions.
Another point is immediately after cleaning the worker info, trying to
find it again seems of no use. In logicalrep_worker_launch(), using
both in_use and restart_immediately to find an unused slot doesn't
look neat to me, we could probably keep the in_use flag intact if we
want to reuse the worker. But again after freeing the worker, keeping
its associated slot allocated sounds odd to me.
--
With Regards,
Amit Kapila.