Opened 10 years ago

Closed 17 months ago

Last modified 17 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

Description

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

Change History (35)

comment:1 by Tim Graham, 10 years ago

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 by Ilya Baryshev, 10 years ago

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

comment:3 by Tim Graham, 10 years ago

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 by Shai Berger, 9 years ago

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 by harrykao, 9 years ago

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 by Erick Yellott, 9 years ago

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.

comment:9 by Aymeric Augustin, 9 years ago

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 by Asser Schrøder Femø, 9 years ago

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 by Kyle Agronick, 7 years ago

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 by Tim Graham, 7 years ago

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

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

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.

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

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

comment:15 by Asif Saifuddin Auvi, 6 years ago

Has patch: set

comment:16 by Carlton Gibson, 6 years ago

Patch needs improvement: set

Comments on PR

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

Patch has been improved - PR

Last edited 6 years ago by Carlton Gibson (previous) (diff)

comment:18 by Tim Graham, 6 years ago

Patch needs improvement: unset

comment:19 by Carlton Gibson, 6 years ago

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 by Carlton Gibson, 6 years ago

Cc: Carlton Gibson added

comment:21 by Tim Graham, 6 years ago

Patch needs improvement: set

comment:22 by Simon Charette, 5 years ago

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 by José Pacheco, 5 years ago

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

Version 1, edited 5 years ago by José Pacheco (previous) (next) (diff)

comment:24 by Rag Sagar, 5 years ago

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

comment:25 by Simon Charette, 5 years ago

@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 by Rag Sagar, 5 years ago

Owner: changed from Simon Charette to Rag Sagar

comment:27 by Rag Sagar, 5 years ago

@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 by Simon Charette, 5 years ago

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 by Patrick Cloke, 5 years ago

Cc: Patrick Cloke added

comment:30 by Mariusz Felisiak, 3 years ago

#33153 was a duplicate.

comment:31 by Mariusz Felisiak, 3 years ago

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

comment:32 by Mariusz Felisiak, 3 years ago

Patch needs improvement: set

comment:33 by Mariusz Felisiak, 18 months ago

#34193 was a duplicate.

comment:34 by Sarah Boyce, 17 months ago

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

comment:35 by Mariusz Felisiak, 17 months ago

Triage Stage: AcceptedReady for checkin

comment:36 by Mariusz Felisiak <felisiak.mariusz@…>, 17 months ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 17 months ago

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