"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> How about we fix the serializable versus HS & Windows bugs in one
> patch, and then look at the other as a separate patch? If that's OK,
> I think this is ready, unless my message text can be improved. (And
> I will have a shot at my first back-patching....)
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.
The particular cases we have discussed so far cannot lead to a crash
there, because in startup scenarios XactReadOnly wouldn't be on already.
However I think that a background process not connected to shared memory
could be crashed if somebody changed transaction_read_only from true to
false in postgresql.conf. That's sufficiently far-fetched that maybe we
shouldn't worry about it, but still it seems ugly.
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. That would make its
IsSubTransaction test a bit saner/safer as well. We could do the same
thing in check_XactIsoLevel. We still need a test in
GetSerializableTransactionSnapshot, or someplace else in the vicinity
of that, to cover the default_transaction_isolation = serializable case;
but if we leave the error check in place in check_XactIsoLevel, you'll
get an immediate rather than delayed error from trying to crank the
level up manually in hot standby, which seems more user-friendly.
Will send an updated patch as soon as I'm done testing this idea.
One other point: I notice that Kevin used ERRCODE_FEATURE_NOT_SUPPORTED
in the error messages he added, which seems sane in isolation.
However, the GUC-based code is (by default) throwing
ERRCODE_INVALID_PARAMETER_VALUE when it rejects due to
RecoveryInProgress. I'm not totally sure whether that was thought about
or just fell out of not thinking. Which one do we want here?
regards, tom lane