Repository: pgjdbc/pgjdbc
Pull request #1252 (2018-07-14 14:06:52)

feat: read only transactions
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.

https://github.com/pgjdbc/pgjdbc/issues/1228
https://github.com/pgjdbc/pgjdbc/issues/848

davecramer: 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.
bokken: @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 https://github.com/pgjdbc/pgjdbc/issues/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.
bokken: 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: >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](https://github.com/pgjdbc/pgjdbc/blob/master/packaging/rpm/postgresql-jdbc.spec.tpl#L61-L76) or not.
@praiskup , could you please clarify? Should test scope dependencies be reflected in packaging scripts?

codecov-io: # [Codecov](https://codecov.io/gh/pgjdbc/pgjdbc/pull/1252?src=pr&el=h1) Report
> Merging [#1252](https://codecov.io/gh/pgjdbc/pgjdbc/pull/1252?src=pr&el=desc) into [master](https://codecov.io/gh/pgjdbc/pgjdbc/commit/80713afe93b480913f5c43921373c49bc091231b?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `73.68%`.



```diff
@@             Coverage Diff              @@
##             master    #1252      +/-   ##
============================================
+ Coverage     68.72%   68.76%   +0.03%     
- Complexity     3857     3871      +14     
============================================
  Files           174      174              
  Lines         16048    16082      +34     
  Branches       2617     2624       +7     
============================================
+ Hits          11029    11058      +29     
- Misses         3781     3784       +3     
- Partials       1238     1240       +2
```

praiskup: On Saturday, July 14, 2018 6:02:39 PM CEST Vladimir Sitnikov wrote:
> 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](https://github.com/pgjdbc/pgjdbc/blob/master/packaging/rpm/postgresql-jdbc.spec.tpl#L61-L76)
> or not.  @praiskup , could you please clarify? Should test scope dependencies
> be reflected in packaging scripts?

We run the testsuite in RPM build, too.  So probably yes;  I fail to see what
are the new dependencies except for "hamcrest";  but hamcrest is in Fedora
repos, just add 'Requires: mvn(org.hamcrest:hamcrest-all)'.




vlsi: @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](https://copr-be.cloud.fedoraproject.org/results/@pgjdbc/pgjdbc-travis/fedora-rawhide-x86_64/00777255-postgresql-jdbc/build.log.gz), 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: On Monday, July 16, 2018 1:53:39 PM CEST Vladimir Sitnikov wrote:
> I don't find `junit` in `BuildRequires`
> Do you know how that works?

The 'junit' package is transitive dependancy through 'maven-local'.

> 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`).

I have zero experience with gradle, dunno.




AppVeyorBot: :white_check_mark: [Build pgjdbc 1.0.227 completed](https://ci.appveyor.com/project/davecramer/pgjdbc/builds/23873769) (commit https://github.com/pgjdbc/pgjdbc/commit/13b8371f86 by @bokken)
DanilaShapkin: 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: @bokken can you rebase fix the conflict and I'll push this
DanilaShapkin: Guys @davecramer @bokken , I'll appreciate if you merge this is pr ASAP.

Patch:
diff --git a/pgjdbc/src/main/java/org/postgresql/PGProperty.java b/pgjdbc/src/main/java/org/postgresql/PGProperty.java
index 9e99dc7a..87705c3a 100644
--- a/pgjdbc/src/main/java/org/postgresql/PGProperty.java
+++ b/pgjdbc/src/main/java/org/postgresql/PGProperty.java
@@ -10,6 +10,7 @@ import org.postgresql.util.GT;
 import org.postgresql.util.PSQLException;
 import org.postgresql.util.PSQLState;
 
+import java.sql.Connection;
 import java.sql.DriverPropertyInfo;
 import java.util.Properties;
 
@@ -443,7 +444,19 @@ public enum PGProperty {
           + "to the database specified in the dbname parameter, "
           + "which will allow the connection to be used for logical replication "
           + "from that database. "
-          + "(backend >= 9.4)");
+          + "(backend >= 9.4)"),
+
+  /**
+   * Connection parameter to control behavior when
+   * {@link Connection#setReadOnly(boolean)} is set to {@code true}.
+   */
+  READ_ONLY_MODE("readOnlyMode", "transaction",
+      "Controls the behavior when a connection is set to be read only, one of 'ignore', 'transaction', or 'always' "
+          + "When 'ignore', setting readOnly has no effect. "
+          + "When 'transaction' setting readOnly to 'true' will cause transactions to BEGIN READ ONLY if autocommit is 'false'. "
+          + "When 'always' setting readOnly to 'true' will set the session to READ ONLY if autoCommit is 'true' "
+          + "and the transaction to BEGIN READ ONLY if autocommit is 'false'.",
+      false, "ignore", "transaction", "always");
 
   private final String name;
   private final String defaultValue;
diff --git a/pgjdbc/src/main/java/org/postgresql/core/BaseConnection.java b/pgjdbc/src/main/java/org/postgresql/core/BaseConnection.java
index 1d316a07..d9f2282b 100644
--- a/pgjdbc/src/main/java/org/postgresql/core/BaseConnection.java
+++ b/pgjdbc/src/main/java/org/postgresql/core/BaseConnection.java
@@ -6,6 +6,7 @@
 package org.postgresql.core;
 
 import org.postgresql.PGConnection;
+import org.postgresql.PGProperty;
 import org.postgresql.jdbc.FieldMetadata;
 import org.postgresql.jdbc.TimestampUtils;
 import org.postgresql.util.LruCache;
@@ -202,4 +203,13 @@ public interface BaseConnection extends PGConnection, Connection {
    * @param flushCacheOnDeallocate true if statement cache should be reset when "deallocate/discard" message observed
    */
   void setFlushCacheOnDeallocate(boolean flushCacheOnDeallocate);
+
+  /**
+   * Indicates if statements to backend should be hinted as read only.
+   *
+   * @return Indication if hints to backend (such as when transaction begins)
+   *         should be read only.
+   * @see PGProperty#READ_ONLY_MODE
+   */
+  boolean hintReadOnly();
 }
diff --git a/pgjdbc/src/main/java/org/postgresql/core/QueryExecutor.java b/pgjdbc/src/main/java/org/postgresql/core/QueryExecutor.java
index 8d95e8d2..a2d23823 100644
--- a/pgjdbc/src/main/java/org/postgresql/core/QueryExecutor.java
+++ b/pgjdbc/src/main/java/org/postgresql/core/QueryExecutor.java
@@ -121,6 +121,11 @@ public interface QueryExecutor extends TypeTransferModeRegistry {
 
   int MAX_SAVE_POINTS = 1000;
 
+  /**
+   * Flag indicating that when beginning a transaction, it should be read only.
+   */
+  int QUERY_READ_ONLY_HINT = 2048;
+
   /**
    * Execute a Query, passing results to a provided ResultHandler.
    *
diff --git a/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java b/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
index 48ca673b..4e85f02a 100644
--- a/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
+++ b/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java
@@ -371,9 +371,7 @@ public class QueryExecutorImpl extends QueryExecutorBase {
               // PostgreSQL does not support bind, exec, simple, sync message flow,
               // so we force autosavepoint to use simple if the main query is using simple
               | QUERY_EXECUTE_AS_SIMPLE);
-
       return true;
-
     }
     return false;
   }
@@ -548,7 +546,9 @@ public class QueryExecutorImpl extends QueryExecutorBase {
 
     beginFlags = updateQueryMode(beginFlags);
 
-    sendOneQuery(beginTransactionQuery, SimpleQuery.NO_PARAMETERS, 0, 0, beginFlags);
+    final SimpleQuery beginQuery = ((flags & QueryExecutor.QUERY_READ_ONLY_HINT) == 0) ? beginTransactionQuery : beginReadOnlyTransactionQuery;
+
+    sendOneQuery(beginQuery, SimpleQuery.NO_PARAMETERS, 0, 0, beginFlags);
 
     // Insert a handler that intercepts the BEGIN.
     return new ResultHandlerDelegate(delegateHandler) {
@@ -1145,8 +1145,8 @@ public class QueryExecutorImpl extends QueryExecutorBase {
             try {
               if (op == null) {
                 throw new PSQLException(GT
-                  .tr("Received CommandComplete ''{0}'' without an active copy operation", status),
-                  PSQLState.OBJECT_NOT_IN_STATE);
+                    .tr("Received CommandComplete ''{0}'' without an active copy operation", status),
+                    PSQLState.OBJECT_NOT_IN_STATE);
               }
               op.handleCommandStatus(status);
             } catch (SQLException se) {
@@ -1171,7 +1171,7 @@ public class QueryExecutorImpl extends QueryExecutorBase {
 
             if (op != null) {
               error = new PSQLException(GT.tr("Got CopyInResponse from server during an active {0}",
-                op.getClass().getName()), PSQLState.OBJECT_NOT_IN_STATE);
+                  op.getClass().getName()), PSQLState.OBJECT_NOT_IN_STATE);
             }
 
             op = new CopyInImpl();
@@ -1184,8 +1184,7 @@ public class QueryExecutorImpl extends QueryExecutorBase {
             LOGGER.log(Level.FINEST, " <=BE CopyOutResponse");
 
             if (op != null) {
-              error =
-                new PSQLException(GT.tr("Got CopyOutResponse from server during an active {0}",
+              error = new PSQLException(GT.tr("Got CopyOutResponse from server during an active {0}",
                   op.getClass().getName()), PSQLState.OBJECT_NOT_IN_STATE);
             }
 
@@ -1199,8 +1198,7 @@ public class QueryExecutorImpl extends QueryExecutorBase {
             LOGGER.log(Level.FINEST, " <=BE CopyBothResponse");
 
             if (op != null) {
-              error =
-                new PSQLException(GT.tr("Got CopyBothResponse from server during an active {0}",
+              error = new PSQLException(GT.tr("Got CopyBothResponse from server during an active {0}",
                   op.getClass().getName()), PSQLState.OBJECT_NOT_IN_STATE);
             }
 
@@ -1220,11 +1218,11 @@ public class QueryExecutorImpl extends QueryExecutorBase {
             byte[] buf = pgStream.receive(len);
             if (op == null) {
               error = new PSQLException(GT.tr("Got CopyData without an active copy operation"),
-                PSQLState.OBJECT_NOT_IN_STATE);
+                  PSQLState.OBJECT_NOT_IN_STATE);
             } else if (!(op instanceof CopyOut)) {
               error = new PSQLException(
-                GT.tr("Unexpected copydata from server for {0}", op.getClass().getName()),
-                PSQLState.COMMUNICATION_ERROR);
+                  GT.tr("Unexpected copydata from server for {0}", op.getClass().getName()),
+                  PSQLState.COMMUNICATION_ERROR);
             } else {
               op.handleCopydata(buf);
             }
@@ -1242,7 +1240,7 @@ public class QueryExecutorImpl extends QueryExecutorBase {
 
             if (!(op instanceof CopyOut)) {
               error = new PSQLException("Got CopyDone while not copying from server",
-                PSQLState.OBJECT_NOT_IN_STATE);
+                  PSQLState.OBJECT_NOT_IN_STATE);
             }
 
             // keep receiving since we expect a CommandComplete
@@ -1283,7 +1281,7 @@ public class QueryExecutorImpl extends QueryExecutorBase {
 
           default:
             throw new IOException(
-              GT.tr("Unexpected packet type during copy: {0}", Integer.toString(c)));
+                GT.tr("Unexpected packet type during copy: {0}", Integer.toString(c)));
         }
 
         // Collect errors into a neat chain for completeness
@@ -2792,6 +2790,11 @@ public class QueryExecutorImpl extends QueryExecutorBase {
           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 emptyQuery =
       new SimpleQuery(
           new NativeQuery("", new int[0], false,
@@ -2815,7 +2818,4 @@ public class QueryExecutorImpl extends QueryExecutorBase {
       new SimpleQuery(
           new NativeQuery("ROLLBACK TO SAVEPOINT PGJDBC_AUTOSAVE", new int[0], false, SqlCommand.BLANK),
           null, false);
-
-
-
 }
diff --git a/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java b/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java
index f4be8197..8baa1fb9 100644
--- a/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java
+++ b/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java
@@ -912,6 +912,22 @@ public abstract class BaseDataSource implements CommonDataSource, Referenceable
     PGProperty.READ_ONLY.set(properties, readOnly);
   }
 
+  /**
+   * @return The behavior when set read only
+   * @see PGProperty#READ_ONLY_MODE
+   */
+  public String getReadOnlyMode() {
+    return PGProperty.READ_ONLY_MODE.get(properties);
+  }
+
+  /**
+   * @param mode the behavior when set read only
+   * @see PGProperty#READ_ONLY_MODE
+   */
+  public void setReadOnlyMode(String mode) {
+    PGProperty.READ_ONLY_MODE.set(properties, mode);
+  }
+
   /**
    * @return true if driver should log unclosed connections
    * @see PGProperty#LOG_UNCLOSED_CONNECTIONS
diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java b/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java
index d9f4debc..2e7b9f2c 100644
--- a/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java
+++ b/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java
@@ -81,6 +81,12 @@ public class PgConnection implements BaseConnection {
   private static final SQLPermission SQL_PERMISSION_ABORT = new SQLPermission("callAbort");
   private static final SQLPermission SQL_PERMISSION_NETWORK_TIMEOUT = new SQLPermission("setNetworkTimeout");
 
+  private enum ReadOnlyBehavior {
+    ignore,
+    transaction,
+    always;
+  }
+
   //
   // Data initialized on construction:
   //
@@ -89,6 +95,8 @@ public class PgConnection implements BaseConnection {
   /* URL we were created via */
   private final String creatingURL;
 
+  private final ReadOnlyBehavior readOnlyBehavior;
+
   private Throwable openStackTrace;
 
   /* Actual network handler */
@@ -99,6 +107,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;
@@ -184,6 +196,8 @@ public class PgConnection implements BaseConnection {
 
     this.creatingURL = url;
 
+    this.readOnlyBehavior = getReadOnlyBehavior(PGProperty.READ_ONLY_MODE.get(info));
+
     setDefaultFetchSize(PGProperty.DEFAULT_ROW_FETCH_SIZE.getInt(info));
 
     setPrepareThreshold(PGProperty.PREPARE_THRESHOLD.getInt(info));
@@ -199,6 +213,9 @@ public class PgConnection implements BaseConnection {
       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);
@@ -292,6 +309,18 @@ public class PgConnection implements BaseConnection {
     replicationConnection = PGProperty.REPLICATION.get(info) != null;
   }
 
+  private static ReadOnlyBehavior getReadOnlyBehavior(String property) {
+    try {
+      return ReadOnlyBehavior.valueOf(property);
+    } catch (IllegalArgumentException e) {
+      try {
+        return ReadOnlyBehavior.valueOf(property.toLowerCase(Locale.US));
+      } catch (IllegalArgumentException e2) {
+        return ReadOnlyBehavior.transaction;
+      }
+    }
+  }
+
   private static Set<Integer> getBinaryOids(Properties info) throws PSQLException {
     boolean binaryTransfer = PGProperty.BINARY_TRANSFER.getBoolean(info);
     // Formats that currently have binary protocol support
@@ -453,6 +482,24 @@ public class PgConnection implements BaseConnection {
     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
@@ -705,10 +752,8 @@ public class PgConnection implements BaseConnection {
           PSQLState.ACTIVE_SQL_TRANSACTION);
     }
 
-    if (readOnly != this.readOnly) {
-      String readOnlySql
-             = "SET SESSION CHARACTERISTICS AS TRANSACTION " + (readOnly ? "READ ONLY" : "READ WRITE");
-      execSQLUpdate(readOnlySql); // nb: no BEGIN triggered.
+    if (readOnly != this.readOnly && autoCommit && this.readOnlyBehavior == ReadOnlyBehavior.always) {
+      execSQLUpdate(readOnly ? setSessionReadOnly : setSessionNotReadOnly);
     }
 
     this.readOnly = readOnly;
@@ -721,6 +766,11 @@ public class PgConnection implements BaseConnection {
     return readOnly;
   }
 
+  @Override
+  public boolean hintReadOnly() {
+    return readOnly && readOnlyBehavior != ReadOnlyBehavior.ignore;
+  }
+
   @Override
   public void setAutoCommit(boolean autoCommit) throws SQLException {
     checkClosed();
@@ -733,6 +783,21 @@ public class PgConnection implements BaseConnection {
       commit();
     }
 
+    // if the connection is read only, we need to make sure session settings are
+    // correct when autocommit status changed
+    if (this.readOnly && readOnlyBehavior == ReadOnlyBehavior.always) {
+      // 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);
   }
diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/PgStatement.java b/pgjdbc/src/main/java/org/postgresql/jdbc/PgStatement.java
index efc0e336..3a3ee015 100644
--- a/pgjdbc/src/main/java/org/postgresql/jdbc/PgStatement.java
+++ b/pgjdbc/src/main/java/org/postgresql/jdbc/PgStatement.java
@@ -408,6 +408,9 @@ public class PgStatement implements Statement, BaseStatement {
     if (connection.getAutoCommit()) {
       flags |= QueryExecutor.QUERY_SUPPRESS_BEGIN;
     }
+    if (connection.hintReadOnly()) {
+      flags |= QueryExecutor.QUERY_READ_ONLY_HINT;
+    }
 
     // updateable result sets do not yet support binary updates
     if (concurrency != ResultSet.CONCUR_READ_ONLY) {
@@ -810,6 +813,9 @@ public class PgStatement implements Statement, BaseStatement {
     if (connection.getAutoCommit()) {
       flags |= QueryExecutor.QUERY_SUPPRESS_BEGIN;
     }
+    if (connection.hintReadOnly()) {
+      flags |= QueryExecutor.QUERY_READ_ONLY_HINT;
+    }
 
     BatchResultHandler handler;
     handler = createBatchHandler(queries, parameterLists);


Test patch:
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ConnectionTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ConnectionTest.java
index a0e211b9..0b5ca798 100644
--- a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ConnectionTest.java
+++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ConnectionTest.java
@@ -6,11 +6,13 @@
 package org.postgresql.test.jdbc2;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import org.postgresql.PGProperty;
 import org.postgresql.jdbc.PgConnection;
 import org.postgresql.test.TestUtil;
 
@@ -26,6 +28,7 @@ import java.sql.SQLWarning;
 import java.sql.Statement;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Properties;
 
 /**
  * TestCase to test the internal functionality of org.postgresql.jdbc2.Connection and it's
@@ -155,6 +158,214 @@ public class ConnectionTest {
     TestUtil.closeDB(con);
   }
 
+  /*
+   * Tests for session and transaction read only behavior with "always" read only mode.
+   */
+  @Test
+  public void testReadOnly_always() throws Exception {
+    final Properties props = new Properties();
+    PGProperty.READ_ONLY_MODE.set(props, "always");
+    con = TestUtil.openDB(props);
+    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) {
+      assertStringContains(e.getMessage(), "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) {
+      assertStringContains(e.getMessage(), "read-only");
+    }
+
+    try {
+      con.setReadOnly(false);
+      fail("cannot set read only during transaction");
+    } catch (SQLException e) {
+      assertStringContains(e.getMessage(), "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) {
+      assertStringContains(e.getMessage(), "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) {
+      assertStringContains(e.getMessage(), "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);
+  }
+
+  /*
+   * Tests for session and transaction read only behavior with "ignore" read only mode.
+   */
+  @Test
+  public void testReadOnly_ignore() throws Exception {
+    final Properties props = new Properties();
+    PGProperty.READ_ONLY_MODE.set(props, "ignore");
+    con = TestUtil.openDB(props);
+    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();
+    assertEquals(1, st.executeUpdate("insert into test_a (imagename,image,id) values ('comttest',1234,5678)"));
+    con.setAutoCommit(false);
+
+    // Now update image to 9876 and commit
+    st.executeUpdate("update test_a set image=9876 where id=5678");
+
+    // back to read only for successful query
+    rs = st.executeQuery("select image from test_a where id=5678");
+    assertTrue(rs.next());
+    assertEquals(9876, rs.getInt(1));
+    rs.close();
+
+    con.rollback();
+
+    // test that value did not change
+    rs = st.executeQuery("select image from test_a where id=5678");
+    assertTrue(rs.next());
+    assertEquals(1234, rs.getInt(1)); // Should not change!
+    rs.close();
+
+    TestUtil.closeDB(con);
+  }
+
+  /*
+   * Tests for session and transaction read only behavior with "transaction" read only mode.
+   */
+  @Test
+  public void testReadOnly_transaction() throws Exception {
+    final Properties props = new Properties();
+    PGProperty.READ_ONLY_MODE.set(props, "transaction");
+    con = TestUtil.openDB(props);
+    Statement st;
+    ResultSet rs;
+
+    con.setAutoCommit(false);
+    con.setReadOnly(true);
+    assertFalse(con.getAutoCommit());
+    assertTrue(con.isReadOnly());
+
+    // Test insert with auto commit false 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) {
+      assertStringContains(e.getMessage(), "read-only");
+    }
+
+    con.rollback();
+
+    con.setAutoCommit(true);
+    assertTrue(con.isReadOnly());
+    //with autocommit true and read only, can still insert
+    assertEquals(1, st.executeUpdate("insert into test_a (imagename,image,id) values ('comttest',1234,5678)"));
+
+    // Now update image to 9876
+    st.executeUpdate("update test_a set image=9876 where id=5678");
+
+    //successful query
+    rs = st.executeQuery("select image from test_a where id=5678");
+    assertTrue(rs.next());
+    assertEquals(9876, rs.getInt(1));
+    rs.close();
+
+    con.setAutoCommit(false);
+    // 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) {
+      assertStringContains(e.getMessage(), "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);
+
+    assertEquals(1, st.executeUpdate("update test_a set image=1111 where id=5678"));
+
+    // test that value did not change
+    rs = st.executeQuery("select image from test_a where id=5678");
+    assertTrue(rs.next());
+    assertEquals(1111, rs.getInt(1)); // Should not change!
+    rs.close();
+
+    TestUtil.closeDB(con);
+  }
+
   /*
    * Simple test to see if isClosed works.
    */
@@ -296,4 +507,10 @@ public class ConnectionTest {
     con.close();
     con.close();
   }
+
+  private static void assertStringContains(String orig, String toContain) {
+    if (!orig.contains(toContain)) {
+      fail("expected [" + orig + ']' + "to contain [" + toContain + "].");
+    }
+  }
 }
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/DateTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/DateTest.java
index 1f54965d..c7da9294 100644
--- a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/DateTest.java
+++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/DateTest.java
@@ -67,6 +67,7 @@ public class DateTest {
     assertEquals(1, stmt.executeUpdate(TestUtil.insertSQL("testdate", "'3456-01-01'")));
     assertEquals(1, stmt.executeUpdate(TestUtil.insertSQL("testdate", "'0101-01-01 BC'")));
 
+
     /* dateTest() contains all of the tests */
     dateTest();
 
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc4/jdbc41/GetObjectTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc4/jdbc41/GetObjectTest.java
index bcdc541e..d20a400b 100644
--- a/pgjdbc/src/test/java/org/postgresql/test/jdbc4/jdbc41/GetObjectTest.java
+++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc4/jdbc41/GetObjectTest.java
@@ -338,6 +338,36 @@ public class GetObjectTest {
     }
   }
 
+  @Test
+  public void testGetNullDate() throws SQLException {
+    Statement stmt = conn.createStatement();
+    stmt.executeUpdate(TestUtil.insertSQL("table1","date_column","NULL"));
+
+    ResultSet rs = stmt.executeQuery(TestUtil.selectSQL("table1", "date_column"));
+    try {
+      assertTrue(rs.next());
+      Date date = rs.getObject(1,Date.class);
+      assertTrue(rs.wasNull());
+    } finally {
+      rs.close();
+    }
+  }
+
+  @Test
+  public void testGetNullTimestamp() throws SQLException {
+    Statement stmt = conn.createStatement();
+    stmt.executeUpdate(TestUtil.insertSQL("table1","timestamp_without_time_zone_column","NULL"));
+
+    ResultSet rs = stmt.executeQuery(TestUtil.selectSQL("table1", "timestamp_without_time_zone_column"));
+    try {
+      assertTrue(rs.next());
+      java.util.Date ts = rs.getObject(1, java.util.Date.class);
+      assertTrue(rs.wasNull());
+    } finally {
+      rs.close();
+    }
+  }
+
   /**
    * Test the behavior getObject for time columns.
    */