Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: read only transactions #1252

Merged
merged 11 commits into from Nov 25, 2019
Merged

feat: read only transactions #1252

merged 11 commits into from Nov 25, 2019

Conversation

@bokken
Copy link
Member

bokken commented Jul 14, 2018

If autocommit is set to false, read only will be set on begin
transaction.
If autocommit is true, it will continue to be managed at session level.
The queries to change session have been cached to avoid re-parsing each
time readonly value changes.

#1228
#848

If autocommit is set to false, read only will be set on begin
transaction.
If autocommit is true, it will continue to be managed at session level.
The queries to change session have been cached to avoid re-parsing each
time readonly value changes.

#1228
#848
/**
* Flag indicating that when beginning a transaction, it should be read only.
*/
int QUERY_BEGIN_READ_ONLY = 2048;

This comment has been minimized.

Copy link
@vlsi

vlsi Jul 14, 2018

Member

I would refrain from mentioning BEGIN here.

Isn't QUERY_READ_ONLY or QUERY_READ_ONLY_HINT better?

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Jul 14, 2018

I still fail to see the point in this. The only use case is with pgpool in transaction mode. I would much rather see us add a startup connection parameter to the backend and use that as a hint to pools to route to a secondary or read only server for the query.

checkstyle and hamcrest test import
@bokken

This comment has been minimized.

Copy link
Member Author

bokken commented Jul 14, 2018

@davecramer I agree that this is imperfect, but I think it is a move in a better direction.

If autocommit is true, the queries to adjust the session are now cached (directly addressing #1228).

If autocommit is false, read only is managed with the transaction rather than on session.

This is the simplest solution I could think of.
Trying to wrap every statement into a transaction does not seem practical (when to commit/rollback?).
Adding a new command/parameter/hint to backend would certainly work (though it seems the set session to read only could also be used as hint for pools to do the same thing), but that obviously cannot be achieved with driver alone.

I really intended this PR as a starting point for discussion. If the result is we don't really like a simple but limited approach, I am cool with that. I benefit from seeing the code written and tested to think about advantages and drawbacks more concretely.

//if autocommit is true, then we change things at the session level
//if autocommit is false, read only is managed with the transaction
if (autoCommit) {
execSQLUpdate(readOnly ? setSessionReadOnly : setSessionNotReadOnly);

This comment has been minimized.

Copy link
@vlsi

vlsi Jul 14, 2018

Member

This should be hidden by a default=true connection property.
That is by default pgjdbc should not issue SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY at all.

This comment has been minimized.

Copy link
@bokken

bokken Jul 14, 2018

Author Member

Just to make sure I understand. You are proposing a new connection property which would, by default, make setting readOnly only used when autoCommit is false?

This addresses issue 1228 by not doing any work when setReadOnly is called.

When autocommit is false transaction will be read only, which gives hint to pools.

This comment has been minimized.

Copy link
@vlsi

vlsi Jul 14, 2018

Member

Well, it might make sense to have it a tri-state option (default should be like item 2):

  1. always ignore readonly
  2. pass readonly for begin only (autocommit==false)
  3. pass readonly always (like 2, but with set session in autocommit mode)

The idea is to avoid overheads, and still pass hints downstream.

This comment has been minimized.

Copy link
@bokken

bokken Jul 14, 2018

Author Member

Is your option 3 the behavior in 42.2.0 or the behavior currently in this PR?

Is there value in 4 options?
IGNORE
TRANSACTION
MIXED (currently this PR)
SESSION (current behavior in 42.2.0)

This comment has been minimized.

Copy link
@vlsi

vlsi Jul 14, 2018

Member

this PR.

I think we can abandon 42.2.0 behavior (==always fuse begin readonly)

This comment has been minimized.

Copy link
@bokken

bokken Jul 14, 2018

Author Member

And have default behavior be TRANSACTION?

This comment has been minimized.

Copy link
@vlsi

vlsi Jul 14, 2018

Member

yes. I'm not sure what the naming should be though.

@bokken

This comment has been minimized.

Copy link
Member Author

bokken commented Jul 14, 2018

Why are the travis ci jobs failing not able to find the hamcrest library? I added it to the pom. Is there something in the job config using different dependencies than what is declared in the pom?

@vlsi

This comment has been minimized.

Copy link
Member

vlsi commented Jul 14, 2018

something in the job config using different dependencies than what is declared in the pom?

jre7 and jre6 use this repositories to build: https://github.com/pgjdbc/pgjdbc-jre7/blob/master/pom.xml and https://github.com/pgjdbc/pgjdbc-jre6/blob/master/pom.xml

CI job fetches those repositories AFAIK.

If you want add hamcrest, you'd need to to that in https://github.com/pgjdbc/pgjdbc-parent-poms/blob/master/pgjdbc-core-parent/pom.xml
Unfortunately, that would require to release a new parent-poms version and update it elsewhere.

I'm not sure if test dependencies should be added to rpm packaging or not.
@praiskup , could you please clarify? Should test scope dependencies be reflected in packaging scripts?

bokken added 2 commits Jul 14, 2018
add connection property with 3 options to control read only behavior
fix missing property methods on BaseDataSource
avoid redundant static modifier
@@ -404,6 +404,8 @@ private void executeInternal(CachedQuery cachedQuery, ParameterList queryParamet

if (connection.getAutoCommit()) {
flags |= QueryExecutor.QUERY_SUPPRESS_BEGIN;
} else if (connection.isTransactionReadOnly()) {

This comment has been minimized.

Copy link
@vlsi

vlsi Jul 14, 2018

Member

I would avoid else here as it would mean that both flags are independent.

This comment has been minimized.

Copy link
@bokken

bokken Jul 14, 2018

Author Member

allowing some future change on the executor side to actually disentangle them?

This comment has been minimized.

Copy link
@vlsi

vlsi Jul 14, 2018

Member

I hate when flags are dependent.
In my point of view, flags should either be independent or it should be a single flag with known values.

more loosely couple read only hints to backend
* @see PGProperty#READ_ONLY_MODE
*/
public String getReadOnlyMode() {
return PGProperty.READ_ONLY_MODE.getSetString(properties);

This comment has been minimized.

Copy link
@bokken

bokken Jul 14, 2018

Author Member

Should just be 'get(properties)'

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 14, 2018

Codecov Report

Merging #1252 into master will increase coverage by 0.01%.
The diff coverage is 70%.

@@             Coverage Diff              @@
##             master    #1252      +/-   ##
============================================
+ Coverage     68.78%   68.79%   +0.01%     
- Complexity     3908     3922      +14     
============================================
  Files           179      179              
  Lines         16420    16455      +35     
  Branches       2674     2681       +7     
============================================
+ Hits          11295    11321      +26     
- Misses         3880     3886       +6     
- Partials       1245     1248       +3
bokken added 2 commits Jul 14, 2018
return default read only mode from data source.
avoid case conversion
@praiskup

This comment has been minimized.

Copy link
Member

praiskup commented Jul 16, 2018

@vlsi

This comment has been minimized.

Copy link
Member

vlsi commented Jul 16, 2018

@praiskup , we have junit dependency: https://github.com/pgjdbc/pgjdbc-parent-poms/blob/master/pgjdbc-core-parent/pom.xml#L27-L29
I don't find junit in BuildRequires

Do you know how that works?
Copr logs show that tests are run, however it is not clear where junit comes from.

PS. What do you think of Gradle? Is building via Gradle an option?
I think it might help us to use a single repository to manage the code (and eliminate the need for pgjdbc-parent-poms).

@praiskup

This comment has been minimized.

Copy link
Member

praiskup commented Jul 16, 2018

@bokken

This comment has been minimized.

Copy link
Member Author

bokken commented Jul 16, 2018

I just removed the dependency on hamcrest.

# Conflicts:
#	pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
#	pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java
@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Apr 15, 2019

@DanilaShapkin

This comment has been minimized.

Copy link

DanilaShapkin commented Nov 13, 2019

Hello!
Could you tell me when are you going to release this task?
There was a problem when working with the pgbouncer "pool_mode=transaction" same as #848

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Nov 13, 2019

@bokken can you rebase fix the conflict and I'll push this

@DanilaShapkin

This comment has been minimized.

Copy link

DanilaShapkin commented Nov 25, 2019

Guys @davecramer @bokken , I'll appreciate if you merge this is pr ASAP.

# Conflicts:
#	pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Nov 25, 2019

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Nov 25, 2019

@bokken seems you have a few check style issues to fix... :)

BO8979 BO8979
@bokken

This comment has been minimized.

Copy link
Member Author

bokken commented Nov 25, 2019

I cannot get checkstyle to run correctly on on windows :(
Let's see if I have made it happy...

BO8979 BO8979
@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Nov 25, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Nov 25, 2019

@bokken

This comment has been minimized.

Copy link
Member Author

bokken commented Nov 25, 2019

@davecramer the checkstyle gods have been sated.

@davecramer davecramer merged commit 0507979 into pgjdbc:master Nov 25, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Copy link

Vadimkin448 left a comment


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.