fix: odd behaviour seen when using REPLICA_DATABASE_URLS

[matthewelwell]

Thanks for submitting a PR! Please check the boxes below:

• I have run pre-commit to check linting
• I have added information to docs/ if required so people know about the feature!
• I have filled in the "Changes" section below?
• I have filled in the "How did you test this code" section below?
• I have used a Conventional Commit title for this Pull Request

Changes

Fixes #3681.

How did you test this code?

Ran the API locally with REPLICA_DATABASE_URLS set up to point to the same database as DATABASE_URL and observed that, without this change, I received an exception when trying to create a Feature.

Once I made this change, I no longer received the exception.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview		Apr 15, 2024 7:03pm
flagsmith-frontend-preview	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 15, 2024 7:03pm
flagsmith-frontend-staging	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 15, 2024 7:03pm

[matthewelwell]

@zachaysan @khvn26 I am requesting a review from both of you on this PR. Zach, because you have the context from our investigation of this issue. Kim, because you might have some context as to why history_instance.instance was used in the first place, rather than just instance.

[github-actions]

Uffizzi Preview deployment-50192 was deleted.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.91%. Comparing base (cc88dab) to head (1083c8e).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3771   +/-   ##
=======================================
  Coverage   95.91%   95.91%           
=======================================
  Files        1102     1102           
  Lines       34782    34782           
=======================================
  Hits        33362    33362           
  Misses       1420     1420           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zachaysan]

I'm happy to see this fixed, but we really should make a ticket for creating a test suite to run with a replica read database in addition to running the whole suite as just the default database. I know 99% of the time it would just duplicate the work, but the small part of the time to catch these regressions would really help out to find issues for our clients that use the replica databases.

[khvn26]

Approved but echoing Zach's proposal here. I'm sure we're going to bump into problems with replicated setups in the future, see https://andrewbrookins.com/python/scaling-django-with-postgres-read-replicas/ ("Solving Consistency Problems Caused By Replication Lag").

[matthewelwell]

@zachaysan @khvn26 yeah, you're both right for sure. I hoped it would be as simple as e.g.:

DATABASE_URL=postgres://postgres:[email protected]:5432/flagsmith \
 REPLICA_DATABASE_URLS=postgres://postgres:[email protected]:5432/flagsmith \
 pytest

but this causes all the tests to fail with a spurious error:

ERROR tests/integration/ core/test_commands.py::test_bootstrap__empty_instance__creates_expected - django.db.utils.ProgrammingError: column users_ffadminuser.organisation_id does not exist

I agree we should implement some testing against this, but it looks like it needs some thought.