Skip to content
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

perf: Perform db.set_value with single query only #18305

Merged
merged 2 commits into from Oct 6, 2022

Conversation

ankush
Copy link
Member

@ankush ankush commented Oct 6, 2022

Concludes #15560 by making all set_value calls to non-single doctypes run with 1 query only.

Explanation:

Previously we were fetching all the primary keys which need to be updated and then updating separately.

Before:

names = frappe.db.get_values(... filters)
frappe.qb.update(fields).where(table.name.isin(names))

After:

frappe.qb.update(fields).where(filters)

QB can already do single update query without much fuss. The last reason for keeping this appears to be for update which locks the rows before updating them.

Why remove for_update completely?

This was added to make sure that N+1 queries that happen don't cause inconsistencies during the time queries are ran. Now that set_value is 1-1 translated to single UPDATE query there's no point in locking rows before updating them. UPDATE locks rows automatically.

Before:
1.02 ms ± 166 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

After:
530 µs ± 144 µs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

Roughly 2x faster. This will be much better on queries that end up affecting more than 1 row.

TODO:

  • better cache eviction when dn is not a filter
  • evict only doctype (deferred, needs design change in cache keys)

ERPNext CI: https://github.com/frappe/erpnext/actions/runs/3197465411

@ankush ankush changed the title perf: single query db.set_value perf: Perform db.set_value with single query only Oct 6, 2022
- Only delete a single doc if we know which doc changed
- Drop all docs other wise (kinda bad, but this isn't used frequently,
  will fix when visiting entire caching system again)
@ankush ankush marked this pull request as ready for review October 6, 2022 13:15
@ankush ankush requested a review from a team as a code owner October 6, 2022 13:15
@ankush ankush requested review from shariquerik and removed request for a team October 6, 2022 13:15
@ankush
Copy link
Member Author

ankush commented Oct 6, 2022

...will delay backport by a week or so.

@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Merging #18305 (6436e67) into develop (6ed3ce5) will decrease coverage by 0.17%.
The diff coverage is 57.14%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #18305      +/-   ##
===========================================
- Coverage    62.72%   62.55%   -0.18%     
===========================================
  Files          746      746              
  Lines        67307    67709     +402     
  Branches      5966     5966              
===========================================
+ Hits         42218    42354     +136     
- Misses       21587    21853     +266     
  Partials      3502     3502              
Flag Coverage Δ
server-mariadb 66.78% <44.44%> (-0.06%) ⬇️
server-postgres 66.78% <44.44%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@ankush ankush merged commit bfa6a5f into frappe:develop Oct 6, 2022
@ankush ankush deleted the set_value_simple branch October 6, 2022 14:16
@ankush ankush added the backport version-14-hotfix backport to version 14 label Oct 10, 2022
ankush added a commit that referenced this pull request Oct 11, 2022
…#18349)

* perf: single query `db.set_value`

(cherry picked from commit cee2b50)

* fix: better cache validation

- Only delete a single doc if we know which doc changed
- Drop all docs other wise (kinda bad, but this isn't used frequently,
  will fix when visiting entire caching system again)

(cherry picked from commit bfa6a5f)

Co-authored-by: Ankush Menat <ankush@frappe.io>
frappe-pr-bot pushed a commit that referenced this pull request Oct 18, 2022
# [14.12.0](v14.11.1...v14.12.0) (2022-10-18)

### Bug Fixes

* add flags to set_permission for docshares ([#18416](#18416)) ([#18429](#18429)) ([a2ba0aa](a2ba0aa))
* correct import for markupsafe.escape ([c2f1c57](c2f1c57))
* **dx:** resolve_class ([#18417](#18417)) ([#18421](#18421)) ([c47de0d](c47de0d))
* explicitly set doctype in queries ([#18403](#18403)) ([#18405](#18405)) ([6848ee9](6848ee9))
* format currency value as float while grid upload ([5eb803c](5eb803c))
* google calendar sync times ([#18384](#18384)) ([#18394](#18394)) ([7c9ee6d](7c9ee6d))
* Notifications. Get Alerts for Today button [#18423](#18423) ([#18428](#18428)) ([63e3c61](63e3c61))
* progress bar not disappearing ([#18375](#18375)) ([#18392](#18392)) ([e0d6d0b](e0d6d0b))
* reshuffle __newname field to accommodate under 1st Tab Break ([#18406](#18406)) ([#18427](#18427)) ([f833e5d](f833e5d))
* Show error page even when routing fails ([#18437](#18437)) ([#18438](#18438)) ([5d3e97a](5d3e97a))
* **UI:** child table custom horizontal scroll ([ec09123](ec09123))
* **UI:** Child table responsive ([cb866f9](cb866f9))
* **UI:** consider 2px border at grid end ([c85fb2d](c85fb2d))
* **UI:** lesser margin if cell is focused ([f3ad63a](f3ad63a))

### Features

* allow syncing new fields in Doctype Layout ([#18408](#18408)) ([c73e2e2](c73e2e2))
* search in translated title, if we show title ([#17828](#17828)) ([#18395](#18395)) ([73758d7](73758d7))

### Performance Improvements

* Perform `db.set_value` with single query only (backport [#18305](#18305)) ([#18349](#18349)) ([23caa1f](23caa1f))
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant