On 14.08.2012 08:23, Kevin Grittner wrote:
OK, attached is a first cut to confirm that the approach looks sane
to everyone; I *think* it is along the lines that we reached
consensus.  After moving the check to the point where we get a
serializable snapshot, it was still behaving badly.  It took a bit,
but forcing the snapshot acquisition in InitPostgres to use 'read
committed' instead of the default isolation level got reasonable
behavior in my initial tests.  It sure looks a lot better to *me*
than what was happening before.

A comment in InitPostgres would be nice, explaining why we want a read committed snapshot there.

Besides confirming that this is the solution people want, this has
not been tested well enough yet to be ready for commit.  It's getting
late, though, and I'm fading.  If the overall approach looks good
I'll beat up on it some more tomorrow.

Thanks! This fixes both the assertion failure and the Windows crash, which is good.

Now that the error is thrown when the first snapshot is taken, rather than at the SET command, I think the error message needs some work:

postgres=# select 123;
ERROR:  cannot use serializable mode in a hot standby
HINT:  You can use REPEATABLE READ instead.

If the isolation level came from default_transaction_isolation, set in postgresql.conf file, it might take the user a while to figure that out. Perhaps something like this:

ERROR:  cannot use serializable mode in a hot standby
DETAIL: default_transaction_isolation was set to 'serializable' in the config file. HINT: You can use "SET transaction_isolation = 'repeatable read'" before the first query in the transaction to override it.

This still leaves the RecoveryInProgress() call in check_transaction_read_only(), which isn't a problem at the moment, but seems fragile. I think we should still add the IsTransactionState() check in there, so that it works without shared memory access. If we're not in a transaction yet (TRANS_DEFAULT), setting transaction_read_only has no effect, as it's overwritten at the beginning of a transaction. If we're in one of the transitory states, TRANS_START or TRANS_COMMIT/ABORT/PREPARE, I'm not sure what should happen, but it should not be possible for user code to run and change transaction_read_only in those states.

Since the existing behavior is so bad, I'm inclined to think this
merits backpatching to 9.1.  Thoughts on that?

Yes, we have to somehow fix the crash and the assertion failure on 9.1.

  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to