I'm also -1 on changing anything in Django right now. I think we should indeed take the "default position" for complex features: write a third party library, get some traction. If many people find it useful, we can look at adding something to core. It sounds like you're already working on such a library Aditya, so part of the way there.
On Mon, 31 May 2021 at 10:43, charettes <charett...@gmail.com> wrote: > Aditya, > > > "atomic does already call DB routers" -> Firstly after reading code, I > don't think the transaction APIs consult the routers. Secondly, I think I > already answered it in the initial discussion. > > Atomic doesn't consult it directly but the ORM does before interacting > with transactions. While it's true that the transaction is not entirely > bound to the writes if you're running Django in non-autocommit mode it's > not the default usage and even discouraged. When you *do* run Django in > auto-commit mode having db_for_read, db_for_write properly defined work > just fine without a need for this proposed hook assuming your application > code retrieves the proper database using db_for_write and use the return > value as atomic(using). That's what Django does internally and what I was > referring to in my message. > > > Also, if in the future, there is some special info that can be delivered > to the transaction based on which a decision regd which DB to use could be > made, then it would be cleanly isolated into its own method. > > I don't think this is a strong argument by any mean. To me this is > basically saying "it might be useful in the future so lets add it now" > without providing any details about why it might be useful in the first > place. > > > How can we take this forward ? > > You'll need to gather positive support for your proposal, I don't know > where Adam stands but I'm personally -1 on it. I'm still not convinced it's > worth adding for your detailed usage for the following reasons: > > 1. There's multiple existing multi-tenancy applications out there that are > relatively mature (I've even personally written one [1]) and none of them > expressed a need for such a hook[2]. From my experience adding a lot of > entries to DATABASES or using one schema per tenant only scales to a > certain point when it starts breaking badly. Django assumes `DATABASES` is > a static constant mapping, many unexpected and subtle things happen when > it's not the case (connections leak, KeyErrors) > 2. It's just not common enough to warrant an inclusion in Django core. > Every single added feature comes with a maintenance burden cost and risks > introducing bugs when interacting other existing features. This is > particularly true with niche features interacting with complex systems such > as ORM transaction handling. > > Best, > Simon > > [1] https://github.com/charettes/django-tenancy > [2] https://djangopackages.org/grids/g/multi-tenancy/ > > Le lundi 31 mai 2021 à 03:13:16 UTC-4, gojeta...@gmail.com a écrit : > >> Hey Adam/Simon, >> >> How can we take this forward ? >> >> Regards, >> Aditya N >> >> On Friday, May 28, 2021 at 3:04:14 PM UTC+5:30 N Aditya wrote: >> >>> Hey Adam, >>> >>> Also, after giving it a bit of thought, I figured that integrating this >>> logic with the routers framework isn't entirely necessary. >>> So I came up with another perspective to the solution as well which I've >>> illustrated in message-3 of this thread. >>> >>> Both approaches work for me. >>> >>> Let me know your thoughts on the same. >>> >>> Regards, >>> Aditya N >>> >>> On Friday, May 28, 2021 at 2:55:02 PM UTC+5:30 N Aditya wrote: >>> >>>> Hey Adam, >>>> >>>> "atomic does already call DB routers" -> Firstly after reading code, I >>>> don't think the transaction APIs consult the routers. Secondly, I think I >>>> already answered it in the initial discussion. >>>> >>>> FYI from message-1: >>>> >>>> >>>> 1. Having a separate method for transaction is good because it need >>>> not be mangled up with the other methods i.e db_for_read / db_for_write >>>> as >>>> they clearly indicate certain operational semantics which aren't tied >>>> to a >>>> transaction since it can have both reads and writes within it. >>>> 2. Also, if in the future, there is some special info that can be >>>> delivered to the transaction based on which a decision regd which DB to >>>> use >>>> could be made, then it would be cleanly isolated into its own method. >>>> >>>> Regards, >>>> Aditya N >>>> >>>> On Friday, May 28, 2021 at 2:03:16 PM UTC+5:30 Adam Johnson wrote: >>>> >>>>> Aditya - you didn't answer Simon's first question: "Can you think of >>>>> places where this db_for_transaction hook would differ in any way >>>>> from what db_for_write returns?" I think this is the crux of the >>>>> issue. atomic does already call DB routers - in what case could you need >>>>> to >>>>> return a different result for atomic() than for a write query? >>>>> >>>>> On Fri, 28 May 2021 at 09:17, N Aditya <gojeta...@gmail.com> wrote: >>>>> >>>>>> Hey Simon, >>>>>> >>>>>> It would be possible to write a method as you've suggested above. But >>>>>> it entails the following problems: >>>>>> 1. There would have to be a wrapper for each of the transaction APIs, >>>>>> not just transaction.atomic since all of them take the using kwarg. >>>>>> 2. I'm trying to build a library that helps achieve multi-tenancy >>>>>> (the isolated DB approach) by dynamically routing DB queries to the >>>>>> appropriate databases at runtime. For more info, check this out -> PyCon >>>>>> India 2021 proposal >>>>>> <https://in.pycon.org/cfp/2021/proposals/django-and-multi-tenancy-the-isolated-database-approach~dR6oO/>. >>>>>> If that be the case, for any new code base, I could expose a bunch of >>>>>> methods as suggested above and ask them to incorporate it. But for >>>>>> older/existing code bases which already directly use various transaction >>>>>> APIs, it would be hard to replace all lines with another method. >>>>>> >>>>>> By inspecting code, I figured that there is this method named >>>>>> get_connection in the transaction.py module which is being used by all >>>>>> the >>>>>> other transaction APIs for fetching the DB connection. Currently I've >>>>>> patched this method with my own to achieve the functionality mentioned >>>>>> above. Might I suggest an alternate implementation for this method as >>>>>> follows: >>>>>> >>>>>> #settings.py >>>>>> >>>>>> TRANSACTION_DB_SELECTOR = "path_to_some_callable" >>>>>> >>>>>> #transaction.py >>>>>> ... >>>>>> transaction_db_selector = >>>>>> import_string(settings.TRANSACTION_DB_SELECTOR) >>>>>> def get_connection(): >>>>>> if transaction_db_selector: >>>>>> using = transaction_db_selector() >>>>>> if using is None: >>>>>> using = DEFAULT_DB_ALIAS >>>>>> return connections[using] >>>>>> >>>>>> Let me know your thoughts on this. >>>>>> >>>>>> Regards, >>>>>> Aditya N >>>>>> >>>>>> On Friday, May 28, 2021 at 10:05:26 AM UTC+5:30 charettes wrote: >>>>>> >>>>>>> Ticket that directed to the mailing list for wider feedback >>>>>>> https://code.djangoproject.com/ticket/32788 >>>>>>> >>>>>>> --- >>>>>>> >>>>>>> Can you think of places where this db_for_transaction hook would >>>>>>> differ in any way from what db_for_write returns? That's what >>>>>>> Django uses internally in such instances >>>>>>> >>>>>>> 1. >>>>>>> >>>>>>> https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/base.py#L745-L760 >>>>>>> >>>>>>> <https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/base.py#L745-L760> >>>>>>> 2. >>>>>>> >>>>>>> https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/base.py#L950 >>>>>>> >>>>>>> <https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/base.py#L950> >>>>>>> 3. >>>>>>> >>>>>>> https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/deletion.py#L400 >>>>>>> >>>>>>> <https://github.com/django/django/blob/0d67481a6664a1e66d875eef59b96ed489060601/django/db/models/deletion.py#L400> >>>>>>> >>>>>>> I get that your asking for a way to avoid explicitly passing >>>>>>> atomic(using) all over the place but I'm having a hard time >>>>>>> figuring out in which cases a db_for_transaction hook, which cannot >>>>>>> be provided any contextual details like other router methods do, can >>>>>>> take >>>>>>> an an advised decision without relying on contextual/global state. >>>>>>> >>>>>>> Is there a reason you can't replace your wide spread internal uses >>>>>>> of atomic by something along these lines >>>>>>> >>>>>>> def routed_atomic(savepoint=True, durable=False): >>>>>>> >>>>>>> using = get_global_using() >>>>>>> >>>>>>> return transaction.atomic(using, savepoint=savepoint, >>>>>>> durable=durable) >>>>>>> >>>>>>> Cheers, >>>>>>> >>>>>>> Simon >>>>>>> Le jeudi 27 mai 2021 à 12:18:50 UTC-4, gojeta...@gmail.com a écrit : >>>>>>> >>>>>>>> From the Django docs, for any ORM query made, the DB alias to use >>>>>>>> is determined by the following rules: >>>>>>>> >>>>>>>> - Checks if the using keyword is used either as a parameter in >>>>>>>> the function call or chained with QuerySet. >>>>>>>> - Consults the DB routers in order until a match is found. >>>>>>>> - Falls back to the default router which always >>>>>>>> returns default as the alias. >>>>>>>> >>>>>>>> Using the router scheme works perfectly for ORM queries. However, >>>>>>>> when it comes to using transaction APIs (like the transaction.atomic >>>>>>>> construct), it becomes mandatory to specify the *using* kwarg >>>>>>>> explicitly in all of its publicly exposed APIs if a DB other than >>>>>>>> the default alias has to be chosen. >>>>>>>> >>>>>>>> To illustrate why this is a problem, consider the following >>>>>>>> scenario: >>>>>>>> >>>>>>>> - A DB router exists such that it directs queries to a specific >>>>>>>> database based on a value set in *thread-local* storage by a >>>>>>>> middleware. >>>>>>>> - When ORM queries from within the view are fired, they >>>>>>>> automatically get forwarded to the right DB *without explicitly >>>>>>>> citing* the using keyword owing to the router. >>>>>>>> - But if the transaction.atomic construct is used, >>>>>>>> the using keyword would have to be explicitly specified each time. >>>>>>>> While >>>>>>>> this might seem fine, it creates the following problems: >>>>>>>> 1. For large code bases, it becomes highly unwieldy to make >>>>>>>> sure that the *using* keyword has been mentioned in every >>>>>>>> transaction API call. Also, if one tries to implement the above >>>>>>>> scheme in >>>>>>>> an already existing code base, it would be impractical to add >>>>>>>> the using keyword in all lines calling the transaction APIs. >>>>>>>> 2. It doesn't gel well with the the routers framework. >>>>>>>> >>>>>>>> A proposed workaround could to be to add a separate method named >>>>>>>> *db_for_transaction* to the routers framework which would be >>>>>>>> consulted by the transaction APIs first, before falling back to using >>>>>>>> the default DB alias. >>>>>>>> >>>>>>>> >>>>>>>> 1. Having a separate method for transaction is good because it >>>>>>>> need not be mangled up with the other methods i.e db_for_read / >>>>>>>> db_for_write as they clearly indicate certain operational semantics >>>>>>>> which >>>>>>>> aren't tied to a transaction since it can have both reads and >>>>>>>> writes within >>>>>>>> it. >>>>>>>> 2. Also, if in the future, there is some special info that can >>>>>>>> be delivered to the transaction based on which a decision regd >>>>>>>> which DB to >>>>>>>> use could be made, then it would be cleanly isolated into its own >>>>>>>> method. >>>>>>>> >>>>>>>> >>>>>>>> If we can finalise on this, I could submit a PR for the same. >>>>>>>> >>>>>>> -- >>>>>> You received this message because you are subscribed to the Google >>>>>> Groups "Django developers (Contributions to Django itself)" group. >>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>> send an email to django-develop...@googlegroups.com. >>>>>> To view this discussion on the web visit >>>>>> https://groups.google.com/d/msgid/django-developers/001a5f54-d848-4398-8712-d0a068519f1an%40googlegroups.com >>>>>> <https://groups.google.com/d/msgid/django-developers/001a5f54-d848-4398-8712-d0a068519f1an%40googlegroups.com?utm_medium=email&utm_source=footer> >>>>>> . >>>>>> >>>>> -- > You received this message because you are subscribed to the Google Groups > "Django developers (Contributions to Django itself)" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to django-developers+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/django-developers/b85ceacd-9f3b-4880-b934-f631489fe1can%40googlegroups.com > <https://groups.google.com/d/msgid/django-developers/b85ceacd-9f3b-4880-b934-f631489fe1can%40googlegroups.com?utm_medium=email&utm_source=footer> > . > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMyDDM1at5y01tG2oVnFW0LfNZawoV%2BxEAeDHbECh5kF7qBYJg%40mail.gmail.com.
