Skip to content
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

Refs #6148 -- Meta table api #5278

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
4 participants
@akaariai
Copy link
Member

commented Sep 12, 2015

The idea is that one can do fancy things (multi-schema setups, shadow table querying, injecting complex subqueries into ORM queries) by changing the Meta.db_table to respect the as_sql() API.

This is work in progress, and I do not target for 1.9 inclusion.

@charettes

This comment has been minimized.

Copy link
Member

commented Sep 13, 2015

This looks like a great approach at solving #6148!

def test_schema_qualified(self):
cursor = connection.cursor()
cursor.execute('create schema other_schema')
cursor.execute('create table other_schema.schema_qualified(id serial primary key, val text)')

This comment has been minimized.

Copy link
@charettes

charettes Sep 13, 2015

Member

Did you make any progress toward making it work with the migration framework?

This comment has been minimized.

Copy link
@akaariai

akaariai Sep 14, 2015

Author Member

No, and I don't intend to do that for the initial merge. Just creating tables doesn't require much changes to migrations. I guess the biggest problems are:

  • Creation of the schema itself
  • We need some support for db inspection (IIRC migrations check which tables exists)
  • What to do with SQLite which doesn't support schemas at all

Same table in multiple schemas requires a lot more changes, but I don't think we have to solve that one at all.

@akaariai akaariai force-pushed the akaariai:meta_table_api branch 2 times, most recently Oct 11, 2015

@akaariai

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2015

I'm pretty happy with the API. It is powerful enough to allow for all kinds of crazy things to be done with the ORM, yet nothing changes if you want to use Django as you always have used it.

We still need the ability for migrations to detect direct table_cls usage. I guess that isn't working yet. Then we need to document what parts of this we want to document. Keeping table_cls as private API is a possible solution for the initial implementation.

@akaariai

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2015

I'm going to need some pointers of how to test that migrations work when you use Meta.db_table = ModelTable('foobar') instead of Meta.db_table = 'foobar'. I can test that the state is recorded correctly in migrations.test_state, but where should I test that the db_table class instance is correctly serialized and deserialized?

@charettes

This comment has been minimized.

Copy link
Member

commented Oct 11, 2015

@akaariai I think those tests can be added to test_writer. While reviewing your changes I remembered about an issue where we only retrieve existing indexes and constraints from the public schema on PostgreSQL. I guess this should also be fixed as part of this commit?

@akaariai

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2015

I don't intend to do any schema support for migrations in this commit. I'm just going to test that if you use db_table = ModelTable('foobar') in your model definitions migrations aren't going to choke on it.

To get schema support for migrations, we need to do a couple of things. First, we need a way to create the actual schema, and also somehow record the fact we have created the schema. Second, we need to have enough capability in db inspection to support migrations (that is, check if a given table has been already created). Finally, we need to make database alteration support schema qualified table references. This actually requires a bit of work, as db_table is used in a lot of db operations (indexes, create/alter/drop table, references and so on). We might even need to support moving tables between schemas.

I don't see any of the above to be exceptionally hard to do. Only real design decision is how we record the fact we have already created a given schema.

@akaariai akaariai force-pushed the akaariai:meta_table_api branch Oct 23, 2015

@akaariai akaariai changed the title [WIP] Meta table api Meta table api Oct 23, 2015

@akaariai

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2015

I think this one is pretty near to being committable (we don't add any new public features in this commit, so no docs needed).

The one problem I have with this is around the hack in options.py and DEFAULT_NAMES handling. We want to store the db_table attribute to _db_table, and that causes a bit of mess in the code. Better ideas welcome!

@akaariai akaariai force-pushed the akaariai:meta_table_api branch to a38f1e1 Oct 24, 2015

@akaariai

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2015

I think this could be committed as is. If using model.Meta.table_cls, then the behaviour of migrations is undefined (I tested in a project,and you'll get an initial migration, but no state changes are detected when changing table_cls). I think this is OK, as the table_cls isn't public at this point.

To get full migrations support, we need to change AlterTableModel operation to be created when table_cls changes, or when db_table is deleted and table_cls is added. We also need to make AlterTableModel able to do the following operations:

  1. Set schema when changing from db_table to table_cls = SchemaQualified()
  2. Remove schema when removing a table_cls = SchemaQualified() (or just raise an error?)
  3. Alter table's schema when changing table_cls = SchemaQualified('foo', 'thetable') to table_cls = SchemaQualified('bar', 'thetable')
  4. Alter table's name when changing the table name with SchemaQualified.
  5. Ability to create (and perhaps also drop) schemas on demand. Or, addition of a manual CreateSchema operation.

@timgraham timgraham changed the title Meta table api Refs #6148 -- Meta table api Oct 24, 2015

@@ -336,9 +336,10 @@ def quote_name_unless_alias(self, name):
"""
if name in self.quote_cache:
return self.quote_cache[name]
if ((name in self.query.alias_map and name not in self.query.table_map) or
tables = [getattr(t, 'table', None) for t in self.query.table_map]

This comment has been minimized.

Copy link
@charettes

charettes Oct 26, 2015

Member

Nitpick but since tables order doesn't matter I would make it a set instead. It looks like it only used for in lookups.

@timgraham

This comment has been minimized.

Copy link
Member

commented Oct 26, 2015

I'd really like to break up the queries tests and models into multiple files. Do you have any advice about how to organize them before I give it a try? I don't mind rebasing this afterwards.

@akaariai

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2015

Yeah, I've often thought we need to split the queries tests. I don't know what should be the criteria. I'll try to come up with something.


def as_sql(self, compiler, connection):
# Note that it would be easy to change the schema using thread locals
# if wanted.

This comment has been minimized.

Copy link
@schinckel

schinckel Nov 9, 2015

Contributor

So the intent here is that we could have dynamic schema selection, based on some external factor? For instance, django-boardinghouse (my project) allows setting the schema as part of the session. Shared models/tables exist only in the public/main schema, and all others are in all client schemata.

This comment has been minimized.

Copy link
@akaariai

akaariai Nov 9, 2015

Author Member

Yes, just like the dynamic query class below uses a subquery when db_time is set, you could use different schemata using either threadlocals or query.context.

Notably we wont have official support for this for some time. In practice this means you'll have to do migrations manually.

This comment has been minimized.

Copy link
@schinckel

schinckel Nov 10, 2015

Contributor

Migrations are only a part of it (and I've been thinking of ways around the "set search_path, migrate, repeat" cycle, which really does take a long time with multiple schemata.

With this approach, we could at least rewrite the DDL query (or the DML query if it was a data migration, but that's even harder to deal with), and possibly make it faster.

I'm not currently writing production code against my solution anyway, so it's all about the future...


class DynamicQuery(models.ModelTable):
def as_sql(self, compiler, connection):
db_time = compiler.query.context.get('db_time')

This comment has been minimized.

Copy link
@schinckel

schinckel Nov 10, 2015

Contributor

So I'm not sure I understand where context comes from: sure, it's an attribute on sql.Query, but how do we add things to the context?

I was only able to find a couple of references to it in the source, and they weren't especially instructive.

This comment has been minimized.

Copy link
@akaariai

akaariai Nov 11, 2015

Author Member

It is currently private API. You are likely better off using threadlocals for passing the used schema.

@akaariai

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2015

I don't have time to work on the migrations side of schema qualified tables. The problem is that there is a lot of work for a lot of different databases.

I see three ways to handle this PR:

  1. Throw away what I have now.
  2. Include added changes pretty much as is. There won't be any migrations support, but the table_cls can be used later on to build migrations support.
  3. Find somebody else to take over the work.

My favorite is 2. Migrations support can be added later on, and the addition of table_cls shouldn't hurt any current users. We can even built migrations support incrementally.

@charettes

This comment has been minimized.

Copy link
Member

commented Dec 29, 2015

If we were to ship this feature without migration support should this be considered a private API or are you planing to document the ModelTable class?

@akaariai

This comment has been minimized.

Copy link
Member Author

commented Dec 30, 2015

We could document it with a mention that you must set the model as managed=False and do the migrations by hand. I guess this solution would be acceptable. If we document it, we might need migrations support for serializing and deserializing the table class. I have to try this with a test project.

You can also do "view-like" functionality with ._meta.table_cls, so the table_cls would be immediately useful in that case, too.

akaariai added some commits Sep 8, 2015

Refs #6148 -- implemented table_cls
The feature allows for using schema qualified table names. There is no
migrations support for this yet, though. With migrations support #6148
would be fixed.

Another compelling (but untested) use case is view-like features. That
is, you could have models that query from a subquery. The essence is
the ability to plug in raw SQL in to normal ORM queries. Something
like:

class Book(models.Model):
    title = models.TextField()

class Author(models.Model):
    age = models.IntegerField()
    book = models.ManyToManyField(Book)

class AverageAuthorAge(models.Model):
    book = models.OneToOneField(Book, primary_key=True)
    average_author_age = models.DecimalField()

    class Meta:
        table_cls = View(
    'select avg(age) as average_author_age, book_authors.book_id '
    '  from author inner join book_authors on '
    '       author.id = book_authors.author_id '
    ' group by book_id')

Currently, if using a table_cls in model.Meta, the behavior of
migrations framework is undefined.

@akaariai akaariai force-pushed the akaariai:meta_table_api branch from df2bd68 to e0b4f36 Dec 31, 2015

@akaariai

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2015

I've added support for serializing table_cls. It seems things are working as expected when using a test project. That is, you can use table_cls as long as managed = False.

@timgraham

This comment has been minimized.

Copy link
Member

commented Feb 3, 2016

Do you want to commit this without docs then? What about tests for the migrations portion?

@akaariai

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2016

I'm planning to get back to this eventually. I'll close this until I have time to work on this more.

@akaariai akaariai closed this Feb 4, 2016

@akaariai akaariai referenced this pull request Feb 19, 2016

Closed

Refs #6148 -- Meta table api #6162

1 of 6 tasks complete
@akaariai

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2016

I can't reopen PR after force-push. Reopened as #6162. There is now migrations support for PostgreSQL!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.