Unless the secondary storage is under direct control by the primary storage it would be hard to avoid any race conflict or getting a large delay. A working solution could be to check one database and then continue, leaving the remaining databases to use rollback for sorting out remaining conflicts. Those will normally be edit conflicts within a short time frame.

Two different issues here:

• We need to not save invalid data to the page table if it was rejected from the secondary tables. Might be better to have a Content::isValid() method that is called before save (but can also be used at other places, such as preview or whatever) and does all the needed checks (in case of WikibaseItem this includes some reads to the db to see if no duplicate stuff is entered). This method can then remove the invalid data and continue the save or abort it, and also return the encountered issues to the caller. One thing to keep in mind is that any changes to this kind of info should always be written to both the page table and the secondary tables, preferably in the same transaction.

• We need some way of dealing with race conditions. If a save happens and we do a read, and it turns out all is well, we must still be sure that no write that creates a previously absent conflict does not happen before the save completes. Not sure how to do this in MediaWiki. Probably needs some investigation.

Let's not put all the storage related stuff into a single bug, so I suggest splitting up the second issue into it's own bug, so this one can be closed once the first one has been addressed.

check one database and then continue, leaving the remaining databases

What databases are you talking about? AFAIK we will only have a single master db for the Wikidata (repo) wiki. This db will hold both the page table and the secondary tables needed for duplication checks, so we won't need to care about any additional stuff we set up for answering queries or other stuff I think.

If secondary storage is within the same database in the repo there should be no problem to use transactions.

I agree that we need a Content::isValid() method (or maybe better: getErrors(), returning a Status object or false). Checking this before save is something that would need to be done in core, I guess. (By the way - being able to cache the result of this check would be a reason to have non-mutable Content objects. Caching would be useful since the check may be called multiple times during a save, on different levels of processing)

I think doing this kind of check before saving is the first thing we need to do, no matter how we try to enforce database consistency. By itself, it introduces a race condition of course. But one that would rarely be hit. While we should try to avoid inconsistent database states, it would be fine to just show a database error message or some such in case we hit a conflict.

As to keeping the database consistent, here's the key points:

• checking validity, saving the primary blob and storing secondary data must all happen in the same transaction.

• if everything should be consistent, there's no distinction (in this context) between primary and secondary data. Indeed, it would be handy to perform the "primary" update the same way the secondary updates are performed: using a list of objects representing updates jobs.

• we should consider the case of having secondary data on several different database systems.

• full ACID conformance always bares the risk of operations blocking for a long time (because of retires in case of communication failure in the commit phase).

Eventually, I think it should work this way (in core):

• The initial validity check, writing the primary blob and updating the secondary data stores should all be modeled as update objects.

• When a page is to be saved, a list of update object is constructed. Each update object can open/commit and abort a transaction, and perform the actual update.

• The update is then performed in 3 stages: open all transactions, do all updates, then commit all transactions.

It would be very hard to make the commit phase truly atomic. I believe we will have to live with the risk of inconsistencies introduces by connections failing the the middle of a transaction.

Anyway, for now, I think it's sufficient to implement a pre-save check without any transactional logic. We should keep the transaction stuff for later. Maybe in a separate ticket.

Yeah, +1, that's basically what I said, but broken down better :)

using a list of objects representing updates jobs

As long as the jobs get run immediately - would be very bad to have them end up in the jobque and not seeing the changes made right after save.

By the way - being able to cache the result of this check would be a reason to have non-mutable Content objects.

Disagree. You can easily cache the validity and set it to unknown when a change that can impact it is made to the object. This would be even more effective since you can ignore changes that don't impact it (you'd need to pass $isValid to the constructor in the immutable case, which seems evil), and you don't have all the overhead of constantly creating new instances. ... And it would not work in the first place since you need to check right before doing the save, unless you don't mind making the transaction significantly "less atomic" :)

(In reply to comment #5)

Yeah, +1, that's basically what I said, but broken down better :)

using a list of objects representing updates jobs

As long as the jobs get run immediately - would be very bad to have them end up
in the jobque and not seeing the changes made right after save.

Yes, of course. Such job objects may also be used for deferred updates, but it must be very clear which jobs have to run when.

By the way - being able to cache the result of this check would be a reason to have non-mutable Content objects.

Disagree. You can easily cache the validity and set it to unknown when a change
that can impact it is made to the object. This would be even more effective
since you can ignore changes that don't impact it

yes, but it's much more error prone and harder to maintain. but I don't insist on imutable objects :)

(you'd need to pass $isValid
to the constructor in the immutable case, which seems evil),

no - immutable only says that isValid() (and all other getters) will always return the same value. Which can be cached internally, but may be initialized lazily.

That's of course problematic in cases like this, when isValid() depends on external state (the database).

and you don't have
all the overhead of constantly creating new instances.

We could have a mutator object, analogous to a StringBuffer in java. But whatever.

... And it would not
work in the first place since you need to check right before doing the save,
unless you don't mind making the transaction significantly "less atomic" :)

Well, if you want to avoid a race condition, yes. Then you can't cache the value anyway. But as I said, I think the race condition is acceptable, as long as it doesn't lead to permanent data corruption or loss.

Constraint violation in secondary storage, is that solved now? That is, is it possible to close this bug or should it be merged with bug 36519 - Validate data structure?

There was an erroneous additional save that lead to the race condition. Seems like it is fixed.

fixed (fir sitelinks) in https://gerrit.wikimedia.org/r/13119

Verified in Wikidata demo time for sprint 8