Index: common/database.h =================================================================== --- common/database.h (revision 12971) +++ common/database.h (working copy) @@ -412,6 +412,15 @@ virtual Xapian::Document::Internal * collect_document(Xapian::docid did) const; //@} + /** Notify the database that document is no longer valid. + * + * This is used to invalidate references to a document kept by a + * database for doing lazy updates. If we moved to using a weak_ptr + * instead we wouldn't need a special method for this, but it would + * involve a fair bit of reorganising of other parts of the code. + */ + virtual void invalidate_doc_object(Xapian::Document::Internal * obj) const; + ////////////////////////////////////////////////////////////////// // Introspection methods: // ====================== Index: common/document.h =================================================================== --- common/document.h (revision 12971) +++ common/document.h (working copy) @@ -49,7 +49,7 @@ Internal & operator=(const Internal &); /// The database this document is in. - const Xapian::Database::Internal *database; + Xapian::Internal::RefCntPtr database; bool data_here; mutable bool values_here; // FIXME mutable is a hack @@ -144,6 +144,24 @@ void need_values() const; void need_terms() const; + /** Return true if the data in the document may have been modified. + */ + bool data_modified() const { + return data_here; + } + + /** Return true if the values in the document may have been modified. + */ + bool values_modified() const { + return values_here; + } + + /** Return true if the terms in the document may have been modified. + */ + bool terms_modified() const { + return terms_here; + } + /** Get the docid which is associated with this document (if any). * * NB If multiple databases are being searched together, then this @@ -163,7 +181,8 @@ * In derived classes, this will typically be a private method, and * only be called by database objects of the corresponding type. */ - Internal(const Xapian::Database::Internal *database_, Xapian::docid did_) + Internal(Xapian::Internal::RefCntPtr database_, + Xapian::docid did_) : database(database_), data_here(false), values_here(false), terms_here(false), did(did_) { } @@ -176,7 +195,7 @@ * Note that the database object which created this document must * still exist at the time this is called. */ - virtual ~Internal() { } + virtual ~Internal(); }; #endif // OM_HGUARD_DOCUMENT_H Index: api/omdocument.cc =================================================================== --- api/omdocument.cc (revision 12971) +++ api/omdocument.cc (working copy) @@ -267,7 +267,7 @@ if (i == values.end()) return ""; return i->second; } - if (!database) return ""; + if (!database.get()) return ""; return do_get_value(valueid); } @@ -275,7 +275,7 @@ Xapian::Document::Internal::get_data() const { if (data_here) return data; - if (!database) return ""; + if (!database.get()) return ""; return do_get_data(); } @@ -293,7 +293,7 @@ if (terms_here) { RETURN(new MapTermList(terms.begin(), terms.end())); } - if (!database) return NULL; + if (!database.get()) RETURN(NULL); RETURN(database->open_term_list(did)); } @@ -410,7 +410,7 @@ { if (!terms_here) { // How equivalent is this line to the rest? - // return database ? database->open_term_list(did)->get_approx_size() : 0; + // return database.get() ? database->open_term_list(did)->get_approx_size() : 0; need_terms(); } Assert(terms_here); @@ -421,7 +421,7 @@ Xapian::Document::Internal::need_terms() const { if (terms_here) return; - if (database) { + if (database.get()) { Xapian::TermIterator t(database->open_term_list(did)); Xapian::TermIterator tend(NULL); for ( ; t != tend; ++t) { @@ -463,7 +463,7 @@ description += "terms[" + om_tostring(terms.size()) + "]"; } - if (database) { + if (database.get()) { if (data_here || values_here || terms_here) description += ", "; description += "doc="; description += "?"; // do_get_description(); ? @@ -478,10 +478,16 @@ Xapian::Document::Internal::need_values() const { if (!values_here) { - if (database) { + if (database.get()) { values = do_get_all_values(); value_nos.clear(); } values_here = true; } } + +Xapian::Document::Internal::~Internal() +{ + if (database.get()) + database->invalidate_doc_object(this); +} Index: backends/database.cc =================================================================== --- backends/database.cc (revision 12971) +++ backends/database.cc (working copy) @@ -477,6 +477,12 @@ return open_document(did, true); } +void +Database::Internal::invalidate_doc_object(Xapian::Document::Internal *) const +{ + // Do nothing, by default +} + RemoteDatabase * Database::Internal::as_remotedatabase() { Index: backends/flint/flint_database.h =================================================================== --- backends/flint/flint_database.h (revision 12971) +++ backends/flint/flint_database.h (working copy) @@ -277,6 +277,18 @@ /// If change_count reaches this threshold we automatically flush. Xapian::doccount flush_threshold; + /** A pointer to the last document which was returned by + * open_document(), or NULL if there is no such valid document. This + * is used purely for comparing with a supplied document to help with + * optimising replace_document. When the document internals are + * deleted, this pointer gets set to NULL. + */ + mutable Xapian::Document::Internal * modify_shortcut_document; + + /** The document ID for the last document returned by open_document(). + */ + mutable Xapian::docid modify_shortcut_docid; + /// Flush any unflushed postlist changes, but don't commit them. void flush_postlist_changes() const; @@ -302,6 +314,10 @@ #endif void delete_document(Xapian::docid did); void replace_document(Xapian::docid did, const Xapian::Document & document); + + Xapian::Document::Internal * open_document(Xapian::docid did, + bool lazy = false) const; + //@} public: @@ -340,6 +356,7 @@ void clear_synonyms(const string & word) const; void set_metadata(const string & key, const string & value); + void invalidate_doc_object(Xapian::Document::Internal * obj) const; //@} }; Index: backends/flint/flint_database.cc =================================================================== --- backends/flint/flint_database.cc (revision 12971) +++ backends/flint/flint_database.cc (working copy) @@ -663,7 +663,9 @@ doclens(), mod_plists(), change_count(0), - flush_threshold(0) + flush_threshold(0), + modify_shortcut_document(NULL), + modify_shortcut_docid(0) { DEBUGCALL(DB, void, "FlintWritableDatabase", dir << ", " << action << ", " << block_size); @@ -818,6 +820,13 @@ DEBUGCALL(DB, void, "FlintWritableDatabase::delete_document", did); Assert(did != 0); + if (rare(modify_shortcut_docid == did)) { + // The modify_shortcut document can't be used for a modification + // shortcut now, because it's been deleted! + modify_shortcut_document = NULL; + modify_shortcut_docid = 0; + } + // Remove the record. If this fails, just propagate the exception since // the state should still be consistent (most likely it's // DocNotFoundError). @@ -904,65 +913,74 @@ return; } - // OK, now add entries to remove the postings in the underlying record. - Xapian::Internal::RefCntPtr ptrtothis(this); - FlintTermList termlist(ptrtothis, did); - - termlist.next(); - while (!termlist.at_end()) { - string tname = termlist.get_termname(); - termcount wdf = termlist.get_wdf(); - - map >::iterator i; - i = freq_deltas.find(tname); - if (i == freq_deltas.end()) { - freq_deltas.insert(make_pair(tname, make_pair(-1, -termcount_diff(wdf)))); - } else { - --i->second.first; - i->second.second -= wdf; - } - - // Remove did from tname's postlist - map > >::iterator j; - j = mod_plists.find(tname); - if (j == mod_plists.end()) { - map > m; - j = mod_plists.insert(make_pair(tname, m)).first; - } - - map >::iterator k; - k = j->second.find(did); - if (k == j->second.end()) { - j->second.insert(make_pair(did, make_pair('D', 0u))); + // Check for a document read from this database being replaced - ie, a + // modification operation. + bool modifying = false; + if (modify_shortcut_docid && + document.internal->get_docid() == modify_shortcut_docid) { + if (document.internal.get() == modify_shortcut_document) { + // We have a docid, it matches, and the pointer matches, so we + // can skip modification of any data which hasn't been modified + // in the document. + modifying = true; + DEBUGLINE(DB, "Detected potential document modification shortcut."); } else { - // Modifying a document we added/modified since the last flush. - k->second = make_pair('D', 0u); + // The modify_shortcut document can't be used for a + // modification shortcut now, because it's about to be + // modified. + modify_shortcut_document = NULL; + modify_shortcut_docid = 0; } + } + + if (!modifying || document.internal->terms_modified()) { + // FIXME - in the case where there is overlap between the new + // termlist and the old termlist, it would be better to compare the + // two lists, and make the minimum set of modifications required. + // This would lead to smaller changesets for replication, and + // probably be faster overall. + + // First, add entries to remove the postings in the underlying record. + Xapian::Internal::RefCntPtr ptrtothis(this); + FlintTermList termlist(ptrtothis, did); termlist.next(); - } + while (!termlist.at_end()) { + string tname = termlist.get_termname(); + termcount wdf = termlist.get_wdf(); - total_length -= termlist.get_doclength(); + map >::iterator i; + i = freq_deltas.find(tname); + if (i == freq_deltas.end()) { + freq_deltas.insert(make_pair(tname, make_pair(-1, -termcount_diff(wdf)))); + } else { + --i->second.first; + i->second.second -= wdf; + } - // Replace the record - record_table.replace_record(document.get_data(), did); + // Remove did from tname's postlist + map > >::iterator j; + j = mod_plists.find(tname); + if (j == mod_plists.end()) { + map > m; + j = mod_plists.insert(make_pair(tname, m)).first; + } - // FIXME: we read the values delete them and then replace in case - // they come from where they're going! Better to ask Document - // nicely and shortcut in this case! - { - Xapian::ValueIterator value = document.values_begin(); - Xapian::ValueIterator value_end = document.values_end(); - string s; - value_table.encode_values(s, value, value_end); + map >::iterator k; + k = j->second.find(did); + if (k == j->second.end()) { + j->second.insert(make_pair(did, make_pair('D', 0u))); + } else { + // Modifying a document we added/modified since the last flush. + k->second = make_pair('D', 0u); + } - // Replace the values. - value_table.delete_all_values(did); - value_table.set_encoded_values(did, s); - } + termlist.next(); + } - flint_doclen_t new_doclen = 0; - { + total_length -= termlist.get_doclength(); + + flint_doclen_t new_doclen = 0; Xapian::TermIterator term = document.termlist_begin(); Xapian::TermIterator term_end = document.termlist_end(); for ( ; term != term_end; ++term) { @@ -1002,21 +1020,39 @@ PositionIterator it = term.positionlist_begin(); PositionIterator it_end = term.positionlist_end(); if (it != it_end) { - position_table.set_positionlist( - did, tname, it, it_end); + position_table.set_positionlist(did, tname, it, it_end); } else { position_table.delete_positionlist(did, tname); } } + DEBUGLINE(DB, "Calculated doclen for replacement document " << did << " as " << new_doclen); + + // Set the termlist + termlist_table.set_termlist(did, document, new_doclen); + + // Set the new document length + doclens[did] = new_doclen; + total_length += new_doclen; } - DEBUGLINE(DB, "Calculated doclen for replacement document " << did << " as " << new_doclen); - // Set the termlist - termlist_table.set_termlist(did, document, new_doclen); + if (!modifying || document.internal->data_modified()) { + // Replace the record + record_table.replace_record(document.get_data(), did); + } - // Set the new document length - doclens[did] = new_doclen; - total_length += new_doclen; + if (!modifying || document.internal->values_modified()) { + // FIXME: we read the values delete them and then replace in case + // they come from where they're going! Better to ask Document + // nicely and shortcut in this case! + Xapian::ValueIterator value = document.values_begin(); + Xapian::ValueIterator value_end = document.values_end(); + string s; + value_table.encode_values(s, value, value_end); + + // Replace the values. + value_table.delete_all_values(did); + value_table.set_encoded_values(did, s); + } } catch (const Xapian::DocNotFoundError &) { (void)add_document_(did, document); return; @@ -1035,6 +1071,18 @@ } } +Xapian::Document::Internal * +FlintWritableDatabase::open_document(Xapian::docid did, bool lazy) const +{ + DEBUGCALL(DB, Xapian::Document::Internal *, "FlintWritableDatabase::open_document", + did << ", " << lazy); + modify_shortcut_document = FlintDatabase::open_document(did, lazy); + // Store the docid only after open_document() successfully returns, so an + // attempt to open a missing document doesn't overwrite this. + modify_shortcut_docid = did; + RETURN(modify_shortcut_document); +} + Xapian::doclength FlintWritableDatabase::get_doclength(Xapian::docid did) const { @@ -1183,3 +1231,12 @@ postlist_table.add(btree_key, value); } } + +void +FlintWritableDatabase::invalidate_doc_object(Xapian::Document::Internal * obj) const +{ + if (obj == modify_shortcut_document) { + modify_shortcut_document = NULL; + modify_shortcut_docid = 0; + } +}