New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create query and dialect options for prepending schema search_path in Postgres #4534

Merged
merged 1 commit into from Sep 23, 2015

Conversation

3 participants
@mliszewski
Contributor

mliszewski commented Sep 17, 2015

When you have a multi-tenant application that has the same database models across multiple schemas the current sequelize model paradigm forces you to create a separate model for each schema that is identical except for the schema property of the tableName object. This change adds a dialect option called prependSearchPath that will allow you to pass a searchPath option to any query that will be prepend the query with "SET search_path to ". If the prependSearchPath options is enabled and no searchPath query option is provided it will use DEFAULT which should be the same behavior as before. By supporting a searchPath option I can now define a common set of models and use them across tenant schemas by passing the correct searchPath. If the prependSearchPath dialect option is not explicitly passed as true than the code works exactly as it did before.

On the surface this functionality would appear to be possible by using model.schema() but this does not work well when the same model needs to be used against multiple schemas concurrently in the same application as it changes the schema option on the model itself which can lead to inconsistent results. Specifically the following use cases do no work well.

  1. Create an instance with model.schema('a').build, create another instance with model.schema('b').build, try and save the first instance. The expected behavior would be that it would be saved in schema 'a' but it will actually be saved in schema 'b'.
  2. Since calling .schema() changes the schema option on the model itself this can lead to unexpected behavior if schema is not specified on every usage of the model. If i do model.schema('a').findAll and then later do model.findAll it will use whatever the last schema value that was set rather than a default schema. While this may be expected behavior since .schema() is essentially just a glorified setter of the schema option it is still dangerous and could lead to mixed tenant data.
  3. Even when chaining .schema() directly a small window exists where if you were calling .schema() on the same model concurrently you could get unexpected results.

describe('concurency tests', function() {
it('should build and persist instances to 2 schemas concurrently in any order', function() {
var Restaurant = this.Restaurant;

This comment has been minimized.

@mliszewski

mliszewski Sep 17, 2015

Contributor

@mickhansen This use case in particular fails when trying to use .schema() with the code below:

it('should but wont build and persist instances to 2 schemas concurrently in any order', function() {
          var Restaurant = this.Restaurant;

          var restaurauntModelSchema1 = Restaurant.schema('schema_one').build({bar: 'one.1'});
          var restaurauntModelSchema2 = Restaurant.schema('schema_two').build({bar: 'two.1'});

          return restaurauntModelSchema1.save()
            .then(function() {
              restaurauntModelSchema1 = Restaurant.schema('schema_one').build({bar: 'one.2'});
              return restaurauntModelSchema2.save();
            }).then(function() {
              return restaurauntModelSchema1.save();
            }).then(function() {
              return Restaurant.schema('schema_one').findAll();
            }).then(function(restaurantsOne) {
              expect(restaurantsOne).to.not.be.null;
              expect(restaurantsOne.length).to.equal(2);
              restaurantsOne.forEach(function(restaurant) {
                expect(restaurant.bar).to.contain('one');
              });
              return Restaurant.schema('schema_two').findAll();
            }).then(function(restaurantsTwo) {
              expect(restaurantsTwo).to.not.be.null;
              expect(restaurantsTwo.length).to.equal(1);
              restaurantsTwo.forEach(function(restaurant) {
                expect(restaurant.bar).to.contain('two');
              });
            });
        });
      });
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 18, 2015

.schema() should actually clone the model allowing for parallel use, if they don't there's definitely a bug/something missing about that (which thinking of it might be a known issue that was never handled)

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 18, 2015

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 18, 2015

Figured there was an existing issue @janmeier

@mliszewski

This comment has been minimized.

Contributor

mliszewski commented Sep 18, 2015

I agree that this is an issue with the .schema() functionality. The issue with cloning the model to set the schema is that you introduce a large memory problem. If you have a model that needs to be used across 1000s of schemas concurrently you are having to store than object in memory 1000s of times with that solution. This does not scale and is the reason we used the search_path approach which allows you to use one copy of the model across all schemas.

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 18, 2015

I don't believe that would be an issue. Each new schema'd model only contains the name of the schema, and a reference to its prototype

@mliszewski

This comment has been minimized.

Contributor

mliszewski commented Sep 18, 2015

OK I missed the part about using Object.create to 'clone'. This sounds like it would solve the problem assuming the created objects were GC'ed properly. If they were not you would still have a slow but steady memory leak.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 18, 2015

@mliszewski i'm still open to introducing search_path but we'd need a way to make sure it's opt-in (for adding 'DEFAULT' aswell).

@mliszewski

This comment has been minimized.

Contributor

mliszewski commented Sep 18, 2015

@mickhansen Sorry I didn't call this out better. I made the functionality opt-in in this versions by only prepending if the dialectOption 'prependSearchPath' is set to true. See https://github.com/mliszewski/sequelize/blob/wrap_search_path/lib/sequelize.js#L744

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 18, 2015

Oh jesus, i missed that @mliszewski, sorry about that.
Ideally we should make schemas work better and more performant, but for people who would infact have thousands of schemas additional stuff like this would be great.

@mliszewski

This comment has been minimized.

Contributor

mliszewski commented Sep 18, 2015

@mickhansen Can you rerun the tests please. They seem to have failed due to an issue with nvm.

@mliszewski

This comment has been minimized.

Contributor

mliszewski commented Sep 22, 2015

@mickhansen Can you please rerun the tests (failed due to nvm issues) and let me know if this looks good to get merge if tests pass?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 22, 2015

Merge conflict now, you will need to rebase (also an ideal time to squash your commits to a single descriptive commit)

@mliszewski mliszewski closed this Sep 22, 2015

@mliszewski mliszewski force-pushed the mliszewski:wrap_search_path branch from e210975 to bc9213f Sep 22, 2015

@mliszewski mliszewski reopened this Sep 22, 2015

@mliszewski mliszewski force-pushed the mliszewski:wrap_search_path branch from 127cb64 to 091e99d Sep 22, 2015

@mliszewski

This comment has been minimized.

Contributor

mliszewski commented Sep 23, 2015

@mickhansen Rebased, squashed commits and test passed

mickhansen added a commit that referenced this pull request Sep 23, 2015

Merge pull request #4534 from mliszewski/wrap_search_path
Create query and dialect options for prepending schema search_path in Postgres

@mickhansen mickhansen merged commit e6c42a4 into sequelize:master Sep 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

mickhansen added a commit that referenced this pull request Sep 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment