Heikki Linnakangas wrote:
> 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.
Oh, further testing this morning shows that while *queries* on the HS
seem OK, streaming replication is now broken. I probably need to
override transaction isolation on the recovery process. I'll take a
look at that.
> A comment in InitPostgres would be nice, explaining why we want a
> read committed snapshot there.
OK.
> This fixes both the assertion failure and the Windows crash, which
> is good.
I wasn't sure whether it would help with the Windows crash or not.
I'm not set up to build under Windows, so I'm glad you gave that a
check.
> 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.
Did you mean?:
HINT: You can use "SET default_transaction_isolation = 'repeatable
read'" before the first query in the transaction to override it.
Otherwise, I agree and will do.
> 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.
I'll take a look.
>> 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.
Should the check_transaction_read_only() stuff be back-patched to
9.1, too? So far as we know, that's fragile, not broken, right?
Could the fix you envision there cause a behavioral change that could
break anything that users might have in place?
-Kevin