New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #27222 -- Refreshed fields that are expressions after Model.save(). #7244

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@holvianssi
Contributor

holvianssi commented Sep 14, 2016

@timgraham timgraham changed the title from Added expression refresh to save to Fixed #27222 -- Refreshed fields that are expressions after Model.save(). Sep 14, 2016

@charettes

This comment has been minimized.

Show comment
Hide comment
@charettes

charettes Sep 15, 2016

Member

As mentioned on the ticket this has a lot of overlap with #5904.

Member

charettes commented Sep 15, 2016

As mentioned on the ticket this has a lot of overlap with #5904.

@timgraham

Can you add a test for #23386? There's also documentation to update or remove in fc4b4fd. Also add a mention in the 1.11 release notes.

Show outdated Hide outdated tests/expressions/tests.py Outdated
Show outdated Hide outdated tests/expressions/tests.py Outdated
Fixed #27222 -- Made Model.save() refresh expressions.
Thanks Tim Graham for the docs.

I made these edits.

@charettes

This comment has been minimized.

Show comment
Hide comment
@charettes

charettes Nov 17, 2016

Member

Some discussion on field refreshing happening in #7515 made me wonder if we shouldn't simply mark these fields as deferred instead to avoid performing any query when not required. With the work you did refactoring deferred fields it should be easily possible.

Accessing them would still yield the database computed value on-demand and in the rare cases where multiple expression fields are saved() at the same time it would still be possible to use refresh_from_db(fields=[...]) to retrieve all of them in a single query instead of one per deferred fields.

Have you thought about this approach @holvianssi?

Member

charettes commented Nov 17, 2016

Some discussion on field refreshing happening in #7515 made me wonder if we shouldn't simply mark these fields as deferred instead to avoid performing any query when not required. With the work you did refactoring deferred fields it should be easily possible.

Accessing them would still yield the database computed value on-demand and in the rare cases where multiple expression fields are saved() at the same time it would still be possible to use refresh_from_db(fields=[...]) to retrieve all of them in a single query instead of one per deferred fields.

Have you thought about this approach @holvianssi?

@holvianssi

This comment has been minimized.

Show comment
Hide comment
@holvianssi

holvianssi Nov 17, 2016

Contributor

@charettes I think we should do the fetch for expressions directly in save(). Later on we are surely going to optimise this to use RETURNING on PostgreSQL, and then we are going to fetch the values directly in save. The only but is I'm not sure if there are cases where fetch inside save vs fetch deferred on first use matters, that is, does it matter at all for the end user?

It might be a bit complicated to fetch all of the deferred expressions on first access. Say, you have blob, updated_at and updated_by fields, where blob is deferred because it's large, and updated_at and updated_by are deferred because they were updated by expression. How can we easily check which fields to fetch when updated_at is assigned?

Contributor

holvianssi commented Nov 17, 2016

@charettes I think we should do the fetch for expressions directly in save(). Later on we are surely going to optimise this to use RETURNING on PostgreSQL, and then we are going to fetch the values directly in save. The only but is I'm not sure if there are cases where fetch inside save vs fetch deferred on first use matters, that is, does it matter at all for the end user?

It might be a bit complicated to fetch all of the deferred expressions on first access. Say, you have blob, updated_at and updated_by fields, where blob is deferred because it's large, and updated_at and updated_by are deferred because they were updated by expression. How can we easily check which fields to fetch when updated_at is assigned?

@charettes

This comment has been minimized.

Show comment
Hide comment
@charettes

charettes Nov 17, 2016

Member

@holvianssi thanks for you reply.

The only but is I'm not sure if there are cases where fetch inside save vs fetch deferred on first use matters, that is, does it matter at all for the end user?

I could take it to the mailing list if you'd like as I personally think it does. On database without RETURNING support I think this feature can be a footgun unless disabled by default. We should at least offer a way to disable it like we did with select_on_save.

It might be a bit complicated to fetch all of the deferred expressions on first access. Say, you have blob, updated_at and updated_by fields, where blob is deferred because it's large, and updated_at and updated_by are deferred because they were updated by expression. How can we easily check which fields to fetch when updated_at is assigned?

In the case where you'd like to only fetch updated_at accessing it will trigger the DefferedAttribute logic and fetch it in a single query. In the case you'd like to fetch both updated_at and updated_by in a single query while keeping blob deferred you'd have to do:

deferred_fields = {'updated_at', 'updated_by'} & set(instance.get_deferred_fields())
instance.refresh_from_db(fields=deferred_fields)

Which is an approach that is completely database agnostic, thus usable by third party apps, and won't perform any database query on backends with RETURNING support.

Other approaches could be to add an additional keyword argument to control this refresh to save() or implement deferred fields grouping like proposed by Adrian previously.

Member

charettes commented Nov 17, 2016

@holvianssi thanks for you reply.

The only but is I'm not sure if there are cases where fetch inside save vs fetch deferred on first use matters, that is, does it matter at all for the end user?

I could take it to the mailing list if you'd like as I personally think it does. On database without RETURNING support I think this feature can be a footgun unless disabled by default. We should at least offer a way to disable it like we did with select_on_save.

It might be a bit complicated to fetch all of the deferred expressions on first access. Say, you have blob, updated_at and updated_by fields, where blob is deferred because it's large, and updated_at and updated_by are deferred because they were updated by expression. How can we easily check which fields to fetch when updated_at is assigned?

In the case where you'd like to only fetch updated_at accessing it will trigger the DefferedAttribute logic and fetch it in a single query. In the case you'd like to fetch both updated_at and updated_by in a single query while keeping blob deferred you'd have to do:

deferred_fields = {'updated_at', 'updated_by'} & set(instance.get_deferred_fields())
instance.refresh_from_db(fields=deferred_fields)

Which is an approach that is completely database agnostic, thus usable by third party apps, and won't perform any database query on backends with RETURNING support.

Other approaches could be to add an additional keyword argument to control this refresh to save() or implement deferred fields grouping like proposed by Adrian previously.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jan 9, 2017

Member

I guess we'll close this for now since some design decisions are TBD.

Member

timgraham commented Jan 9, 2017

I guess we'll close this for now since some design decisions are TBD.

@timgraham timgraham closed this Jan 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment