Nodereference performance issues

I think these have been mentioned before. I think #1 is a great idea, and we are not too late for a patch there. #2 looks good too though I'm not sure I fully grok it. In any case, we are not too late for performance fixes.

1) Proposed solution sounds OK, but this indeed needs to be a setting for noderef fields. Unanchored title match is precious in many cases.
Patch welcome :-)

2) I don't think I get your proposition either.
For noderef fields in 'standard mode' (refenrenceable nodes defined by node type checkboxes), it's true that validation could be smarter, we only need to get the node type of a given nid - simple query indeed.
For noderef fields in 'views' mode, you can't avoid building the query and retrieving the nodes.
I'll try to come up with smthing.

About 2) :
What do you think of this ?

subscribe - I'm having the same issues as well

CNW based on last comment.

Might be worth checking out #308215: Creates malformed SQL and does not scale (maybe it's just a plain dupe - I'm not where I can easily check this)

This saved performance for my large site. You are right that the LIMIT is even more important that the 'starts with' operator ... I did a code review as well and it looks good. You might uppercase 'Contains' and 'Starts with' in the #options but thats real nitpicky stuff. Well done.

subscribing

subscribing

Nice work !
Updated patch, with a few ediits / fixes :
- better namespacing for the 'match mode' constants - also start from 1, beacause 0 is painful with isset()s
- got rid of the _nodereference_translate_op() function, this does not really deserve to be abstracted out IMO
- renamed $op to $match, $op is used for 'à la hook_nodeapi' operators (you know, the ones that just got dumped in D7 :-) )
- I don't think we really want the 'exact title match' option available as a widget setting. Who would want that ?
- Preserve the setting when switching widget type back and forth.
- $limit was not working in Views mode
- validation failed if submitting without the [nid:x] part (JS disabled) in Views mode

Also (detail), I think the user-facing text in for the widget setting could be a little simplified IMO. 'Operator' is a little complex.

The only remaining gripe I have is that when validating a multiple noderef fields, the patch performs one query per nid to check. So while the patch makes it better for sites with many nodes, it also degrades it for all sites.

It would be cool to have _nodereference_potential_references() accept an array of $nids instead of a $nid.
Problem is that Views doesn't come with a filter for node.nid that lets us do that. We could add our own (we already do something like that for userref autocomplete), based on views_handler_filter_in_operator, but then it would also be available in Views UI, which we wouldn't want here.
Earl suggested we add that clause in the display plugin's query() method. Could be worth investigating

We'll also need to port this to userref, probably. I'd rather keep the code for those two modules as 'duplicate' as possible.

The code still needs some polishing, but the attached patch does as described above :
- move additional filtering clauses into direct add_where() calls in the plugin's query()
- handles arrays of nids

Still need to apply this to userref.

- Thks for the update.
"sanity check to catch old (1.x) $match parameters"
I'm not sure what you mean ?

- Same patch as #15, mirrored to userref as well. I changed $nids to $ids in the functions signatures to minimize diffs between the modules.
This raises the question of the NODEREFERENCE_MATCH_* constants. Both modules will use the same constants and values. Currently, userref uses the constants defined in noderef, but we don't want to have userref.module depend on noderef.module. Defining them in both modules sounds awkward. Maybe content.module should define them instead, just like it holds the views display plugin. Still thinking.

Previous patch was missing a ';'

Polished patch :
- About the constants issue mentioned in #16 : I actually dropped the constants in favor of string literals
- further unify bits of code in noderef and userref modules : non-views query builder in [node|user]reference_potential_references_standard()
- polish the Views plugin code as per Earl's advice on IRC
- proposed wording for the widget settings :

Autocomplete matching:
- Starts with
- Contains
Select the method used to collect autocomplete suggestions. Note that <em>Contains</em> can cause performance issues on sites with thousands of [nodes|users].

Suggestions / enhancements welcome.

This should be ready now. If no-one objects, I'll commit this shortly.

I had a quick read and the only parts that raised an eyebrow are $view->display_handler->set_option('use_pager', TRUE); and the plugin definition which also enables the pager. Is this needed? In typical views execution, pagers are very expensive on big sites since they trigger a count query. Perhaps thats not applicable here.

My bad, I added the pager stuff when trying to make $limit work in Views mode, and didn't realize it was not actually needed.

Committed. Thanks guys !

here is a quick fix patch for D5, in case anyone needs it.

Patch in #24 is not good.
It just makes sure the node exists, not that it is valid for the nodereference field (list of 'referenceable' nodes, defined on the field settings form)
If users enter a 'real' node name and skips autocompletion, the input will be accepted even if the node is not valid.

This is just a warning, not an invitation for someone to tackle a D5 backport. I'm personally not planning to spend time on checking / reviewing a D5 patch.

D5 Related:
yched, it fixes the memory "leak" which was completely taking down our server w/128mb of ram on node save. so, the patch may help someone even if it's not perfect. however, thanks for pointing that out. I didn't even realize a nodereference field would work if you just typed a name w/out the autocomplete [nid:###] bit.

I tested that patch under several scenarios and it seemed to work the same as w/out the patch. I did not test just typing in a node name w/out [nid:####]. Could you explain to me how a nodereference field is supposed to perform in that situation?

This patch fixes a major performance issue we experienced in 5.x originally and I would really like to see it in 5.x, too (partially, if necessary).

Here is an improved version of KingMoore's patch (#24) that does validates node type and uses a simple select statement instead of node_load

It ought to be possible to use the view to validate a selection, by retrieving the query from the view then adding a where clause for the node id(s), something like "node.nid IN(nnn, nnn)". It should work though I haven't tried it.

What about Drupal 6? Is this patch useful for D6 too?

CCK D6 has the changes already.

The suggested patch by andrewlevine contains a bug on r129. This condition is also FALSE for new nodereference fields, this inhibits you from creating new node reference fields.

Too old now, way past the time when we will add features to D5 version.