Issue: Better support for database routing
Author: foxx
State: closed
Created: 2015-09-04 13:42:34

As an extension from the discussions in #582 and #583, I'd like to propose a few changes. The implementation of #583 was rejected on the basis of design, as it would have required a connection manager, something which `peewee` does not handle by design (and rightly so).

Playhouse module [ReadSlave](https://github.com/coleifer/peewee/blob/master/playhouse/read_slave.py) implements routing by overloading `select()` and injecting the correct database as determined from `_get_read_database()`, which allows for basic routing of read queries.

However, implementing write queries is significantly harder because methods such as `create_or_get()` reference `cls._meta` directly. I've been able to work around this by extending `ReadSlaveModel` with a custom metaclass, which I've included in this post for future ref, but it's a dirty hack at best.

As such, I would like to propose an extension to `Model`;
- Add two new methods on `Model` called `get_read_db()` and `get_write_db()`. Both of which return  `cls._meta.database` by default, ensuring backwards compatibility
- Modify anywhere which uses `cls._meta.database` to use these methods instead (e.g. `select()` uses `get_read_db()` and `create_or_get()` uses `get_write_db()` etc).

I've looked over the source and can't see any problems with implementing the above. It will ensure backwards compatibility whilst allowing developers to implemented true read/write routing.

I would be happy to contribute time putting together a PR for this.

Thoughts?

``` python

class MetaOptionsMixin(object):
    """Late mix-in to provide simple database routing"""
    @property
    def database(self):
        """Return write db here"""
        return your.custom.db.stuff.here

class BaseModelMetaClass(peewee.BaseModel):
    """Add support for database routing"""

    # XXX: add proper database router support
    def __new__(cls, name, bases, attrs):
        """Add support for automatically registering models with app"""
        new_cls = super().__new__(cls, name, bases, attrs)
        new_cls._meta.db_name = new_cls._meta.database
        extend_instance(new_cls._meta, MetaOptionsMixin)
        return new_cls
    def selct

class Model(ReadSlaveModel, metaclass=BaseModelMetaClass):
    pass
```


Relevant https://github.com/coleifer/peewee/issues/689#issuecomment-133063492


> - Add two new methods on Model called get_read_db() and get_write_db(). Both of which return cls._meta.database by default, ensuring backwards compatibility
> - Modify anywhere which uses cls._meta.database to use these methods instead (e.g. select() uses get_read_db() and create_or_get() uses get_write_db() etc).

Good ideas, I will put this together.


Well, as I was getting into this, it looks to me like `get_read_database()` will be called a bit. I noticed in the "read slave" tests that the actual read-slave used was not predictable. This is because there may be multiple calls to `get_read_database()` over the course of a select query, each one of which performs the 'round robin' logic in the ReadSlaveModel. Having the calls isolated to the query classes was a way to work around this, but had other issues as you mentioned.

I'm not positive where this will end up, but I do agree it could be better.


I don't think this should be handled at the model level. This is a database adapter concern. In our case, we patched the `SelectQuery._execute()` to overcome this issue.

As I said previously, all our problems are related to the way connections are handled right now (and specifically because we're trying to keep them in a state per model). I think we should all take a fresh look at this and come up with a better API to handle all the use-cases.


> I don't think this should be handled at the model level. This is a database adapter concern.

@stas, the more I was looking at what the changes would look like, the more I agree with you. Specifically, the scope of the chosen db should be a transaction, in my mind.


@coleifer I would be happy to help with the postgres parts. Including the read replicas support. I could draft something so you can take a look, just let me know.


After consideration (and several reply rewrites!), I would agree that the query database persistency should probably be via a transaction, as this is more explicit. Am I correct in saying that `get_read_database/get_write_database` on the model are still required? 

Also happy to assist with testing/review.


Just for what its worth,you can use `Using` to specify a database to use
for a list of models, optionally within a transaction.

Scroll down a bit:

http://peewee.readthedocs.org/en/latest/peewee/database.html#using-multiple-databases
On Sep 14, 2015 5:35 AM, "Cal Leeming" notifications@github.com wrote:

> After consideration (and several reply rewrites!), I would agree that the
> query database persistency should probably be via a transaction, as this is
> more explicit. Am I correct in saying that
> get_read_database/get_write_database on the model are still required?
> 
> Also happy to assist with testing/review.
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/coleifer/peewee/issues/701#issuecomment-140031569.


> @stas, the more I was looking at what the changes would look like, the more I agree with you. Specifically, the scope of the chosen db should be a transaction, in my mind.

I can add another finding which should be taken into consideration while moving db out of model level.
Imagine you use server side cursor on one model and operate related one:

``` python
from playhouse import postgres_ext

for user in postgres_ext.ServerSide(User.select()):
     user.do_something_on_related_model(user.posts)
     user.number_of_posts = x
     user.save() 
```

Peewee will not be able to save changes to user instance as User's db is occupied by server side iteration and any changes to User model are "postponed" until for loop is actually finished.


That's another good point @alexanderad, and one I had not considered.


> Peewee will not be able to save changes to user instance as User's db is occupied by server side iteration and any changes to User model are "postponed" until for loop is actually finished.

The reason they are postponed may be because `ServerSide` runs in a transaction. I don't see anywhere in the code (been a while since I wrote it) that would modify the db itself for server-side cursors.


@stas -- have you tried the `Using` helper?


@coleifer nope, in our context we are working with a bunch of models for read-replicas. `Using` would just complicate things. We ended up patching and handling database routing around `SelectQuery._execute`. It sort of works, but it is still a pain because of the way pool handles the connections.


Not sure there's anything actionable here. Closing.


@coleifer From what I can see, the original proposal for `get_read_database` and `get_write_database` are still both outstanding and valid, and was waiting on you to either accept or reject. The other comments don't seem to give a clear conclusion, could you give your thoughts/findings please?

PS) Fwiw, I'm still willing to invest time into submitting a PR based on my original post


@foxx is right. The problems with the way peewee handles connection management is still a major headache.


Well, to my thinking, I figured that the `Using` context manager took care of most issues ([see docs](http://peewee.readthedocs.org/en/latest/peewee/api.html#Using)).

You could build higher-level APIs on top of it if that was your preference.

Regarding connection management, again I'm not sure what to do here.


@coleifer Ah okay, I'll have a try with `Using` and will update this issue if I run into any blockers. Thanks!