Fix sequence name

[rpbaltazar]

Resolves #161
Resolves #81

• Copied @fsateler solution from the draft PR to this repo
• Added test coverage to the sceario of tenant switch within a block
• Updated specs to make use of Apartment::Tenant.adapter to avoid creating two instances of the adapter that wouldn't be in sync

[rpbaltazar]

@fsateler if you have the time, do you mind taking a look at this PR?

[fsateler]

I think the only dealbraker is that this would force the loading of pg adatper, which wouldn't exist on non-pg databases.

[fsateler]

This would force the loading of the pg adapter. Are other dbs no longer supported? This was my comment on #143 (comment)

[rpbaltazar]

Yes. I have not looked at how can we fix this and test it on a non postgresql db. I currently run only postgres so it's also hard for me to test these details on all dbs. I've been tempted on starting refactoring the gem to have one gem per adapter and a core, but that's discussion for another day.

[fsateler]

perhaps we can go the "easy" route:

if defined?(PG)
  require_relative 'apartment/active_record/postgresql_adapter'
end

Perhaps in an initializer so that this happens after all gems are loaded?

[rpbaltazar]

I think i have a clearer working solution. We only require the adapter once we eval the config. I'm requiring this patch after requiring the postgres adapter

[fsateler]

This causes extra work when default_tenant == current

[fsateler]

On second thought, this is not strictly correct, because it should happen even if the sequence name is prefixed with some other scheam. No idea if that can happen though 🤔

[rpbaltazar]

I thought of that when i was writing this and then forgot to address it. tks

[rpbaltazar]

I agree w your second concern. addressed it as well

[rpbaltazar]

@marksiemers and @citrus

I have been working on this with @fsateler and just remembered your request to be able to switch to multiple schemas:

• Allow a list of schemas when switching using schemas #112
• Allow a list of schemas when switching using schemas #154

The solution here assumes that the search path will resolve the table's sequence properly and therefore tries to remove the prefix.
I wonder what is the sequence name in your use cases with multiple tenants. Does it return the first schema name that it finds? I think that this path needs to be looked by one of you as I don't have a realistic use case scenario

[rpbaltazar]

I'll merge this one first. We can look at your specific use case after

[marksiemers]

@rpbaltazar - Thanks for calling this out. I don't work at the company anymore where we used Apartment, so I don't have the system/codebase in place that has the real-world use case. I can offer some info from the postgres docs.

Your question:

I wonder what is the sequence name in your use cases with multiple tenants. Does it return the first schema name that it finds? I think that this path needs to be looked by one of you as I don't have a realistic use case scenario

From docs:

The first schema named in the search path is called the current schema. Aside from being the first schema searched, it is also the schema in which new tables will be created
,,,
The first schema in the search path that exists is the default location for creating new objects. That is the reason that by default objects are created in the public schema. When objects are referenced in any other context without schema qualification (table modification, data modification, or query commands) the search path is traversed until a matching object is found.
,,,
The search path works in the same way for data type names, function names, and operator names as it does for table names.

Source: https://www.postgresql.org/docs/current/ddl-schemas.html#DDL-SCHEMAS-PATH

I assume this applies to sequences as well. The first named schema in the search path is where any new object will be created.

@ryanswood - Is this something you can check into or confirm?

[ryanswood]

The solution here assumes that the search path will resolve the table's sequence properly and therefore tries to remove the prefix. I wonder what is the sequence name in your use cases with multiple tenants. Does it return the first schema name that it finds? I think that this path needs to be looked by one of you as I don't have a realistic use case scenario

@rpbaltazar Thank you so much for remembering our request. I worked with @marksiemers and I am at still at the company where the multi-schema switch feature is still needed.

To answer your question, the value of sequence_name is dependent on the existence of the table in the schema and NOT the existence of the sequence. Postgres will scan the search path until it finds the first schema where the table exists and it returns the sequence name if it exists or nil if it does not exist for the found table.

It appears as if the override of ActiveRecord::ConnectionAdapters::PostgreSQLAdapter#default_sequence_name needs to account for the original's implementation when it returns nil. As it stands right now, there is a NoMethodError when the model is excluded from Apartment and res.sub! is called. Would you like me to research this further and submit a PR?

Quick background: We use the multi-schema switch feature for our data pipeline where we create a temporary schema that partially mirrors the original so that we can mass import data and hide it from UI. There are cases where the duplicate table in the temporary schema does not have its own sequence.