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
5 changes: 5 additions & 0 deletions pgjdbc/src/main/java/org/postgresql/core/QueryExecutor.java
Expand Up @@ -118,6 +118,11 @@ public interface QueryExecutor extends TypeTransferModeRegistry {
*/
int QUERY_EXECUTE_AS_SIMPLE = 1024;

/**
* Flag indicating that when beginning a transaction, it should be read only.
*/
int QUERY_BEGIN_READ_ONLY = 2048;
Copy link
Member

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 or QUERY_READ_ONLY_HINT better?


/**
* Execute a Query, passing results to a provided ResultHandler.
*
Expand Down
Expand Up @@ -516,7 +516,9 @@ private ResultHandler sendQueryPreamble(final ResultHandler delegateHandler, int

beginFlags = updateQueryMode(beginFlags);

sendOneQuery(beginTransactionQuery, SimpleQuery.NO_PARAMETERS, 0, 0, beginFlags);
final SimpleQuery beginQuery = ((flags & QueryExecutor.QUERY_BEGIN_READ_ONLY) == 0) ? beginTransactionQuery : beginReadOnlyTransactionQuery;

sendOneQuery(beginQuery, SimpleQuery.NO_PARAMETERS, 0, 0, beginFlags);

// Insert a handler that intercepts the BEGIN.
return new ResultHandlerDelegate(delegateHandler) {
Expand Down Expand Up @@ -2731,6 +2733,11 @@ public boolean getIntegerDateTimes() {
new NativeQuery("BEGIN", new int[0], false, SqlCommand.BLANK),
null, false);

private final SimpleQuery beginReadOnlyTransactionQuery =
new SimpleQuery(
new NativeQuery("BEGIN READ ONLY", new int[0], false, SqlCommand.BLANK),
null, false);

private final SimpleQuery EMPTY_QUERY =
new SimpleQuery(
new NativeQuery("", new int[0], false,
Expand Down
49 changes: 45 additions & 4 deletions pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -696,7 +721,6 @@ public synchronized void clearWarnings() throws SQLException {
firstWarning = null;
}


@Override
public void setReadOnly(boolean readOnly) throws SQLException {
checkClosed();
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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):

  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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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?
IGNORE
TRANSACTION
MIXED (currently this PR)
SESSION (current behavior in 42.2.0)

Copy link
Member

Choose a reason for hiding this comment

The 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 begin readonly)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And have default behavior be TRANSACTION?

Copy link
Member

@vlsi vlsi Jul 14, 2018

Choose a reason for hiding this comment

The 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;
Expand All @@ -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);
}
Expand Down
4 changes: 4 additions & 0 deletions pgjdbc/src/main/java/org/postgresql/jdbc/PgStatement.java
Expand Up @@ -404,6 +404,8 @@ private void executeInternal(CachedQuery cachedQuery, ParameterList queryParamet

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

// updateable result sets do not yet support binary updates
Expand Down Expand Up @@ -805,6 +807,8 @@ public int[] executeBatch() throws SQLException {

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

BatchResultHandler handler;
Expand Down
Expand Up @@ -5,7 +5,7 @@

package org.postgresql.test.jdbc2;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.*;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
Expand All @@ -14,6 +14,7 @@
import org.postgresql.jdbc.PgConnection;
import org.postgresql.test.TestUtil;

import org.hamcrest.Matchers;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
Expand Down Expand Up @@ -155,6 +156,100 @@ public void testTransactions() throws Exception {
TestUtil.closeDB(con);
}

/**
* Tests for session and transaction read only behavior.
* @throws Exception
*/
@Test
public void testReadOnly() throws Exception {
con = TestUtil.openDB();
Statement st;
ResultSet rs;

con.setAutoCommit(true);
con.setReadOnly(true);
assertTrue(con.getAutoCommit());
assertTrue(con.isReadOnly());

// Now test insert with auto commit true and read only
st = con.createStatement();
try {
st.executeUpdate("insert into test_a (imagename,image,id) values ('comttest',1234,5678)");
fail("insert should have failed when read only");
} catch (SQLException e) {
assertThat(e.getMessage(), Matchers.containsString("read-only"));
}

con.setAutoCommit(false);

// auto commit false and read only
try {
st.executeUpdate("insert into test_a (imagename,image,id) values ('comttest',1234,5678)");
fail("insert should have failed when read only");
} catch (SQLException e) {
assertThat(e.getMessage(), Matchers.containsString("read-only"));
}

try {
con.setReadOnly(false);
fail("cannot set read only during transaction");
} catch (SQLException e) {
assertThat(e.getMessage(), Matchers.startsWith("Cannot change transaction read-only"));
}

// end the transaction
con.rollback();

// disable read only
con.setReadOnly(false);

assertEquals(1, st.executeUpdate("insert into test_a (imagename,image,id) values ('comttest',1234,5678)"));

// Now update image to 9876 and commit
st.executeUpdate("update test_a set image=9876 where id=5678");
con.commit();

// back to read only for successful query
con.setReadOnly(true);
rs = st.executeQuery("select image from test_a where id=5678");
assertTrue(rs.next());
assertEquals(9876, rs.getInt(1));
rs.close();

// Now try to change with auto commit false
try {
st.executeUpdate("update test_a set image=1111 where id=5678");
fail("update should fail when read only");
} catch (SQLException e) {
assertThat(e.getMessage(), Matchers.containsString("read-only"));
con.rollback();
}

// test that value did not change
rs = st.executeQuery("select image from test_a where id=5678");
assertTrue(rs.next());
assertEquals(9876, rs.getInt(1)); // Should not change!
rs.close();

// repeat attempt to chagne with auto commit true
con.setAutoCommit(true);

try {
st.executeUpdate("update test_a set image=1111 where id=5678");
fail("update should fail when read only");
} catch (SQLException e) {
assertThat(e.getMessage(), Matchers.containsString("read-only"));
}

// test that value did not change
rs = st.executeQuery("select image from test_a where id=5678");
assertTrue(rs.next());
assertEquals(9876, rs.getInt(1)); // Should not change!
rs.close();

TestUtil.closeDB(con);
}

/*
* Simple test to see if isClosed works.
*/
Expand Down