Opened 5 years ago

Last modified 8 weeks ago

#23718 assigned Bug

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

Reported by: Ilya Baryshev Owned by: Rag Sagar
Component: Testing framework Version: 1.7
Severity: Normal Keywords: replica testing
Cc: Carlton Gibson, Patrick Cloke Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 (27)

comment:1 Changed 5 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 5 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: https://gist.github.com/coagulant/d04b3e729b851784a93a

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

comment:9 Changed 4 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 4 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 3 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 3 years ago by Tim Graham

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

comment:13 Changed 21 months ago by 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.

comment:14 Changed 21 months ago by eng. Ilian Iliev

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

comment:15 Changed 21 months ago by Asif Saifuddin Auvi

Has patch: set

comment:16 Changed 21 months ago by Carlton Gibson

Patch needs improvement: set

Comments on PR

comment:17 Changed 18 months ago by eng. Ilian Iliev

Patch has been improved - PR

Last edited 18 months ago by Carlton Gibson (previous) (diff)

comment:18 Changed 18 months ago by Tim Graham

Patch needs improvement: unset

comment:19 Changed 18 months 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 18 months ago by Carlton Gibson

Cc: Carlton Gibson added

comment:21 Changed 17 months ago by Tim Graham

Patch needs improvement: set

comment:22 Changed 4 months 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 3 months 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.

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

Last edited 3 months ago by José Pacheco (previous) (diff)

comment:24 Changed 3 months 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 3 months 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 3 months ago by Rag Sagar

Owner: changed from Simon Charette to Rag Sagar

comment:27 Changed 2 months 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 2 months 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 8 weeks ago by Patrick Cloke

Cc: Patrick Cloke added
Note: See TracTickets for help on using tickets.
Back to Top