Avoid expensive tracking objects for prepared statements

[SamSaffron]

Per #36949 we introduce a race condition fix for #36763

This refines the fix to avoid using Concurrent::ThreadLocalVar

The implementation in the concurrent lib is rather expensive, culminating in
a finalizer per object that spins off a thread to do cleanup work.

None of this expense is needed as we can simply implement
the desired behavior using Ruby primitives. Additionally this moves to a Fiber bound implementation vs a thread bound implementation, something that is not desired for this particular usage.

cc @matthewd / @rafaelfranca / @97jaz

[kaspth]

Are we sure we need the extra instance variable versus something in this direction?

module PreparedStatementsRegistry
  extend ActiveSupport::PerThreadRegistry

  attr_accessor :enabled

  def disable
    old_enabled, self.enabled = self.enabled, false
    yield
  ensure
    self.enabled = old_enabled
  end
end

if self.class.type_cast_config_to_boolean(config.fetch(:prepared_statements) { true })
  PreparedStatementsRegistry.enabled = true
  @visitor.extend(DetermineIfPreparableVisitor)
else
  PreparedStatementsRegistry.enabled = true
end

def prepared_statements
  PreparedStatementsRegistry.enabled
end

def unprepared_statement(&block)
  PreparedStatementsRegistry.disable(&block)
end

[SamSaffron]

An interesting minor advantage of the proposal I made is that it is pretty much free if you are not using prepared statements, we don't even store anything in the thread local storage.

I am not sure per thread registry will work cause it is based off an object name per https://github.com/SamSaffron/rails/blob/f9e843b25efc35ecb47dc1bb4b2aac2fc7239c88/activesupport/lib/active_support/per_thread_registry.rb#L43-L43

So I am guessing what will happen is when two connections are on the same thread and control is yielded between 2 fibers, weird can happen.

Honestly I am not sure how people feel about allowing the same connection to be used at the same time from 2 threads, this is not something we ever practice at Discourse. The whole underlying premise of the original change is somewhat confusing to me.

[kaspth]

@SamSaffron good points! I revised the patch a little further and merged it in b9cc147. Thanks so much! 😊

[SamSaffron]

@kaspth thanks heaps, nice improvement using a Set there, it is a far better fit.

Any chance you can backport this to the next 6 release?

[rafaelfranca]

Backported in 46cc85d

[kaspth]

Thanks, I forgot to backport it ✌️