[Review Request] Added support for database delegated fields (like AutoField)

488 views
Skip to first unread message

Owais Lone

unread,
Jan 6, 2016, 10:04:51 AM1/6/16
to django-d...@googlegroups.com
Hi, I would really appreciate if someone could review the API design (and implementation) for a PR I created. The PR implements support for fields that are assigned a value by the database (using a default value or trigger) instead of Django. It also adds support for PostgreSQL’s RETURNING and Oracle’s RETURNING INTO clauses.

This will allow us to have AutoField like behaviour on fields other than the primary key or have primary key fields other than Integer and BigInteger types.

The PR also sets the stage for returning primary keys (and optionally other fields) from the bulk_create operation on PostgreSQL and Oracle.

Pull request: https://github.com/django/django/pull/5904
Django ticket: https://code.djangoproject.com/ticket/21454

Thanks

Tim Graham

unread,
Jan 22, 2016, 4:09:45 PM1/22/16
to Django developers (Contributions to Django itself)
It would be helpful if you could sketch out the proposal a bit more -- maybe write some documentation that explains and motivates the feature (and put explanation in this thread). The pull request description has some explanation but it doesn't seem suited toward someone unfamiliar with the background of the ticket which could explain the lack of response here.

Owais Lone

unread,
Jan 31, 2016, 9:55:57 AM1/31/16
to Django developers (Contributions to Django itself)
Tim, I assumed the explanation and discussion on  the ticket would be enough. I'll write it up if I find some time next week.

Thanks

Aymeric Augustin

unread,
Jan 31, 2016, 2:24:43 PM1/31/16
to django-d...@googlegroups.com
Hello Owais,

I had flagged this for review but it didn’t make it to the top of my list until today, sorry.

The current approach adds 7 new APIs that only target the use case exposed in the ticket, which I find rather narrow. Some of these APIs (ignore_delegated) only exist to work around the inflexibility of others. This doesn’t sound like the best possible design. I think there’s a more generally useful set of APIs that could make your use case possible, as well as others.

If I understood your PR correctly, you need five things, which I’m going to discuss below.


1) Ignore some fields on updates

Django already provides three ways to do this:

MyModel.objects.defer(‘ignored_1’, ‘ignored_2’).save()
MyModel.objects.only(‘included_1’, ‘included_2’).save()
MyModel.objects.save(update_fields=[‘included_1’, ‘included_2’])

(Of course, the second and third need something like included_fields = all_fields - excluded_fields.)

Writing a custom manager to add the `defer()` clause automatically sounds preferable to adding a fourth way of doing the same thing, with a `delegated_on_update` keyword argument on fields.

While we’re there, I don’t know if there’s a reason why `update_fields` can’t be replaced with `only()`. Both appear to achieve the same effet and `only()` is shorter. Could we deprecate `update_fields`?

Technically, the replacement for `.save(update_fields=fields)` would be `.only(*fields).save(force_update=True)` because `update_fields` forces an update. But I suppose most people won’t bother with the `force_update` argument if `.only()` does the right thing both on inserts and updates.


2) Ignore some fields on inserts

Since Django abstracts the insert/update distinction with its single API, `save()`, I’m reluctant to create insert_* / update_* APIs.

I don’t know how `defer()` and `only()` behave on inserts. If they don’t have the same effect as on updates, I think we could consider it a bug and fix it.

Combined with a deprecation of `update_fields` for the reasons explained above, this would leave just `.defer()` and `.only()`, a consistent and versatile API for controlling which fields are included or excluded by `.save()`.


3) Return some values on updates
4) Return some values on inserts

I think a QuerySet-level method similar to `.only()` and `.defer()` would work here and would be more generally useful. Exposing this capability for databases that support it should be uncontroversial.

I haven’t given it too much thought. Others probably have.


5) A reasonably DRY way to specify all this

I think you should implement a custom default manager. This is the generic API Django gives to make this use case and many others possible.


To sum up, I’m suggesting to remove one API instead of adding seven, to achieve pretty much the same result :-) I realize you already put a lot of work into the approach I’m discouraging. Sorry about that. I hadn’t noticed this ticket before.

In any case, a long-winded discussion that results in a massive API is a clear sign that we mustn’t add one keyword argument for each possible behavior. Instead we must give tools for developers to describe the behavior they need in code.


Best regards,

--
Aymeric.
> --
> 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 post to this group, send email to django-d...@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CE5C6CD8-146E-4680-89A7-1351DF0CA76F%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.

Shai Berger

unread,
Jan 31, 2016, 4:55:46 PM1/31/16
to django-d...@googlegroups.com
Hi Aymeric,

Your message seems to be confusing the queryset API with the model-instance
API. Details below.

On Sunday 31 January 2016 21:24:17 Aymeric Augustin wrote:
>
> 1) Ignore some fields on updates
>
> Django already provides three ways to do this:
>
> MyModel.objects.defer(‘ignored_1’, ‘ignored_2’).save()
> MyModel.objects.only(‘included_1’, ‘included_2’).save()
> MyModel.objects.save(update_fields=[‘included_1’, ‘included_2’])
>
> While we’re there, I don’t know if there’s a reason why `update_fields`
> can’t be replaced with `only()`. Both appear to achieve the same effet and
> `only()` is shorter. Could we deprecate `update_fields`?
>

only() is a queryset method. save() is a model method. There is actually no
MyModel.objects.save(), just MyModel.objects.only('field1').get(...).save().
Adding only() to model instances makes very little sense (is it supposed to
defer fields which have already been fetched?).

>
> 2) Ignore some fields on inserts
>
> Since Django abstracts the insert/update distinction with its single API,
> `save()`, I’m reluctant to create insert_* / update_* APIs.
>
> I don’t know how `defer()` and `only()` behave on inserts.

They don't. Since they only apply to objects fetched from the database, they
can only have any effect on updates (and I'm not even sure they do that).

WRT the original request: Owais, your API now seems to have gone a bit beyond
the needs detailed in the ticket description. In detail:

1. I think the separation between "delegate" and "return" in field options is
artificial (I find even the separation between "on insert" and "on update"
questionable). Django models always read (and write) all the fields against the
database, unless specifically told otherwise, and when told otherwise, it is on
a per-query basis, not in field definitions. You don't have a "deferred" flag on
any field, so it is odd to have an equivalent only for computed fields. Thus, of
your field optinos, I'd use only the "is_delegated" flag -- perhaps I'd allow it
to have, besides True and False, a value for "insert only" (I would have to
see an example where "update only" made sense before suggesting that too).

2. The name "delegated" is a bit odd. "db_computed" or "from_db" seem better
to me; "delegated" is just too abstract (doesn't say delegated to whom, and
there are many options).

3. Once the fields are marked as computed in the database, any attempt to set
them explicitly should either override the database calculation, or be an
error; regardless of the comments above on API for choosing the fields, your
example:

class MyModel(models.Model):
pytthon_ts = fields.DatetimeField()
db_ts = fields.DatetimeField(delegated=True)

# This will only update python_ts as db_ts is delegated to database
MyModel.objects.update(db_ts=now(), python_ts=now())

I wouldn't like that behavior; updating python_ts is sensible, erroring out is
sensible, quietly ignoring user input isn't. Also, disabling triggers on a
per-query basis is at best suspicious. As consequence from all these,
ignore_delegates() seems problematic.

4. This leaves us with a "hole" in the API: Current APIs let us pick fields to
retrieve when we're selecting (only() and defer()) and fields to save when
we're updating (save(update_fields=...)) but the new API gives us a way to
retrieve fields on saving, with no way to defer those. I'd handle that with a
new argument to save, say 'refresh_fields' -- if it is not given, all computed
fields are retrieved.

5. In very very general -- look at the use cases in the ticket. The first case,
database-defined defaults, was discussed on this list[1] and a different
solution was agreed upon (though I don't think it was implemented yet). It has
also been made a lot easier since the ticket was filed, because we now have
database functions which we didn't have back then. The other use-cases are for
fields whose values are always controlled in the database; so, you can make
this patch a lot simpler by dropping the option for a field to be updated from
both the database and the app.

Hope this helps,
(and like Aymeric, sorry I didn't find the time to react to this sooner),

Shai.


[1] https://groups.google.com/d/topic/django-developers/3mcro17Gb40/discussion

Aymeric Augustin

unread,
Feb 1, 2016, 3:33:26 AM2/1/16
to django-d...@googlegroups.com
> On 31 janv. 2016, at 22:55, Shai Berger <sh...@platonix.com> wrote:
>
> Your message seems to be confusing the queryset API with the model-instance
> API.

Oops :-(

Anyway, it seems that we agree on:

- controlling this behavior in the query rather than in the field definition — this avoids the awkward “ignore what the field says” query parameter
- trying not to provide separate APIs for insert and update behavior
- improving the save(update_fields=…) API to support inserts as well as updates

--
Aymeric.

Anssi Kääriäinen

unread,
Feb 2, 2016, 1:08:40 AM2/2/16
to Django developers (Contributions to Django itself)

For the .save() method we could go with a new argument fields=[...]. The way it works is that only the mentioned fields are inserted or updated. If you want the current behavior of update_fields, set force_update=True.

The main reason why update_fields patch didn't ever try to target insert statements was that the feature didn't make much sense. We didn't have database defaults and we didn't have any other sort of expressions support for inserts. There was very limited use cases for skipping fields in insert. I'm not sure we have much of an use case now either (notably one can write DbDefaultExpression that compiles to sql, params = "default", [] already). What is the main use case for this feature?

I've been toying around with an idea that we add a non-data descriptor (one implementing just __get__) for all fields of an model, and use this for deferred loading. Currently we use dynamically created subclasses of the model which is a hack. The way this would work is that if an instance's __dict__ doesn't have any value for given field, then we interpret that field as deferred. This would also give a natural way to refresh a given field from DB. You would set a field "deferred" manually by "del instance.field", and after that instance.field would reload that field. The descriptor wouldn't cost much, as non-data descriptors don't add any measurable overhead when the attribute is set on the instance's __dict__. I think the feature would be backwards compatible (though without trying it is hard to be sure).

This feature would also allow skipping fields on insert: instance = MyModel(); del instance.some_field; instance.save() -> some_field is skipped as it is deferred; instance.some_field -> some_field is refreshed from DB. This would close the gap in the APIs.

I would like to see support for expressions that can tell Django the field should be skipped in update/insert and if the field should be refreshed from database after update/insert. This would give a nice low-level API for manipulating the behavior of save() and bulk_insert(). I don't see a need to mix this with the current pull request. The biggest problem for such expressions is that they don't make any sense when using a model. For example TransactionNow() isn't a nice value for last_update field in any other context than saving. Not sure this is a big enough problem to worry, but something to consider anyways.

Refreshable expressions would give a natural way for skipping fields on insert. Also, things such as database defaults would be easy to use with the ORM:
   
    class DbDefault(Expression):
        refresh_on_insert = True
        refresh_on_update = True

        def as_sql(self, compiler, connection):
            return "default", []

    my_instance = MyInstance(uuid=DbDefault())
    print(my_instance.uuid) -> "DbDefault()"
    my_instance.save()
    print(my_instance.uuid) -> "f81d4fae-7dec-11d0-a765-00a0c91e6bf6"  (or whatever the DB happens to generate)

 - Anssi

Anssi Kääriäinen

unread,
Feb 3, 2016, 1:47:16 AM2/3/16
to Django developers (Contributions to Django itself)
The options for save() API seem to be:
  1. refresh_fields=[...] for save() - if set, then issue RETURNING for fields mentioned in the refresh_fields API.
  2. expressions: my_model.updated_at = TransactionNow(); my_model.save(), where TransactionNow is flagged to refresh the value from DB post-save.
  3. field flags like done in the PR.

Field flags are nice for things you always want to load from database (say, updated_at).

If the use cases we target are "generate value in db on insert" and "generate value in db on insert or update", and only those, then I think we can go with just a flag that tells Django to always skip the model's value on save(), and to reload from DB always.

We are going to override the assigned value on save(), but I think it has to be this way. Consider:
  a_object.save()  # a_object.updated_at is set to '2016-02-03' as it is database delegated
  a_object.save()  # a_object.updated_at has a value, so we should error if we don't ignore the value
forcing users to set the value to None pre-save breaks the whole field flag feature. We could go directly with expressions in that case. We could also use the value in the update and require the DB trigger to ignore the value.

We don't have a separate flag for doing the refresh on insert only, but that isn't IMO such a big problem. We are going to add a minor overhead to updating by refreshing a field value from the DB when it hasn't changed, but making the API simpler is worth this IMO.

So, my opinion is that for the field flags API we need just one flag. A name like db_generated comes to mind. This gives a nice way to support the most common use case - that of calculating an inserted_at/updated_at field directly in DB.

After we have the field flag API, I think we should aim for expressions support. For expressions we should have enough flags for allowing full control over save() behavior. So, if the db_generated field flag isn't enough for some use case, you can go low-level and use expressions directly. We could perhaps even allow db_generated value to be an expression - if so, that expression would be used on save() and bulk_create().

For the update_fields change, I think we can do that completely separately. The same goes for changing the way how deferred fields are implemented.

 - Anssi

Owais Lone

unread,
Feb 6, 2016, 6:24:32 AM2/6/16
to Django developers (Contributions to Django itself)
Shai and Ayeric, thank you so much for the feedback. The PR did indeed snowball into a much bigger one that I had initially planned. I agree with all points except,


> - controlling this behavior in the query rather than in the field definition — this avoids the awkward “ignore what the field says” query parameter

The main reason this PR exists is to let the fields decide if they should be updated/refreshed or not. If we don't define this behavior on the fields, then the feature will never work with 3rd party apps as developers will have to consciously remember to use some method on query API. For example, we could add a pguuid field to postgresql package that sets behaves like a normal uuid field but calculates the value using postgresql's uuid function. This would only work if the we define the preference on the field itself and the query API implicitly respects that.

The minimum public API we would need to make this happen is to add an option to the Field class, something like,

> id = models.IntegerField(db_computed=True)

`db_computed` would make django do 2 things, it would implicitly ignore the field when updating and inserting and it would auto fetch the value from the DB after every insert/update on supported backends. That's it. Everything else was added to make this a bit flexible, like to make a field behave like this only or INSERT or on UPDATE but I think even having just one param that does it for both insert and update would be awesome!

--
Owais

Podrigal, Aron

unread,
Feb 8, 2016, 1:32:10 AM2/8/16
to Django developers (Contributions to Django itself)

Like it has been discussed a while ago [1] about adding db_default, we should stick with something similar to that and support updates as well.

My 2 cents here.
I like the idea Anssi has proposed to delegate as much as possible using expressions. So I would propose similar to what discussed in the other thread using updated_at = DateTimeField(db_default=Expression, db_update=Expression) (I don't like the param name db_update but you got the idea).

Updating existing records. I would opt for leaving out fields which have a db_update parameter, but still allow to override this behavior by specifying those fields explicitly instance.save(update_fields=...). And the same for MyModel.objects.filter().update(**fields) we should not include so called auto fields in the update statement but we should never leave out a field explicitly specified, (we already have problems how to change pk values on existing records, and we don't want more behavior like that).

We should refresh values for records on insert and update using the RETURNING clause for those databases which support it. And defer fetching the new value for other databases and for update only, since for insert we already have to fetch the pk anyway, so we can fetch the other values as well. An interesting idea like Anssi proposed would be to allow refreshing values behavior be controlled via the Expression.

For queryset methods like bulk_create and update we can already control deferring values for fields using .defer() and .only(). So if one would really want to save the extra overhead fetching new values from db after update, this can be done by MyModel.objects.get(pk=instance.pk).update().

I don't like Field(db_computed=True) it adds very minimal control over how and what that does. 

To sum up:

1)  fields should have flags how they are used in insert / update queries.

2)  controlling behavior should be done via Expressions

3) Field api should add 2 attributes db_default and db_update (perhaps some better param name)

4) Do not limit overriding save behavior on a per query bases.


[1] https://groups.google.com/forum/#!msg/django-developers/3mcro17Gb40/NPINCcyyBAAJ


--
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 post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.

Anssi Kääriäinen

unread,
Feb 11, 2016, 4:51:59 AM2/11/16
to Django developers (Contributions to Django itself)
On Wednesday, February 3, 2016 at 8:47:16 AM UTC+2, Anssi Kääriäinen wrote:
For the update_fields change, I think we can do that completely separately. The same goes for changing the way how deferred fields are implemented.

I created a proof-of-concept pull request for improved deferred fields loading. See https://github.com/django/django/pull/6118. In short, it is faster and cleaner (no more dynamically created subclasses for deferred loading!) than the previous implementation, but we have a couple of backwards incompatibilities:
    1) Model.__init__() will be called with value DEFERRED for those fields which are deferred. Previously those fields were not given to __init__() at all.
    2) The class level attribute Model._deferred must be removed (we can't have class level attribute as we don't have dynamic classes any more)
    3) A potential name clash for fields when one has a class method and field with clashing names.

Of these the Model.__init__() case could be problematic for those users who have overridden __init__().

Before doing any further work on this we should decide if the Model.__init__() problem is bad enough to stop this cleanup, and if so, do anybody have any ideas of how to avoid the problem?

 - Anssi

Florian Apolloner

unread,
Feb 11, 2016, 7:38:44 AM2/11/16
to Django developers (Contributions to Django itself)


On Thursday, February 11, 2016 at 10:51:59 AM UTC+1, Anssi Kääriäinen wrote:
Before doing any further work on this we should decide if the Model.__init__() problem is bad enough to stop this cleanup, and if so, do anybody have any ideas of how to avoid the problem?

I do not think Model.__init__() is anywhere near public API, add it to the release notes and be done with it, worst case add a try/except around it…

Cheers,
Florian

Florian Apolloner

unread,
Feb 11, 2016, 7:41:44 AM2/11/16
to Django developers (Contributions to Django itself)
Oh, I somewhat missread and though there would be a new DEFERRED argument, the backwards issue is easy enough though:

 * Unless I miss something, YourModel.__init__ is Model.__init__ if the user didn't change it -> pass is DEFERRED
 * If the user changed it check for model._meta.new_style_deferred and continue accordingly
 * Raise a warning if the __init__ is a custom one and new_style_deffered is not set…

Florian Apolloner

unread,
Feb 11, 2016, 8:01:52 AM2/11/16
to Django developers (Contributions to Django itself)


On Thursday, February 11, 2016 at 1:41:44 PM UTC+1, Florian Apolloner wrote:
 * Unless I miss something, YourModel.__init__ is Model.__init__ if the user didn't change it -> pass is DEFERRED

Sadly this is not true, you can check if __init__ is in dict though…

Carl Meyer

unread,
Feb 11, 2016, 2:06:22 PM2/11/16
to django-d...@googlegroups.com
Hi Anssi,

On 02/11/2016 02:51 AM, Anssi Kääriäinen wrote:
> I created a proof-of-concept pull request for improved deferred fields
> loading. See https://github.com/django/django/pull/6118. In short, it is
> faster and cleaner (no more dynamically created subclasses for deferred
> loading!) than the previous implementation, but we have a couple of
> backwards incompatibilities:
> 1) |Model.__init__()| will be called with value DEFERRED for those
> fields which are deferred. Previously those fields were not given to
> |__init__()| at all.
> 2) The class level attribute Model._deferred must be removed (we
> can't have class level attribute as we don't have dynamic classes any more)
> 3) A potential name clash for fields when one has a class method and
> field with clashing names.
>
> Of these the Model.__init__() case could be problematic for those users
> who have overridden __init__().
>
> Before doing any further work on this we should decide if the
> Model.__init__() problem is bad enough to stop this cleanup, and if so,
> do anybody have any ideas of how to avoid the problem?

Overriding Model.__init__() is specifically called out in the
documentation as something to avoid, and there is no mention in the
documentation of what is passed to __init__() in the case of defer/only.
I think this is a fine change to just document in the release notes.

Carl

signature.asc

Anssi Kääriäinen

unread,
Feb 12, 2016, 7:32:11 AM2/12/16
to Django developers (Contributions to Django itself)
On Thursday, February 11, 2016 at 2:41:44 PM UTC+2, Florian Apolloner wrote:
Oh, I somewhat missread and though there would be a new DEFERRED argument, the backwards issue is easy enough though:

 * Unless I miss something, YourModel.__init__ is Model.__init__ if the user didn't change it -> pass is DEFERRED
 * If the user changed it check for model._meta.new_style_deferred and continue accordingly
 * Raise a warning if the __init__ is a custom one and new_style_deffered is not set…

If we are going to introduce a backwards compat system for this, then I think I want to get rid of calling Model.__init__ at all when loading from DB. We get faster model initialization, and conceptually loading from DB is like unpickling which (correctly) doesn't call __init__.

However, based on the comments in the PR, I think we are going to just document the change to __init__ and skip deprecation.

 - Anssi
 

Ben Cole

unread,
Nov 5, 2016, 11:17:31 AM11/5/16
to Django developers (Contributions to Django itself)
As discussed with many core team members (Simon Charette, Josh Smeaton, Marc Tamlyn, Tim Graham) at DUTH 2016 sprints, myself and Joachim Jablon have proposed a new, simpler solution to this. See https://code.djangoproject.com/ticket/27446

The proposal therefore is to add a `readonly` option to the base `Field` class that when `True` would strip these fields from being compiled to SQL during `INSERT`s and `UPDATE`s. This allows for a very simple change that covers all possible write queries that Django may perform (including bulk_*).
There exists a proof of concept https://github.com/novafloss/django-readonly-field

PR to follow soon...

Joachim Jablon

unread,
Nov 7, 2016, 7:44:52 AM11/7/16
to Django developers (Contributions to Django itself)
So the PR followed here : https://github.com/django/django/pull/7515

The PR is based on the idea defined by Ben just above.

It raised 2 questions which I'd like to bring to your attention :

1. When inserting or updating a model instance that contains one or more readonly fields, should we fetch the readonly fields values from the database (instance.refresh_from_db(fields=readonly_fields))
Pro : 
- Consistent with AutoFields
- Easier to work with
- Can be done without an extra query for PGSQL using RETURNING (but it's out of scope for this ticket. That being said, it could be done within the 1.11 scope)
Con :
- Intruduces an extra select query for models that have at least 1 readonly field.
- Can be easily done manually (especially if we provide a model instance such as ".refresh_readonly_fields()")
Alternative choice :
Give the choice to the user (in this case we still need to choose the default behaviour)

It's worth noting that the only affected models are those that include a "readonly" field, so that's not an extra select for everyone but only for the adopters of the new feature. It's also worth noting that the select is affecting a single element, using its pk, and targetting only the readonly fields.

2. For now this feature is named "readonly" and its default value is False. There was a suggestion to rather user the word "writable" and have its default value to True. I think both terms carry the same information.
Pro "readonly"
It's easier to name things in the code ("readonly fields" is a simpler concept than "non-writable fields" or "unwritable fields")
Pro "writable"
I'm not sure, but there were 3 :+1: on the GitHub comment so it's probably clearer

My own personal opinion: 1. refesh by default, add an argument to "model_instance.save()" to opt-out. 2. readonly

Aymeric Augustin

unread,
Nov 7, 2016, 9:35:38 AM11/7/16
to django-d...@googlegroups.com
On 7 Nov 2016, at 13:44, Joachim Jablon <ewjo...@gmail.com> wrote:

> My own personal opinion: 1. refesh by default, add an argument to "model_instance.save()" to opt-out. 2. readonly

I agree.

--
Aymeric.

Joachim Jablon

unread,
Nov 8, 2016, 7:59:28 AM11/8/16
to Django developers (Contributions to Django itself)
We were having a discussion on this matter on the Django under the Hood Slack channel, and there's a design decision that I think I can't take by myself.

There are 2 related subject : the "readonly" part and the "auto_refresh" part. Readonly means that the database won't try to write, autorefresh means that when creating /updating an instance, the value will be fetched from the database automatically

There was a debate on whether the readonly behaviour should imply auto_refresh, and there are cases that are good candidates for auto_refresh without readonly (autofields, serial fields etc.).

What I'd suggest, if we can get an agreement on this, would be to define those 2 behaviours completely independently (possibly as 2 different PRs, each with their tests and docs and such).

Auto_refresh could then be used for autofields with the quirk that some SQL DBs, will provide last_insert_rowid() (sqlite) or LAST_INSERT_ID() (mysql), while other use RETURNING (PGSQL / Oracle) allowing to use a single query.

Readonly, on its side, would only be a simple independent feature and its behaviour would do what the title says, no more, no less.

Do you think I need to do a DEP on this ?
If not, do you think it's ok if https://github.com/django/django/pull/7515 is changed to only do the readonly part, and we make another PR (and possibly ticket) for the "auto_refresh" part ?

While it would be nice to have both landing at the same time on master, I feel there's nothing blocking if they land at separate time, even in separate versions if need be (the freeze on Django 1.11 is coming fast, IIRC).

Are there usecases that I'm missing and that could not be expressed as a mix of readonly and auto_refresh ?

Marc Tamlyn

unread,
Nov 8, 2016, 10:32:48 AM11/8/16
to django-d...@googlegroups.com
I have fed a lot into this discussion in person and on Slack. I don't understand all the ins and outs completely, but I feel that the situation is sufficiently complex enough that we need to consider it all together. The code was certainly very challenging when I tried to make a UUID field use a DB generated value and "return properly" when used as a pk, hence the substandard docs for it. I'm personally -0 on the readonly patch unless it does fit into a genuine coherent design.

I personally believe we do need a DEP, and it needs to have the following attributes:
- Discussion of all the problems - refresh, read only, database level defaults, primary keys and the auto generated "id" field.
- Clear reference to all previous attempts to solve part(s) of the problem
- A clear plan as to how all the use cases will be solved by the proposed design
- A road map of how to build the pieces one ticket at a time. readonly may fit into this, it may not, that depends on the plan.

This will allow everyone, but especially the technical board, to understand all the considerations.

Hope that helps. I may be overcomplicating the problem, but I don't think I am.
Marc

--
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+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.

Ben Cole

unread,
Nov 8, 2016, 1:28:00 PM11/8/16
to Django developers (Contributions to Django itself)
This is what I wanted to know. I am willing to take on starting this DEP, with the caveat I've not done one before, and it will almost certainly require many revisions.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Reply all
Reply to author
Forward
0 new messages