fix_event_counters_2: another way for fix 3480#3481
fix_event_counters_2: another way for fix 3480#3481neyromant wants to merge 1 commit intonpgsql:mainfrom
Conversation
roji
left a comment
There was a problem hiding this comment.
I indeed like the approach of just using the existing logic in GetPoolAndSettings to determine when a new pool (actually) gets created.
One point re HashSet vs. array... This is a cold code path where performance isn't a concern: pools seldom get added (and should seldom get removed once #3396 is implemented), and the actual iteration in GetIdleConnection/GetBusyConnections happens once per second, at the very most.
So I'd prefer a simple HashSet protected by a simple lock, rather than this more complicated logic. Note that a lock is already being used when adding (because it may grow), and likely will be needed when pools are removed after #3396. Basically no reason not to keep it simple.
@vonzshik any thoughts?
vonzshik
left a comment
There was a problem hiding this comment.
I do like this more than the previous one. There is one place which I'm unsure of, but overall it does look good.
|
@roji About Hashset... In the GetIdleConnection / GetBusyConnections methods, we will have to iterate Hashset's items. Potentially, an addition / deletion to Hashset can occur at that moment. This will throw an exception or am I wrong? |
|
@neyromant, it will throw like other non concurrent collections (except arrays) because all of them has a version inside it in the iterator. If they ain't equal, the iterator throws. |
|
@neyromant yes, above I was proposing to protect all accesses to the hashset with a lock. Event counters are polled at most once per second, and contention is likely to be zero - so I don't see how this would add any overhead or perf issues. |
@roji I have created PR #3485 - using HashSet and lock. Should I close #3476 and #3481? |
|
@neyromant I think so, I think we're going down that way and just have to sort out the locking. |
|
Fixed in #3485 |
Alternative approach to fix #3480