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