Attached patch does the following:

Removes cache_clear_all() from node_save() and moves it to node_submit()
Stops deleting node access grants for nodes that don't even exist yet.

It does not duplicate the field cache clear changes from #757288: Optimize comment_save() which removes 10% of the queries. Just making a note of it here in case that other patch gets held up.

devel generate, 100 pages.
Before:
Executed 1046 queries in 1003.7 ms.

After:
Executed 842 queries in 519.24 ms.

I'm also eyeing the db_update() in there suspiciously, seems like we could use db_next_id() instead, not tried patching for that yet though.

CommentFileSizeAuthor
#12 pressflow6-optimize_node_save-757484-12-do-not-test.patch1.96 KBpdrake
#11 pressflow6-optimize_node_save-757484-11-do-not-test.patch1.89 KBpdrake
#10 d6-optimize_node_save-757484-10.patch1.87 KBpdrake
#6 node_grants.patch658 bytescatch
#3 node_save.patch2.35 KBcatch
node_save.patch2.37 KBcatch

Comments

catch’s picture

catch
he/him
Primary language English
commented

vid is a serial, so db_next_id() is pointless/not relevant, and db_last_insert_id() no longer exists in D7. So I'm sticking with this for now.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work 
+++ modules/node/node.module	30 Mar 2010 16:46:42 -0000
@@ -1072,15 +1072,14 @@ function node_save($node) {
+    $delete = $op == 'insert' ? TRUE : FALSE;

briefer: $delete = $op == 'insert'. no need for ternary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new node_save.patch2.35 KB

Ack of course. Fixed.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Code and logic look good.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Critical
Status: Fixed » Needs review
StatusFileSize
new node_grants.patch658 bytes

Slight whoopsie here, we only want to delete existing records if updating, not deleting. This would actually mess stuff up so bumping priority.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community
dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

pdrake’s picture

pdrake commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Needs review
StatusFileSize
new d6-optimize_node_save-757484-10.patch1.87 KB

Here is a backport of this patch to D6.

pdrake’s picture

pdrake commented
StatusFileSize
new pressflow6-optimize_node_save-757484-11-do-not-test.patch1.89 KB

The above patch is the correct patch for D6 - this one is only for those running pressflow and is posted here for reference/testing purposes.

pdrake’s picture

pdrake commented
StatusFileSize
new pressflow6-optimize_node_save-757484-12-do-not-test.patch1.96 KB

Patch for pressflow in #11 is bad. Here's a better pressflow patch.

mdupont’s picture

mdupont
Primary language French
Location Switzerland
commented
Priority: Critical » Normal

Nice but not critical

mdupont’s picture

mdupont
Primary language French
Location Switzerland
commented

I'm not sure about moving cache_clear_all() out of node_save(). There can be plenty of existing code that assumes node_save() is doing a cache_clear_all() and moving it to node_submit() may break it.

hefox’s picture

hefox commented

The cache clear all could conflict with any using node_save to save a single node outside of node submit

The grant access conflicts if acquire grants got called for whatever reason during 'insert' (Actually had some code doing it, one was a bug the other was due needing a draggable view to recalculate based on new node :/).

edit: forgot to finish first sentance

Praveen Sharma’s picture

Praveen Sharma commented

Removing cache_clear_all() increase the node_save time, but this impact the multisite installations.
Also, when the existing node is revised, old values might appear on the node view if cache_clear_all is removed.

pdrake’s picture

pdrake commented

@peeje, to be clear, cache_clear_all() is not completely removed in this patch, just moved from node_save to the node form submission handler, as in D7. Old values will not appear on the node view unless you are saving nodes in another manner and not clearing cache after doing so.

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.