@neyromant thanks for spotting this - you're right with your analysis of the bug.

The fix looks good overall (see some comments below). However, I'm a bit worried about adding more book-keeping inside PoolManager, specifically with respect to #3396. If we wanted to, we could also fix this by deduplicating the pools inside NpgsqlEventSource with a HashSet (though it's true this is better done once when adding pools, rather than every time).

@vonzshik what do you think?

PS I've opened #3480 to have an issue tracking this (not just a PR)

Thanks for the review, I made some edits.
I also like the idea with HashSet for pools in NpgsqlEventSource, but I probably have to wrap the addition in lock?

// NpgsqlEventSource:
// internal void PoolCreated() => Interlocked.Increment(ref _pools);
internal void PoolCreated (ConnectorPool pool)
{ 
  lock(Lock) 
   { 
     PoolsHashSet.Add(pool);
  }
} 

If lock is enough to solve the thread safety, I can do a different PR

If we wanted to, we could also fix this by deduplicating the pools inside NpgsqlEventSource with a HashSet (though it's true this is better done once when adding pools, rather than every time).

I'll all for this, as the pool manager is already not that simple. As a possible solution, we can set a flag every time we add or remove a pool to reset the HashSet.

I think actual addition/removal of pools should be rare enough that a simple lock around the HashSet should be fine - even after #3396 - do you agree @vonzshik?

BTW I'd keep the HashSet on the PoolManager rather than on the EventSource - it could be useful for other purposes (e.g. tests).

I think actual addition/removal of pools should be rare enough that a simple lock around the HashSet should be fine - even after #3396 - do you agree @vonzshik?

BTW I'd keep the HashSet on the PoolManager rather than on the EventSource - it could be useful for other purposes (e.g. tests).

I personally just don't like the idea of adding another array for the PoolManager to handle - for me it sounds like asking for more bugs. But if no one else has any objections, then I guess...

@vonzshik I'm OK either way. It should be OK on PoolManager - as long as we make it clear that the pools array/HashSet isn't reliable in the same sense as the one actually used when opening a connection. But am also OK with keeping it out.

@vonzshik @roji
I think that will not be able to use the HashSet directly - it is not thread safety. We could have used ConcurrentDictionary, but the fact is that getting Keys from it leads to increased memory traffic.
We rarely write to an array of pools, but we often read it in NpgEventSource.

I created a pr #3481 with an alternative approach - accounting for pools in NpgsqlEventSource.
There, as in the current review, there is no deduplication of pools as such. Everything is based on the correct logic in NpgsqlConnection.GetPoolAndSettings

Fixed in #3485