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
Changes from 2 commits
edf54b4
72c9f0e
741b087
cdcf357
3d22199
27411e7
22c48f3
2b76735
bdf22c6
b8be807
5c8f6ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,6 +99,10 @@ public class PgConnection implements BaseConnection { | |
/* Query that runs ROLLBACK */ | ||
private final Query rollbackQuery; | ||
|
||
private final CachedQuery setSessionReadOnly; | ||
|
||
private final CachedQuery setSessionNotReadOnly; | ||
|
||
private final TypeInfo _typeCache; | ||
|
||
private boolean disableColumnSanitiser = false; | ||
|
@@ -199,6 +203,9 @@ public PgConnection(HostSpec[] hostSpecs, | |
LOGGER.log(Level.WARNING, "Unsupported Server Version: {0}", queryExecutor.getServerVersion()); | ||
} | ||
|
||
setSessionReadOnly = createQuery("SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY", false, true); | ||
setSessionNotReadOnly = createQuery("SET SESSION CHARACTERISTICS AS TRANSACTION READ WRITE", false, true); | ||
|
||
// Set read-only early if requested | ||
if (PGProperty.READ_ONLY.getBoolean(info)) { | ||
setReadOnly(true); | ||
|
@@ -453,6 +460,24 @@ public void execSQLUpdate(String s) throws SQLException { | |
stmt.close(); | ||
} | ||
|
||
void execSQLUpdate(CachedQuery query) throws SQLException { | ||
BaseStatement stmt = (BaseStatement) createStatement(); | ||
if (stmt.executeWithFlags(query, QueryExecutor.QUERY_NO_METADATA | QueryExecutor.QUERY_NO_RESULTS | ||
| QueryExecutor.QUERY_SUPPRESS_BEGIN)) { | ||
throw new PSQLException(GT.tr("A result was returned when none was expected."), | ||
PSQLState.TOO_MANY_RESULTS); | ||
} | ||
|
||
// Transfer warnings to the connection, since the user never | ||
// has a chance to see the statement itself. | ||
SQLWarning warnings = stmt.getWarnings(); | ||
if (warnings != null) { | ||
addWarning(warnings); | ||
} | ||
|
||
stmt.close(); | ||
} | ||
|
||
/** | ||
* <p>In SQL, a result table can be retrieved through a cursor that is named. The current row of a | ||
* result can be updated or deleted using a positioned update/delete statement that references the | ||
|
@@ -696,7 +721,6 @@ public synchronized void clearWarnings() throws SQLException { | |
firstWarning = null; | ||
} | ||
|
||
|
||
@Override | ||
public void setReadOnly(boolean readOnly) throws SQLException { | ||
checkClosed(); | ||
|
@@ -707,9 +731,11 @@ public void setReadOnly(boolean readOnly) throws SQLException { | |
} | ||
|
||
if (readOnly != this.readOnly) { | ||
String readOnlySql | ||
= "SET SESSION CHARACTERISTICS AS TRANSACTION " + (readOnly ? "READ ONLY" : "READ WRITE"); | ||
execSQLUpdate(readOnlySql); // nb: no BEGIN triggered. | ||
//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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be hidden by a default=true connection property. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it might make sense to have it a tri-state option (default should be like item 2):
The idea is to avoid overheads, and still pass hints downstream. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is your option 3 the behavior in 42.2.0 or the behavior currently in this PR? Is there value in 4 options? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this PR. I think we can abandon 42.2.0 behavior (==always fuse There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And have default behavior be TRANSACTION? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. I'm not sure what the naming should be though. |
||
} | ||
} | ||
|
||
this.readOnly = readOnly; | ||
|
@@ -734,6 +760,21 @@ public void setAutoCommit(boolean autoCommit) throws SQLException { | |
commit(); | ||
} | ||
|
||
// if the connection is read only, we need to make sure session settings are | ||
// correct when autocommit status changed | ||
if (this.readOnly) { | ||
// if we are turning on autocommit, we need to set session | ||
// to read only | ||
if (autoCommit) { | ||
this.autoCommit = true; | ||
execSQLUpdate(setSessionReadOnly); | ||
} else { | ||
// if we are turning auto commit off, we need to | ||
// disable session | ||
execSQLUpdate(setSessionNotReadOnly); | ||
} | ||
} | ||
|
||
this.autoCommit = autoCommit; | ||
LOGGER.log(Level.FINE, " setAutoCommit = {0}", autoCommit); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would refrain from mentioning BEGIN here.
Isn't
QUERY_READ_ONLY
orQUERY_READ_ONLY_HINT
better?