Re: default_isolation_level='serializable' crashes on Windows - Mailing list pgsql-hackers

I wrote:
> I poked around this area a bit.  I notice that
> check_transaction_read_only has got the same fundamental error: it
> thinks it can safely consult RecoveryInProgress(), which in general
> it cannot.

After rereading the whole thread I saw that Heikki had already pointed
this out, and come to the same conclusion about how to fix it:

> On reflection I think maybe the best solution is for
> check_transaction_read_only to test IsTransactionState(), and just
> allow the change always if outside a transaction.

Attached is a version of the patch that does it like that.  I've checked
that this fixes the problem (as well as Robert's earlier report) in an
EXEC_BACKEND build, which is as close as I can get to the Windows
environment.

I tweaked Kevin's error message to keep the same capitalization as the
existing text for the message in check_XactIsoLevel --- if we change
that it will cause work for the translators, and I don't think it's
enough of an improvement to justify that.

I also back-propagated the use of ERRCODE_FEATURE_NOT_SUPPORTED into
the GUC check hooks.  On reflection this seems more appropriate than
ERRCODE_INVALID_PARAMETER_VALUE.

Lastly, I simplified the added code in InitPostgres down to just a
bare assignment to XactIsoLevel.  It doesn't seem worthwhile to add
all the cycles involved in SetConfigOption(), when we have no desire
to change the GUC permanently.  (I think Kevin's code was wrong anyway
in that it was using PGC_S_OVERRIDE, which would impact the reset
state for the GUC.)

I think this is ready to go.  Kevin, do you want to apply it?  You
had mentioned wanting some practice with back-patches.

            regards, tom lane

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 0e9eb3a64f5db66dd2fca68252a597e68defdbf8..5d894ba77f74d02637008deedd4ca2919d8af6b8 100644
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
*************** show_log_timezone(void)
*** 533,543 ****
   * read-only may be changed to read-write only when in a top-level transaction
   * that has not yet taken an initial snapshot.    Can't do it in a hot standby
   * slave, either.
   */
  bool
  check_transaction_read_only(bool *newval, void **extra, GucSource source)
  {
!     if (*newval == false && XactReadOnly)
      {
          /* Can't go to r/w mode inside a r/o transaction */
          if (IsSubTransaction())
--- 533,548 ----
   * read-only may be changed to read-write only when in a top-level transaction
   * that has not yet taken an initial snapshot.    Can't do it in a hot standby
   * slave, either.
+  *
+  * If we are not in a transaction at all, just allow the change; it means
+  * nothing since XactReadOnly will be reset by the next StartTransaction().
+  * The IsTransactionState() test protects us against trying to check
+  * RecoveryInProgress() in contexts where shared memory is not accessible.
   */
  bool
  check_transaction_read_only(bool *newval, void **extra, GucSource source)
  {
!     if (*newval == false && XactReadOnly && IsTransactionState())
      {
          /* Can't go to r/w mode inside a r/o transaction */
          if (IsSubTransaction())
*************** check_transaction_read_only(bool *newval
*** 556,561 ****
--- 561,567 ----
          /* Can't go to r/w mode while recovery is still active */
          if (RecoveryInProgress())
          {
+             GUC_check_errcode(ERRCODE_FEATURE_NOT_SUPPORTED);
              GUC_check_errmsg("cannot set transaction read-write mode during recovery");
              return false;
          }
*************** check_transaction_read_only(bool *newval
*** 569,574 ****
--- 575,582 ----
   *
   * We allow idempotent changes at any time, but otherwise this can only be
   * changed in a toplevel transaction that has not yet taken a snapshot.
+  *
+  * As in check_transaction_read_only, allow it if not inside a transaction.
   */
  bool
  check_XactIsoLevel(char **newval, void **extra, GucSource source)
*************** check_XactIsoLevel(char **newval, void *
*** 598,604 ****
      else
          return false;

!     if (newXactIsoLevel != XactIsoLevel)
      {
          if (FirstSnapshotSet)
          {
--- 606,612 ----
      else
          return false;

!     if (newXactIsoLevel != XactIsoLevel && IsTransactionState())
      {
          if (FirstSnapshotSet)
          {
*************** check_XactIsoLevel(char **newval, void *
*** 616,621 ****
--- 624,630 ----
          /* Can't go to serializable mode while recovery is still active */
          if (newXactIsoLevel == XACT_SERIALIZABLE && RecoveryInProgress())
          {
+             GUC_check_errcode(ERRCODE_FEATURE_NOT_SUPPORTED);
              GUC_check_errmsg("cannot use serializable mode in a hot standby");
              GUC_check_errhint("You can use REPEATABLE READ instead.");
              return false;
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index b22faf9607d87f1a2bc7ce306e923dc12b8cfcc7..8b5a95ceaa0ff460d34231dadc2d3562fa7b0c63 100644
*** a/src/backend/storage/lmgr/predicate.c
--- b/src/backend/storage/lmgr/predicate.c
*************** GetSerializableTransactionSnapshot(Snaps
*** 1572,1577 ****
--- 1572,1590 ----
      Assert(IsolationIsSerializable());

      /*
+      * Can't use serializable mode while recovery is still active, as it is,
+      * for example, on a hot standby.  We could get here despite the check
+      * in check_XactIsoLevel() if default_transaction_isolation is set to
+      * serializable, so phrase the hint accordingly.
+      */
+     if (RecoveryInProgress())
+         ereport(ERROR,
+                 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                  errmsg("cannot use serializable mode in a hot standby"),
+                  errdetail("\"default_transaction_isolation\" is set to \"serializable\"."),
+                  errhint("You can use \"SET default_transaction_isolation = 'repeatable read'\" to change the
default.")));
+
+     /*
       * A special optimization is available for SERIALIZABLE READ ONLY
       * DEFERRABLE transactions -- we can wait for a suitable snapshot and
       * thereby avoid all SSI overhead once it's running.
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 6a3fc6f693f66304a487032e2099d1660590d836..bef301a79c8fea1f602f5bd8b340bb4950d285fa 100644
*** a/src/backend/utils/init/postinit.c
--- b/src/backend/utils/init/postinit.c
*************** InitPostgres(const char *in_dbname, Oid
*** 584,589 ****
--- 584,598 ----
          /* statement_timestamp must be set for timeouts to work correctly */
          SetCurrentStatementStartTimestamp();
          StartTransactionCommand();
+
+         /*
+          * transaction_isolation will have been set to the default by the
+          * above.  If the default is "serializable", and we are in hot
+          * standby, we will fail if we don't change it to something lower.
+          * Fortunately, "read committed" is plenty good enough.
+          */
+         XactIsoLevel = XACT_READ_COMMITTED;
+
          (void) GetTransactionSnapshot();
      }