Objection.js: Feature request: row-level locking option for upsertGraph

[ollisal]

Currently I think there is no option to have upsertGraph take row-level locks to the "existing" rows it modifies. It just does a bunch of SELECTs (via an eager query) to determine which rows are already existing in each table and what needs to be done for each row (insert, update, delete) based on that. But other database operations can then modify, or even delete or add relevant rows in the related tables, before those modifications are carried out.

If there was an option to have the initial SELECTs take row-level locks to the selected row(s), it would allow a pattern where a top-level row, or something else with children still underneath it, is used as a 'mutex' that blocks such conflicting updates to any related rows.

It would probably be useful to have an option to either lock the row(s) at the root level, or all existing rows. Just the first one alone would be helpful, though. For the latter there would need to be some defined order that it takes the locks in, based on the structure of the graph. If the lock order is hard to determine, it would be impossible to avoid deadlocks from other code taking some of the same locks in a different order. Just locking the top-level rows is simpler in this regard.

[heisian]

could this be solved by putting your .upsertGraph() call inside of a transaction block?

[ollisal]

A transaction is needed across the full upsertGraph operation, yes, to make the proposed locks have an effect. Row-level locks are taken until the end of current transaction, after all, or just for the duration of the statement if there is none. But just using a transaction alone doesn't ensure the initially SELECTed rows don't change.

[heisian]

right, but one would want to also put the initial SELECT within the same transaction block, no?

[heisian]

the problem with my suggestion is that one would have to SELECT (or in Objection MyModel.query(trx).where({ ... })) and verify the data of the model, plus any other nested models in the graph, and then perform query(trx).upsertGraph({ ... }) which could be a bit messy for complex graphs.

[ollisal]

Ah indeed, one could first manually select and lock the same row(s) as upsertGraph is expected to find as existing rows, in the same transaction where upsertGraph is going to be executed in, as a workaround.

That's an additional query round-trip of course (in addition to the essentially same SELECT the upsertGraph is going to do internally anyway, just without taking the lock) but that wouldn't be a big problem for most use-cases.

[heisian]

Yeah, that problem could perhaps be solved if there was a hook that you could define for .upsertGraph() to perform your data checks after reading but before writing.. my gut thinks that it's possible..

[heisian]

Actually, you could also build your own "upsertGraph()" method:

(async () => {

  // Wrap everything in a transaction
  await Objection.transaction((trx) => {

    // Perform your initial fetch, eager loading relations
    const user = await User
      .query(trx)
      .eager({
        emails: true,
        addresses: true,
        resources: { subresources: true },
      })
      .where('id', 123)

      // Iterate through every node in the eager-loaded graph
      .traverse(async (model, parentModel, relationName) => {
        // perform data checks here

        // update model instance
        await model.$query(trx).patch({ ... })
      })

  })

})()

[koskimas]

For postgres, this could probably be solved by propagating forUpdate and forShare calls to the select queries executed before the upsert operations. So doing this:

Person.query(trx).upsertGraph(graph).forUpdate()

would call forUpdate() for all SELECT queries that are used to fetch the current state of the graph.

[koskimas]

Implemented here https://github.com/Vincit/objection.js/commit/a8992dca4f1ed6a236f64aa5cc801e2ffc766cfe

[jure]

As said in this comment https://github.com/Vincit/objection.js/commit/a8992dca4f1ed6a236f64aa5cc801e2ffc766cfe#r34086038, I don't think a8992dc implements what you're describing above, @koskimas?

Or am I missing something crucial here? From the description, I was expecting that all SELECT queries that occur during an upsertGraph have a FOR UPDATE appended, but looking at tests and the actual functionality, it's only the root query that gets that appended. I've tried fiddling with GraphOperation bits here https://github.com/Vincit/objection.js/blob/master/lib/queryBuilder/graph/GraphOperation.js#L65 but got lost in the weeds. Some guidance would be much appreciated!

[koskimas]

@jure In which case would it make sense to lock all the selected rows? I think you are missing the point of row locking

[jure]

Yes, I can’t seem to wrap my head around this completely.
So the way I imagined this would work, is that all select queries done during an upsertGraph() would have an appended for update. That’s how I understood the description:

by propagating forUpdate and forShare calls to the select queries executed before the upsert operations

I realize that Postgres, for example, will automatically additionally lock rows that are related on the database level with foreign keys: http://shiroyasha.io/selecting-for-share-and-update-in-postgresql.html - is that what you’re relying on?

The way I understand the current implementation is that, talking specifically about the test, Model1’s selected rows get locked, but Model2’s rows remain unlocked. If upsertGraph will change Model2, it could be that there’s another patch or upsertGraph somewhere that will mess with the integrity of it.

So, I guess what I’m failing to understand is how a for update on a single of those 4 selects that upsertGraph does during that test, is enough.