dmfay / massive-js

A data mapper for Node.js and PostgreSQL.
2.49k stars 158 forks source link

Transactions #543

Closed flux627 closed 6 years ago

flux627 commented 6 years ago

I can see from the past that this has been a touchy subject (#14 #81 #207 #208) but I wanted to bring this up once again. Lack of transaction support is currently a dealbreaker for my use-case, which is such a shame because Massive makes everything else we're doing so much better.

Maybe we can bootstrap the transaction support of pg-promise? I don't think this was an option in previous discussions.

dmfay commented 6 years ago

pg-promise's transaction capability was one of the big reasons I made sure it's possible to use the driver directly. I wanted it to be possible to run transactions even if having to use pgp's idioms instead of Massive's makes for a bit of a mechanical disconnect.

A while back I was looking at doing some stuff with proxies to support multiple connection pools by switching out what happened under the hood when Massive invoked the driver's query functionality. I never quite got it off the ground and I've been focusing on other stuff since, but if it worked out I think it could apply to transactions as well. Or if you have other ideas, I'm definitely open to discussing them!

vitaly-t commented 6 years ago

A while back I was looking at doing some stuff with proxies to support multiple connection pools by switching out what happened under the hood when Massive invoked the driver's query functionality

As it stands now, each Database object creates its own Pool object, and each instance of Massive creates one Database object, so you should end up with one Pool per Massive root object :wink:

dmfay commented 6 years ago

What I was trying was to maintain multiple pools in the Database object and build the API tree out of proxies so I could attach different pools at the root of the same tree -- you'd be able to db.mytable.find(...) or db.poolname.mytable.find(...) as necessary, and in the latter case you'd be navigating a tree of proxies to the former which would substitute the appropriate pool into db.query. The usage seemed a little less arcane than adding a poolName option. I didn't get it off the ground and it's been on ice for a good while though :grimacing:

But it seems like the same principle could apply for transactions, a db.tx returning a similar proxy tree representing the API that tracks and uses a handle to the connection maintaining the transaction.

ferdinandsalis commented 6 years ago

I really would love to utilise task and tx from massive. Anything I can do to help facilitate this ☺️

dmfay commented 6 years ago

You can already run raw SQL in tasks & transactions through db.instance. What's missing is the ability to work with the API (db.mytable.insert(...)) in a transaction. Like I mentioned above, I've got ideas in that direction but haven't been able to follow through yet. If you're interested in hacking on it I'm happy to talk ideas & review pull requests :)

ferdinandsalis commented 6 years ago

What's missing is the ability to work with the API (db.mytable.insert(...)) in a transaction

That is what I was referring to. I am interested though I am sure I lack the knowledge and direction. Any directions would be very helpful. Will start by reading up on the current implementation. Thanks

dmfay commented 6 years ago

This is more or less what I envision as far as the syntax (with async/await since that's a lot more readable):

db.transaction(async function (tx) {
  const one = await tx.mytable.insert(...);
  const two = await tx.anothertable.insert(...);

  try {
    tx.aScriptWhichCouldFail();
  } catch (e) {
    return tx.rollback(e);
  }

  return tx.commit();
});

So tx has a copy of the API Massive builds and attaches to db. This is where it gets sticky: each Queryable, Table, or Executable maintains an encapsulated dependency on db so it can query Postgres through the db.query method. In order to build something like this, that circle has to be broken somehow.

The proxy stuff I talked about before was one idea: if a Queryable could hot-swap what db meant, the same structure could be used with different connection pools or transactions.

Another possibility is to brute-force copy entities off the Database object and attach them to the Transaction object, but I'm a little worried about performance in that case.

vitaly-t commented 6 years ago

@dmfay If you are to use pg-promise transactions, you would never resort to manual commit/rollback. Those things work automatically within pg-promise transactions, that's the most important feature of those transactions. More importantly, without those you would run into all sorts of trouble that you may not even know exist. Here's just one example: https://github.com/brianc/node-postgres/issues/1454

Other things that pg-promise provides for transactions:

flux627 commented 6 years ago

@vitaly-t, just to clarify- you say "... without those you would run into all sorts of trouble ... . Here's just one example". Then, the example issue you link does not seem to be resolved. Is that issue resolved by the things you mentioned pg-promise does automatically?

vitaly-t commented 6 years ago

Is that issue resolved by the things you mentioned pg-promise does automatically?

Yes. As that link explains, pg-promise is implementing a work-around for that issue, not to attempt execution of ROLLBACK when a transaction fails as a result of a failed connection, to avoid halting the process.

dmfay commented 6 years ago

I had a little time to play around with it and this is suspiciously light but tailing the log shows it behaving as expected -- check out the transactions branch. I'm just brute-forcing the API with _.cloneDeep and overriding instance; I'd like to do that more elegantly but this may turn out good enough? At least it's not incurring the full introspection hit.

ferdinandsalis commented 6 years ago

@dmfay awesome, will take a look!

dmfay commented 6 years ago

@ferdinandsalis @flux627 I'm interested in hearing how it's working out for you if you've had a chance to take it for a spin!

ferdinandsalis commented 6 years ago

Hi @dmfay. Yes I did try it out but not enough to judge if it works for me.

On a different note. I am currently in the middle of a big refactor and I really want to use massive but I am deploying with ‘up’ to aws lambda, its stateless and every request will incur the cost of the introspection query. Is there any way to mitigate this (some way of persisting the result).

If not I would just love to use the query builder part of massive. Dont know if this even makes sense. I just dont want to use knex, bookshelf.js, sequelize, et al. Also I am a fan of pg-promise.

vitaly-t commented 6 years ago

If not I would just love to use the query builder part of massive. Dont know if this even makes sense. I just dont want to use knex, bookshelf.js, sequelize, et al. Also I am a fan of pg-promise.

I don't know if Massive.js supports this, like pg-promise does, exposing the complete query-formatting engine separate from everything else, i.e. namespace pgp.as, and in particular method pgp.as.format(query, values, options), on which the entire library relies. But if it doesn't then it should, and then it would be possible to combine the query-building facility of Massive.js with an external query execution engine.

dmfay commented 6 years ago

@ferdinandsalis I'm not familiar with lambda but you could require/import the individual massive/lib/statement/{select,insert,update,delete} files and build your own queries by mocking the source spec (see lib/queryable and lib/table constructors for what those need to be) and passing in criteria and options. It'll be a little gnarly and you won't have the API tree but you shouldn't need to intitialize to call format.

ferdinandsalis commented 6 years ago

@dmfay I will try doing this. Thanks for your input everyone.

dmfay commented 6 years ago

Transactions are available starting with v5 now :tada: