Add support for Prefer tx=rollback

[wolfgangwalther]

Resolves #1598.

In that issue we discussed Prefer: rollback vs. Prefer: tx=rollback. I implemented the latter and also added Prefer: tx=commit (the default) for completeness (see #1656 as well).

---

This would also allow adding another config option to use rollback as the default, to be overwritten with Prefer: tx=commit. This would be something that I would run the test environment in my projects with, because those automated tests should almost never change the database for real.

Now that I write this, I wonder whether we should change the config option allow-rollback to something like allow-prefer-tx now. This would allow us to allow/deny the use of tx=commit, when the default is rollback.

The combo of tx-default = rollback and allow-prefer-tx = false could be a cool thing to setup a demo postgrest server, where everyone can make requests against a test schema immediately, maybe even from a web-interface?

Would rollback-by-default = true | false or tx-default = commit | rollback be better? Any other ideas?

---

Back to the current PR. Prefer: tx=rollback is only applied for POST, PUT, PATCH and DELETE requests and only when those do not return with an error. Not for GET and HEAD. The Preference-Applied header is set accordingly. When a request returns an error, the transaction is rolled back anyway, the error is returned regularly and the Preference-Applied header is not set.

TODO:

• Add Preference-Applied header for explicit tx=commit.
• Rename allow-rollback to allow-prefer-tx or something better
• Add test for ActionInvoke
• Add rollback-by-default config option

And then for another PR: Make rollback the default for our spec-tests. This will require quite a bit of cleanup. My goal would be to be able to run the whole test suite multiple times against the same database without recreating the schema.

[steve-chavez]

Oh.. wait. I remember the rollback logic being simple. On:

postgrest/src/PostgREST/App.hs

Lines 89 to 91 in 944efb3

	handleReq = runPgLocals conf claims (app dbStructure conf) apiRequest
	dbResp <- P.use pool $ HT.transaction HT.ReadCommitted (txMode apiRequest) handleReq
	return $ either (errorResponseFor . PgError authed) identity dbResp

Do:

handleReq =  HT.condemn >> runPgLocals conf claims (app dbStructure proc cols conf) apiRequest

I remember getting almost all the tests green, besides the ones that depended on each other. It should work.

---

My goal would be to be able to run the whole test suite multiple times against the same database without recreating the schema.

Are you using Nix? This is something I already do with postgrest-test-spec. You can even switch versions or run the tests on all of them.

[wolfgangwalther]

Oh.. wait. I remember the rollback logic being simple.

Thanks. I was going to ask about that, the current implementation felt a bit repetitive :D

Are you using Nix? This is something I already do with postgrest-test-spec. You can even switch versions or run the tests on all of them.

Yes, I do. Since yesterday :). I was a bit short with the explanation here or maybe my logic was kind of backwards. With postgrest-test-spec (and also make test which I used before), the database is recreated on each run. That works fine, of course.

Let me start somewhere else: When I did #1658 I felt like sorting the following lines alphabetically while trying to figure out where to insert my new lines:

postgrest/test/Main.hs

Lines 103 to 114 in 944efb3

	("Feature.AuthSpec" , Feature.AuthSpec.spec actualPgVersion)
	, ("Feature.RawOutputTypesSpec" , Feature.RawOutputTypesSpec.spec)
	, ("Feature.ConcurrentSpec" , Feature.ConcurrentSpec.spec)
	, ("Feature.CorsSpec" , Feature.CorsSpec.spec)
	, ("Feature.JsonOperatorSpec" , Feature.JsonOperatorSpec.spec actualPgVersion)
	, ("Feature.OpenApiSpec" , Feature.OpenApiSpec.spec)
	, ("Feature.OptionsSpec" , Feature.OptionsSpec.spec)
	, ("Feature.QuerySpec" , Feature.QuerySpec.spec actualPgVersion)
	, ("Feature.EmbedDisambiguationSpec" , Feature.EmbedDisambiguationSpec.spec)
	, ("Feature.RpcSpec" , Feature.RpcSpec.spec actualPgVersion)
	, ("Feature.AndOrParamsSpec" , Feature.AndOrParamsSpec.spec actualPgVersion)
	, ("Feature.UpsertSpec" , Feature.UpsertSpec.spec)

However, when I do that - suddenly some tests turn red. This is because some of those tests depend on each other across specs. That's bad.

Getting the tests to run without recreating the database would prove to a certain degree, that this coupling across tests is removed.

Then I noticed:

postgrest/test/Main.hs

Line 125 in 944efb3

-- Only certain Specs need a database reset, this should be used with care as it slows down the whole test suite.

So maybe it could help with performance while running the tests as well.

[wolfgangwalther]

So I did that and learned a bit about Monads as well. Also did all the other TODOs and simplified the tests a alot. I think they read really nicely now.

[steve-chavez]

Both of these default to False right?

I remember this part being confusing because of Hlint. Check this: ndmitchell/hlint#970 (comment)

Maybe newer versions don't show this hint.

[wolfgangwalther]

Uh, eh. I have no idea. This is not covered by the tests, I guess I'll need to add to the io-tests.

I just copied from a couple more lines above, I think and thought False must be False. But according to the link you're giving... I am completely lost now :D

[steve-chavez]

Actually when trying the tx-rollback-all=true while running the postgrest binary I don't get the all ROLLBACK behavior. When changing these to (Just True ==) I do get the behavior. Maybe these need to be corrected?

[wolfgangwalther]

Hm. Ok. I had a brief look at the IO tests, but I don't think it's simple to add such a test there. What I would really like to have is a command line argument to call the postgrest binary with, which just exits and dumps the effectively loaded config. This could be useful for debugging and the io tests.

[wolfgangwalther]

Since this is very confusing, I added some very explicit comments on each of those boolean options to make that clear.

The IO test is something for later.

[steve-chavez]

Yes, very confusing. We should look at the latest hlint version and see if that still prevents us from using a fromMaybe False.

[monacoremo]

That might be very useful! Related topic if we add an exit command line option (or env variable): It would also be useful to have an option for postgrest to exit once the dbstrucutre is loaded. That would allow us to benchmark the startup time (including in CI) as a basis for refactorings/improvements

[wolfgangwalther]

Those should definitely be command line options and not environment variables.

Something like --dump-config could work. Is there anything useful that we could output for the dbstructure? What if --dump-structure would output a json with all the query results?

[wolfgangwalther]

Yes, very confusing. We should look at the latest hlint version and see if that still prevents us from using a fromMaybe False.

I just changed all of them to a fromMaybe False and hlint is not complaining. Maybe this is fixed already with what we have now. Let's see what CI says.

[monacoremo]

Yes, that would be the relevant output. Will need to discuss in more detail at some other point I think, just wanted to flag that we might want to consider additional exit points if we add that command line option

[steve-chavez]

TIL about mapeResponseHeaders. Maybe that can be used for the Prefer: return as well. That would solve #740.

[steve-chavez]

Looks good! Don't forget to add an entry to the CHANGELOG :)

[steve-chavez]

Had an idea for saving effort on the CHANGELOG: PostgREST/postgrest-docs#340 (comment)

[steve-chavez]

Edit: Nevermind. I cannot reproduce anymore(I recreated the AWS VPC, not sure what went wrong). I now get around 15 req/s less. which is not a big deal.

---

After this change I'm getting a drop in performance of about 100 req/s on the GETSingle load test:

Some runs with prepared statements without tx rollback logic:

• 2069.639507/s
• 2073.134588/s
• 2078.380637/s
• 2067.949048/s

With tx rollback logic:

• 1908.421302/s
• 1927.784673/s
• 1924.697279/s
• 1930.679238/s

#1609 would have definitely helped in detecting this.

---

I'm not sure what part of the code is causing the regression(I thought mapResponseHeaders).

My idea was to shortcut this code path in case tx-end=commit is not modified, but no dice: steve-chavez@ea3f582

Removing the logic does bring back the previous perf results: steve-chavez@18057c1.

Not sure how to fix for now :(

[wolfgangwalther]

Edit: Nevermind. I cannot reproduce anymore(I recreated the AWS VPC, not sure what went wrong). I now get around 15 req/s less. which is not a big deal.

Puh. I will still try to have a go at using #1609 and see what I get there locally to confirm.

[wolfgangwalther]

So I did a few test runs across 30 min with postgrest-loadtest. The commit with the tx=rollback change came it at 347 req/s and 350 reqs/. The commit immediately before that came in at 349 req/s. I couldn't see any drop in performance.

[steve-chavez]

That's good. The tests I did before were 1 min only. In general I've found that when there's around 100req/s less on short tests then usually that means a drop in perf. This case was weird for me, the load tests on a newly created AWS VPC tended to be somewhat reliable.

[dsyrstad]

This should be added to the doc. I just spent 30 minutes searching for something like this and ran across this PR. Edit: I guess it is kind of documented here: https://postgrest.org/en/latest/configuration.html#db-tx-end, but that's not completely obvious.

[steve-chavez]

Thanks for the feedback @dsyrstad. I've opened PostgREST/postgrest-docs#511 to make the rollback header more noticeable.