Send nextval queries to master and show queries to replicas for Postgres

[ankane]

Hey, big thanks for this gem! We're heavy users at Instacart. We've been running off a fork for a while, so wanted to try and get a few of the changes merged upstream. Here's the first of a few.

1. The nextval function in Postgres returns the next value in a sequence. Since it updates the sequence, it needs to be called on master. More info on sequences

2. show functions are okay to run on replicas.

Added as separate commits for each so you can cherry-pick if desired.

[bleonard]

Looks like currval and nextval and lastval are in other databases too. Seems ok to me to add those the abstract_adapter.

show seems to be already in the SQL_SKIP_STICKINESS_MATCHERS - not sure of our past reasoning about that. I'd be ok putting that in the abstract one too.

[ankane]

Hey @bleonard, thanks for the response. Happy to move them to abstract adapter.

When moving show, it looks like a few tests expect those queries to run on master. Any thoughts?

Failures:

  1) ActiveRecord::ConnectionAdapters::MakaraAbstractAdapter determines that "show fields" requires master
     Failure/Error: expect(proxy.master_for?(sql)).to eq(should_go_to_master)
     
       expected: true
            got: false
     
       (compared using ==)
     # ./spec/active_record/connection_adapters/makara_abstract_adapter_spec.rb:35:in `block (3 levels) in <top (required)>'

  2) ActiveRecord::ConnectionAdapters::MakaraAbstractAdapter determines that "show tables" requires master
     Failure/Error: expect(proxy.master_for?(sql)).to eq(should_go_to_master)
     
       expected: true
            got: false
     
       (compared using ==)
     # ./spec/active_record/connection_adapters/makara_abstract_adapter_spec.rb:35:in `block (3 levels) in <top (required)>'

  3) ActiveRecord::ConnectionAdapters::MakaraAbstractAdapter determines that "show index" requires master
     Failure/Error: expect(proxy.master_for?(sql)).to eq(should_go_to_master)
     
       expected: true
            got: false
     
       (compared using ==)
     # ./spec/active_record/connection_adapters/makara_abstract_adapter_spec.rb:35:in `block (3 levels) in <top (required)>'

[bleonard]

It looks like at some point, very early on, we decided that SHOW should go to master but not cause it to stick.

Does ActiveRecord do SHOW a lot? It seems possible it's to just get the schema and master would be good for that. Or does it do it all the time to make sure there is a connection or something?

Or maybe more interestingly, was SHOW being master-only a problem for you?

[ankane]

For Postgres, SHOW is only called when a connection is initialized to get the search_path and client_min_messages, not to get the schema.

We haven't had any Rails specific issues, just a very small amount of application code that calls SHOW (nothing critical).

[ankane]

Moved nextval to abstract adapter, and added currval and lastval. Also backed out the show change. I don't think it's needed after looking more at the use-cases.

[bleonard]

Thanks @ankane

[ankane]

Thank you. We tried 4 different libraries for sending reads to replicas, and Makara was the clear winner :)

[datibbaw]

We're currently using this hack to effect one of the fixes in this PR:

class ActiveRecord::ConnectionAdapters::MakaraAbstractAdapter
  def sql_master_matchers
    SQL_MASTER_MATCHERS + [/\A\s*select.+(nextval|currval|lastval)\(/i]
  end
end

Will there be a new release soon? :)