On Thu, Jan 05, 2023 at 10:57:58AM +0530, Amit Kapila wrote:
> True, if we want we can use dshash for this.
I'll look into 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.
Yeah, I think this deserves a comment. We can remove anything beyond
wal_retrieve_retry_interval because the lack of a hash table entry is taken
to mean that we can start the worker immediately. There might be a corner
case when wal_retrieve_retry_interval is concurrently updated, in which
case we'll effectively use the previous value for the worker. That doesn't
seem too terrible to me.
It might be possible to remove this garbage collection completely if we use
dshash, but I haven't thought through that approach completely yet.
> 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.
Right.
> 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.
Yeah, this flag certainly feels hacky. With a shared hash table, we could
just have backends remove the last-start-time entry directly, and we
wouldn't need the flag.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com