TEST_MIRROR setting doesn't work as expected (and has no tests)

[Ilya Baryshev]

TEST_MIRROR promises "connection to slave will be redirected to point at default. As a result, writes to default will appear on slave"
I've set up a minimum django project (using postgres backed) to demonstrate behavior.

    def test_fixture(self):
        MyModel.objects.using('default').create(name=1)
        MyModel.objects.using('slave').create(name=2)
        MyModel.objects.using('slave').create(name=3)
        self.assertEqual(list(map(repr, MyModel.objects.using('default').all())),
                         list(map(repr, MyModel.objects.using('slave').all())))

Both lists should be equal, because replica queries should be hitting default instead.
It appears not to be the case for Django>=1.4 up to latest 1.7.1 (but actually passes against 1.3.7)

AssertionError: Lists differ: ['<MyModel: 1>', '<MyModel: 2>... != ['<MyModel: 2>', '<MyModel: 3>...

First differing element 0:
<MyModel: 1>
<MyModel: 2>

First list contains 1 additional elements.
First extra element 2:
<MyModel: 3>

- ['<MyModel: 1>', '<MyModel: 2>', '<MyModel: 3>']
?             ^               ----------------

+ ['<MyModel: 2>', '<MyModel: 3>']
?             ^

Here is a project I used to test: ​https://github.com/coagulant/test_mirror

[Tim Graham]

It would be quite helpful if you could convert your test to one for Django's test suite and then bisect to determine the commit in Django that introduced the regression.

[Ilya Baryshev]

I've run git bisect, and first commit with regression happened to be: ​905e33f84a1a10e4f0183d879c52076ef876fc3b, which is related to #16885

Here is a log of my bisect: ​https://gist.github.com/coagulant/d04b3e729b851784a93a

[Tim Graham]

I haven't verified the issue, but I noticed there are no tests for TEST['MIRROR'] (renamed on master) so we should at least add some. Also the test in the "regression commit" seems to be gone -- not sure if its removal was intentional or not.

[Shai Berger]

The behavior in the bug description is sort-of expected, because of transactions (default and slave are the same database -- in the test project, they are defined this way even regardless of TEST_MIRROR -- but accessed through different connections, and so, separate transactions).

I'm saying sort-of expected because it seems the test is running in a transaction on default but not on slave, which is a little surprising -- but not the problem claimed.

I don't think we want to force test-cases to run each test in transactions on all databases -- I'm not sure that even makes sense; but we should probably document that tests using more than one database should be TransactionTestCases.

[harrykao]

Is it possible to have the mirrors share a connection? Using TransactionTestCase has the additional disadvantage of preventing multiprocessed test runs, which works fine when each test runs in a transaction.

[Erick Yellott]

I just ran into this as well. Based on the documentation, I expected to set

DATABASES = {
    'default': {
        ...
    },
    'replica': {
        ...
        'TEST_MIRROR': 'default', 
        # 'TEST': {'MIRROR': 'default'},  # I tried this too.
    }
}

and have my tests read from replica and write to default, which should be the same database and act as if I had just one database defined as default.

[Aymeric Augustin]

I just rediscovered that test mirrors require TransactionTestCase.

I'm wondering if it would be possible to share the database connection instead of having two connections with the same parameters.

If that doesn't work, we can document this limitation instead.

[Asser Schrøder Femø]

Just ran into this, but with raw queries instead. Using the connection directly does not work even with TransactionTestCase.

So for now I'm using this hacky workaround:

from django.db import connections

class ReplicationTestCase(TestCase):
    """
    Redirect raw queries to the replica, ie.

        from django.db import connections

        cursor = connections['replica'].cursor()
        cursor.execute(...)  # Is run directly on master in tests
    """
    def setUp(self):
        super(ReplicationTestCase, self).setUp()
        connections['replica']._orig_cursor = connections['replica'].cursor
        connections['replica'].cursor = connections['master'].cursor

    def tearDown(self):
        connections['replica'].cursor = connections['replica']._orig_cursor
        super(ReplicationTestCase, self).tearDown()

As an added benefit it works for model managers too even though I'm not using TransactionTestCase indicating that the connection might be able to be shared in a similar way?

[Kyle Agronick]

Is this ever going to be fixed? Setting the mirror should just copy the cursor so no matter where the slave DB is called it redirects to the master.

[Tim Graham]

It will be fixed when someone submits a patch and another person reviews it.

[eng. Ilian Iliev]

I have created a small patch, the PR is here ​https://github.com/django/django/pull/9603

The solution I went for is that when the MIRROR is specified then the mirror uses the same connection as the mirrored one.
If the tests look a bit redundant please let me know.

[Carlton Gibson]

Comments on PR

[eng. Ilian Iliev]

Patch has been improved - ​https://github.com/django/django/pull/9603

[Carlton Gibson]

Question from PR:

The PR currently uses mocks in the tests, but it's much simpler (for the current test and presumably into the future) and much clearer to just declare an extra replica alias with the TEST["MIRROR"] setting.

This second approach would require adjustments to test settings for Jenkins, the Django-Box VM and all devs' settings running against other DBs.

So the question is can we make that change?

[Simon Charette]

Planning to work on this since affecting our use of testing using TEST_MIRROR and TestCase.

I think the right approach here is to have mirror connections swapped to the connection object they mirror but only when using TestCase to allow proper transaction testing to take place. That's something the previous patch wasn't doing appropriately.

[José Pacheco]

My team at work also noticed an issue here which happens when you have a default + read_replica config, and you have a router that only selects a read_replica when considering particular models. If you have those conditions, and a data migration that uses both Job and User, you will have issues because the "default" connection config will be pointing to your test_myschema, whereas the read_replica config will still be pointing to myschema.

e.g.,

models.py:
Job: 
   user_id := FK(User)

DBRouter:
   db_for_write := 'read_replica' if model == Job else 'default'

settings:
   DATABASES = {
         'default': { ... },
         'read_replica': {
                # <configs>...
                'TEST': {'MIRROR': 'default'}
          }
    }


0001_migration.py:
    # ...
    for job in Job.objects.all():  # using apps.get_model()
        job.user.id  # will error out with __fake__.DoesNotExist

In the case above, if your "live" read_replica schema has any job data, it will try to run that last line, but there won't be any Users in the test_schema to match to.

Not sure if we're doing something wrong, but just letting you know that there are cases where a custom DBrouter could route to a config that should be a test mirror, but isn't.

Using python 3.6, Django 1.11.20, MySQL 5.7

[Rag Sagar]

@Simon Charette If you haven't started working on this, I would like to give this a try.

[Simon Charette]

@Rag Sagar.V, sure feel free to assign the ticket to yourself.

I'd start by having a look at ​the previous PR and make sure to understand the rationale behind why it got rejected.

For what it's worth I gave this ticket a 2-3 hours try by trying to point connections['mirror'] at connections['mirrored'] Python objects for the duration of TestCase and I got some failures related to transaction handling. I think ​Tim did a good job at nailing the issues with this approach. In short transaction handing is alias based so if something goes wrong with connections['mirrored'] then it's also surfaced in connections['mirror'] which differs from what would happen in a real life scenario.

The further I got from getting things working were by setting the isolation level on the miror/replica to READ UNCOMMITED for the duration of the test to allow it to see changes within the testcase transaction. This isolation level is not supported on all backends though (e.g. PostgreSQL) and was quite unreliable on MySQL from my minimal testing.

That one might be hard one if you're not familiar with how TestCase transactions wrapping takes place but you'll definitely learn a lot by giving this ticket a shot and seeing how the suite fails. Happy to review your changes if you get stuck on Github.

[Rag Sagar]

@Simon Charette Since READ_UNCOMMITED is not supported in postgres, Should I explore further in that direction? The solution that comes up on my mind is pointing mirror connections to mirrored for the duration of the TestCase. But that has the issues you pointed out. So I am bit confused which way I should go.

[Simon Charette]

Rag Sagar, I'm afraid none of the above solution will work appropriately.

Here's the problem:

If you configure mirrors to create a new connection to mirrored connection settings then changes performed within a transaction using the mirrored connection won't be visible in the mirror connection per READ COMMITED transaction isolation definition. That's the case right now.

Using READ UNCOMMITTED on the mirror connection seems to be a no-go from limited support and the fact MySQL seems inconsistent and reusing the same connection has confuses transaction management.

The only solutions I see going forward are:

1. Document this caveats of TEST_MIRROR and TestCase transaction wrapping interactions.
2. Invest more time trying to get the connections repointing during TestCase.setUpClass working to get the suite passing and document the new caveats.

In both cases we could at least do 1. or warn about it to make it clear this is not currently supported.

[Mariusz Felisiak]

#33153 was a duplicate.

[Mariusz Felisiak]

​PR

[Mariusz Felisiak]

#34193 was a duplicate.

[Sarah Boyce]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In d550e3c:

[4.1.x] Fixed #23718 -- Doc'd that test mirrors require TransactionTestCase.

Co-authored-by: Christian Bundy <me@…>

Backport of 0fbdb9784da915fce5dcc1fe82bac9b4785749e5 from main

[Mariusz Felisiak <felisiak.mariusz@…>]

In 0fbdb97:

Fixed #23718 -- Doc'd that test mirrors require TransactionTestCase.

Co-authored-by: Christian Bundy <me@…>