nikita-volkov / hasql-transaction

A composable abstraction over retriable transactions for Hasql
http://hackage.haskell.org/package/hasql-transaction
MIT License
12 stars 15 forks source link

Question: Why BEGIN/COMMIT/ROLLBACK are prepared by default? #15

Closed steve-chavez closed 3 years ago

steve-chavez commented 3 years ago

Hey @nikita-volkov,

Currently the BEGIN.., COMMIT, ROLLBACK statements are prepared on:

https://github.com/nikita-volkov/hasql-transaction/blob/56dafd8ac0ea5abd70b93167dec72ee51a826a63/library/Hasql/Transaction/Private/Statements.hs#L15-L25.

I was wondering if preparing those brings a perf advantage, perhaps you benchmarked it?

Preparing those statements by default makes PostgREST incompatible with connection poolers(using transaction pooling), according to https://github.com/PostgREST/postgrest/pull/1633#issuecomment-716705188.

nikita-volkov commented 3 years ago

I think it's possible to extend the executing function with a parameter controlling whether these control statements should be prepared. Would that suffice?

While we're on it, what do you think about the experimental non-monadic, but selective API presented in the 0.10.* version space? It associates Mode and IsolationLevel with the individual statements and automatically chooses the strictest settings from all the constituent settings when running the transaction. This makes it more safe and conceptually correct I think.

I'm asking because I've been planning to release it as another library and I would rather introduce that change to the new library.

steve-chavez commented 3 years ago

I think it's possible to extend the executing function with a parameter controlling whether these control statements should be prepared. Would that suffice?

Yes, that should do it!

It associates Mode and IsolationLevel with the individual statements

So this interface would be like using SET TRANSACTION between the statements right? Like:

BEGIN ISOLATION LEVEL READ COMMITTED READ ONLY;
SET TRANSACTION READ WRITE;
INSERT INTO projects VALUES (1,'project 1',null)
-- INSERT 0 1
SET TRANSACTION READ ONLY;
INSERT INTO projects VALUES (2,'project 2',null)
-- ERROR:  25006: cannot execute INSERT in a read-only transaction
ROLLBACK

This makes it more safe and conceptually correct I think.

It does look more aligned with pg capabilities, but it's also a bit more complicated to use. I assume most hasql-transactions users would have to create wrappers for statement/sql/session for setting the tx mode globally, which should be the most common use case(also for PostgREST).

I'm asking because I've been planning to release it as another library and I would rather introduce that change to the new library.

Yes, I think that'd be the best way to go. Users that want more control over transactions could go to the hasql-dynamic-transaction library.

nikita-volkov commented 3 years ago

So this interface would be like using SET TRANSACTION between the statements right?

No. The idea is to still have single isolation and mode set in the begin statement. The difference is that those settings now participate in composition. E.g.,

findUserByEmail :: Text -> Transaction (Maybe Int32)
findUserByEmail =
  statement Read ReadCommitted Statement.findUserByEmail

insertUser :: Text -> ByteString -> Text -> Maybe Text -> Transaction Int32
insertUser email password name phone =
  statement Write Serializable Statement.insertUser
    (email, password, name, phone)

{-|
The isolation and mode settings of this transaction will automatically
be derived as Write and Serializable.
-}
findOrInsertUser :: Text -> ByteString -> Text -> Maybe Text -> Transaction Int32
findOrInsertUser email password name phone =
  fromMaybeS
    (insertUser email password name phone))
    (findUserByEmail email)

So findOrInsertUser will be executed as one of the following (depending on the result of finding the user):

begin isolation level serializable read write;
select ... from "user" ...;
commit;

or

begin isolation level serializable read write;
select ... from "user" ...;
insert into "user" ...;
commit;

This is the way things are implemented there now. However your interpretation gives me an idea how an optimisation could be implemented there, which would gradually increase the isolation and mode settings depending on the execution path that the transaction takes.

With that optimization in place the sql would then be the following:

begin isolation level read committed read;
select ... from "user" ...;
commit;

or

begin isolation level read committed read;
select ... from "user" ...;
set transaction isolation level serializable read write;
insert into "user" ...;
commit;

What do you think about the way it works now and the potential optimization?

nikita-volkov commented 3 years ago

I've just realised that set transaction makes it possible to have a Monad instance with that API. 😲

steve-chavez commented 3 years ago

No. The idea is to still have single isolation and mode set in the begin statement.

Ah, got it. In my case I think I'd do a INSERT .. ON CONFLICT DO NOTHING RETURNING * for a find or insert, but the composition looks nice for doing it at the Haskell level.

begin isolation level read committed read; select ... from "user" ...; set transaction isolation level serializable read write; insert into "user" ...; commit;

@nikita-volkov Oh, unfortunately that won't work:

BEGIN ISOLATION LEVEL READ COMMITTED READ ONLY;
SET TRANSACTION READ WRITE;
INSERT INTO projects VALUES (1333333,'project 1',null);
SET TRANSACTION READ ONLY;
SELECT * FROM proejcts;
SET TRANSACTION READ WRITE;
-- ERROR:  25001: transaction read-write mode must be set before any query
-- LOCATION:  call_bool_check_hook, guc.c:10102

Basically it looks like it's not possible to go from READ ONLY to READ WRITE if a statement is executed before.

nikita-volkov commented 3 years ago

Oh. That is unfortunate.

Any way, I'll release the confirmed updates in the coming days.

shinzui commented 2 years ago

I've just realised that set transaction makes it possible to have a Monad instance with that API.

That would be very convenient. @nikita-volkov, Is that a planned API change?

nikita-volkov commented 2 years ago

I've just realised that set transaction makes it possible to have a Monad instance with that API.

That would be very convenient. @nikita-volkov, Is that a planned API change?

Unfortunately it's impossible. See https://github.com/nikita-volkov/hasql-transaction/issues/15#issuecomment-749309476