Opened 9 years ago

Closed 11 months ago

Last modified 11 months ago

#23718 closed Bug (fixed)

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

Reported by: Ilya Baryshev Owned by: Sarah Boyce
Component: Documentation Version: 1.7
Severity: Normal Keywords: replica testing
Cc: Carlton Gibson, Patrick Cloke Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


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):
        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:

Change History (35)

comment:1 Changed 9 years ago by 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.

comment:2 Changed 9 years ago by 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:

comment:3 Changed 9 years ago by Tim Graham

Summary: TEST_MIRROR setting doesn't work as expectedTEST_MIRROR setting doesn't work as expected (and has no tests)
Triage Stage: UnreviewedAccepted

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.

comment:6 Changed 9 years ago by 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.

comment:7 Changed 9 years ago by 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.

comment:8 Changed 8 years ago by Erick Yellott

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

    '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.

comment:9 Changed 8 years ago by 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.

comment:10 Changed 8 years ago by 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?

comment:11 Changed 7 years ago by 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.

comment:12 Changed 7 years ago by Tim Graham

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

comment:13 Changed 6 years ago by eng. Ilian Iliev

I have created a small patch, the PR is here

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.

comment:14 Changed 6 years ago by eng. Ilian Iliev

Owner: changed from nobody to eng. Ilian Iliev
Status: newassigned

comment:15 Changed 6 years ago by Asif Saifuddin Auvi

Has patch: set

comment:16 Changed 6 years ago by Carlton Gibson

Patch needs improvement: set

Comments on PR

comment:17 Changed 6 years ago by eng. Ilian Iliev

Version 0, edited 6 years ago by eng. Ilian Iliev (next)

comment:18 Changed 6 years ago by Tim Graham

Patch needs improvement: unset

comment:19 Changed 6 years ago by 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?

comment:20 Changed 6 years ago by Carlton Gibson

Cc: Carlton Gibson added

comment:21 Changed 6 years ago by Tim Graham

Patch needs improvement: set

comment:22 Changed 4 years ago by Simon Charette

Owner: changed from eng. Ilian Iliev to 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.

comment:23 Changed 4 years ago by 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.

   user_id := FK(User)

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

         'default': { ... },
         'read_replica': {
                # <configs>...
                'TEST': {'MIRROR': 'default'}
    # ...
    for job in Job.objects.all():  # using apps.get_model()  # 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

Last edited 4 years ago by José Pacheco (previous) (diff)

comment:24 Changed 4 years ago by Rag Sagar

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

comment:25 Changed 4 years ago by 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.

comment:26 Changed 4 years ago by Rag Sagar

Owner: changed from Simon Charette to Rag Sagar

comment:27 Changed 4 years ago by 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.

comment:28 Changed 4 years ago by 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.

comment:29 Changed 4 years ago by Patrick Cloke

Cc: Patrick Cloke added

comment:30 Changed 2 years ago by Mariusz Felisiak

#33153 was a duplicate.

comment:31 Changed 2 years ago by Mariusz Felisiak

Component: Testing frameworkDocumentation
Owner: changed from Rag Sagar to Christian Bundy
Patch needs improvement: unset

comment:32 Changed 2 years ago by Mariusz Felisiak

Patch needs improvement: set

comment:33 Changed 12 months ago by Mariusz Felisiak

#34193 was a duplicate.

comment:34 Changed 11 months ago by Sarah Boyce

Owner: changed from Christian Bundy to Sarah Boyce
Patch needs improvement: unset

comment:35 Changed 11 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:36 Changed 11 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

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

comment:37 Changed 11 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 0fbdb97:

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

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

Note: See TracTickets for help on using tickets.
Back to Top