db_manager() method doesn't increment creation_counter

[Rhett Garber]

Creating a manager with the db_manager() method can cause unexpected behavior. Consider the following:

class MyModel(models.Model):
    objects = models.Manager()
    ro_objects = objects.db_manager('ro')

There are certain situations where ro_objects becomes the 'default' manager of the object. Both of these managers have the same 'creation_counter'.

Patch is here:
​https://github.com/rhettg/django/compare/db_manager_creation_counter?expand=1

[Markus Holtermann]

I looked through the documentation and none of the examples show this as a feature. I'd rather inherit from Manager and pass a db attribute along:

class ROManager(models.Manager):
    def __init__(self, db):
        super(ROManager, self).__init__(self)
        self._db = db

class MyModel(models.Model):
    objects = models.Manager()
    ro_objects = ROManager('ro')

Given that, maybe the BaseManager implementation could accept db and hints directly.

[Rhett Garber]

I think it does make sense to allow creation of a manager with db or hints directly rather than relying on the copy behavior.

However,

1. That's obviously a bit more involved since BaseManager doesn't currently take any arguments and sub-classing is a really popular pattern. It could conceivable cause problems for people.
2. That doesn't mean this patch isn't also correct. I don't think it's appropriate to have any 'copy' patterns that don't increment the creation_counter.

So maybe this patch is appropriate for backporting (since not incrementing the counter is a bug), while adding arguments to init would be a valid for a release?

[Loic Bistuer]

I'm sitting on the fence here.

db_manager() was designed to generate short-lived managers, not to be assigned as class managers. That said, other short-lived managers (and short-lived fields in custom lookups/expressions for that matter) increase the creation counters (e.g. Category().article_set) so db_manager() probably should too, if only for consistency.

However supporting non-empty db or hints attributes for long-lived managers is something that would require a fair amount of investigation and some hard design decisions:

• Are these supposed to be default values to be used when nothing else is provided at query time? In that case what about hints, should they be merged? Current implementation of RelatedManager.get_queryset() completely overrides hints.

• Are these supposed to be definitive values? Current implementation of RelatedManager.create|get_or_create|update_or_create() asks the router directly for db and doesn't account that one may be set already on the manager.

If we recognize that long-lived managers can have a db or hints state (as it would be the case if we made them __init__() arguments) then we must define the behavior. Right now setting these may work for some cases, but the behavior is largely undefined as far as Django is concerned.

[Rhett Garber]

Sorry I'm not quite following what is happening with this ticket.

Is the conclusion that my change is incorrect because incrementing the creation counter is not the desired behavior?