Autocommit mode

[gma2th]

Description

Add autocommit mode.
Add on error rollback.
When autocommit_mode is set to false, users can manually commit or rollback the current transaction.
When on_error_rollback is set to true, if a statement in a transaction block generates an error, the error is ignored and the transaction continues.
In order to deactivate autocommit for users transactions, without messing up with intern pgcli transactions, we better use two different connections.
Requested here #410

Limitation

Users are not able to activate/disable autocommit from the UI.
Autocommit has to be activated in order to use on error rollback.
No tests to cover the change.

Ressource

• https://www.postgresql.org/docs/current/static/ecpg-sql-set-autocommit.html
• https://www.postgresql.org/docs/current/static/tutorial-transactions.html
• https://www.endpoint.com/blog/2015/02/24/postgres-onerrorrollback-explained

Checklist

• I've added this contribution to the changelog.rst.
• I've added my name to the AUTHORS file (or it's already there).

[j-bennet]

Unit tests are failing with a unicode error there in tests/test_pgexecute.py. Are you able to run them locally (running py.test in pgcli directory)?

[lelit]

I think that the reason of the failure is that the new get_new_connection() method should install the same typecasters as done by connect()... Maybe some refactor could be made, to avoid duplicating basically the same code to create the connection.

Also, I'm not sure if those typecasters should be moved only onto the new user_conn: I would expect that only user queries will involve JSON values...

Finally, in the method execute_normal_sql() (the only place where the new user_conn gets used), there's a loop that pops possible notices: shouldn't it targeting the same user_conn connection?

[codecov-io]

Codecov Report

Merging #917 into master will decrease coverage by 0.51%.
The diff coverage is 59.09%.

@@            Coverage Diff             @@
##           master     #917      +/-   ##
==========================================
- Coverage   84.57%   84.05%   -0.52%     
==========================================
  Files          21       21              
  Lines        2463     2496      +33     
==========================================
+ Hits         2083     2098      +15     
- Misses        380      398      +18     

Impacted Files	Coverage Δ
pgcli/pgexecute.py	78.90% <58.73%> (-3.64%)	⬇️
pgcli/main.py	77.08% <66.66%> (+0.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f0be9d...59f0c77. Read the comment docs.

[gma2th]

Thank you for having a look at this PR.
I fixed the unicode error (due to typecasters) and notices, thank you.
It definitely need some refactor, in order to init all connections in the same place. I can have a look in the next days. This could be done in another PR, up to you.
[Edit] Made the refactor, any feedback welcome. Idk why pep8radius doesn't produce any output locally...

[amjith]

@gma2th I looked through the PR and I have 2 questions. Let me preface these comments by saying, I would like to minimize the number of configuration options in pgcli. It is wise to choose sensible defaults wherever possible.

1. What is the purpose of surfacing the autocommit mode as a config? Is there an advantage in disabling autocommit mode? From my basic understanding we want to have the autocommit mode enabled at all times and the user can specify the BEGIN command to explicitly start a transaction.

2. Why is the on_error_rollback set to off by default? Wouldn't it make sense to have that on all the time?

[amjith]

Hrm. Even when I set the on_error_rollback set to True it ends up aborting the transaction. I haven't traced the issue yet. Can you please verify if it works for you?

[Gollum999]

Any updates on this? I've been interested in this feature for quite a while.

[samskeller]

@amjith @gma2th any updates on getting this through? Not being able to turn off autocommit for pgcli is basically a dealbreaker for me and many others who want some safety from fat-fingering!

[quezak]

Hello, could we get this merged? I want to be able to set autocommit off when accessing important databases :)

[j-bennet]

@quezak This PR has a couple of outstanding questions, and needs a rebase on master. We've heard nothing from @gma2th in a long time, and I don't think they are interested in finishing the work.

Perhaps you'd be interested to pick up and continue working on this feature, by forking pgcli master and porting over these changes?

[quezak]

I see. Let's see if we're able to solve the questions and I could be able to help if it's not too complicated :)

What is the purpose of surfacing the autocommit mode as a config?

I'd like to have autocommit always disabled when I access a production database that's generally not operated manually -- just in case I hit enter without thinking twice :) But on a dev or local instance I'm fine with having autocommit on.

I would like to minimize the number of configuration options

I agree, but also I'd like to have some way to disable autocommit for some databases as described above. I think it would be good to at least limit the number of CLI parameters -- this PR proposes two of them, #535 another one, and at a glance I see more cases in open issues. Maybe it would be nice to come up with a more general solution to allow overriding some config vars from CLI and/or per different databases? I've dropped a few ideas in #535 (comment), but I don't know where would be a good place to discuss it.

[Oliver-Fish]

If anyone could work on this and get it merged, it would be greatly appreciated. This increased safety would be beneficial. I am not super familiar with Python. Otherwise, I would consider jumping and trying this myself.