Thread

1. Re: default_isolation_level='serializable' crashes on Windows

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-08-15T03:12:05Z

   > "Kevin Grittner" wrote:
   > Heikki Linnakangas wrote:
   >> On 14.08.2012 14:25, Kevin Grittner wrote:
   
   Attached is version 3.
   
   >>> 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.
   >>
   >> Hmm, seems to work for me. Do you get an unexpected error or what?
   >
   > No, I wasn't getting errors in the clients or the logs, but I
   > wasn't seeing data pop up on the replica when I expected. Perhaps I
   > messed up my streaming replication configuration somehow.
   
   Yeah, setup error. I accidentally dropped the primary_conninfo
   setting from my recovery.conf file. Now that I've put that back, it
   works just fine.
   
   >> I didn't, I meant to point out that you can set
   >> transaction_isolation just for the one transaction. But your
   >> suggested hint is OK as well - you suggest to set it for the whole
   >> session, which will also work around the problem. The "before the
   >> first query in the transaction" isn't necessary in that case
   >> though.
   
   >> Yet another option is to suggest the "BEGIN ISOLATION LEVEL
   >> REPEATABLE READ" syntax, instead of SET.
   >
   > I'm inclined toward hinting at a session override of the default.
   > If you're typing away in psql, that's a lot less work. :-)
   
   I tinkered with the messages (including correcting a reverse-logic
   bug in which was displayed under what circumstances) and tweaked a
   comment. I struggled with how to capitalize and quote. Let me know
   what you think.
   
   >>>>> 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?
   >>
   >> Good question. I don't see how it could cause a behavioral change,
   >> but I've been wrong before.
   >
   > If we don't know of any actual existing bugs I'm inclined to not
   > back-patch that part to 9.1, although it makes sense for 9.2 since
   > we shouldn't be risking breakage of any production systems. I'm
   > really cautious about giving anybody any excuse not to apply a
   > minor update.
   
   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....)
   
   -Kevin
   
   
   

2. Re: default_isolation_level='serializable' crashes on Windows

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-08-23T16:55:46Z

   "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
   
   
   

3. Re: default_isolation_level='serializable' crashes on Windows

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-08-23T18:50:07Z

   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
   
   

4. Re: default_isolation_level='serializable' crashes on Windows

   Kevin Grittner <kevin.grittner@wicourts.gov> — 2012-08-23T19:48:37Z

   Tom Lane <tgl@sss.pgh.pa.us> wrote:
    
   > 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.
    
   That's one of the reasons I agonized over it -- I think the way I
   left it is a little better and more consistent with other messages,
   but didn't know whether the difference was worth translator effort. 
   I'm happy to trust your judgment on that.
    
   > 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.)
    
   Point taken on PGC_S_OVERRIDE.  And that probably fixes the issue
   that caused me to hold up when I was about ready to pull the trigger
   this past weekend.  A final round of testing showed a "SET" line on
   psql start, which is clearly wrong.  I suspected that I needed to go
   to a lower level in setting that, but hadn't had a chance to sort
   out just what the right path was.  In retrospect, just directly
   assigning the value seems pretty obvious.
    
   > I think this is ready to go.
    
   With your changes, I agree.
    
   > Kevin, do you want to apply it?  You had mentioned wanting some
   > practice with back-patches.
    
   I'm getting on a plane to Istanbul in less than 48 hours for the
   VLDB conference, and scrambling to tie up loose ends.  I don't want
   to be under that kind of time-pressure when I back-patch for the
   first time, for fear of making a mess of things and not being around
   to clean up the mess; so my first back-patch is probably best left
   for another time.
    
   I'll run through my tests again tonight, against your patch, not
   that I expect any problems with it.  Unfortunately I can't test
   Windows, as I don't have a build environment for that.
    
   Thanks for going over this.
    
   -Kevin
   
   
   

5. Re: default_isolation_level='serializable' crashes on Windows

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-08-23T20:04:01Z

   "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
   > I'll run through my tests again tonight, against your patch, not
   > that I expect any problems with it.  Unfortunately I can't test
   > Windows, as I don't have a build environment for that.
   
   FWIW, you can approximate Windows close enough for this type of problem
   by building with EXEC_BACKEND defined.  I usually add the #define to
   pg_config.h after configuring, though of course there's more than one
   way to do it.
   
   			regards, tom lane
   
   
   

6. Re: default_isolation_level='serializable' crashes on Windows

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-08-24T17:13:45Z

   "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
   > Tom Lane <tgl@sss.pgh.pa.us> wrote:
   >> 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.)
    
   > Point taken on PGC_S_OVERRIDE.  And that probably fixes the issue
   > that caused me to hold up when I was about ready to pull the trigger
   > this past weekend.  A final round of testing showed a "SET" line on
   > psql start, which is clearly wrong.  I suspected that I needed to go
   > to a lower level in setting that, but hadn't had a chance to sort
   > out just what the right path was.  In retrospect, just directly
   > assigning the value seems pretty obvious.
   
   Hm, I'm not sure why using SetConfigOption() would result in anything
   happening client-side.  (If this GUC were GUC_REPORT, that would result
   in a parameter-change report to the client; but it isn't, and anyway
   that shouldn't cause psql to print "SET".)  Might be worth looking
   closer at what was happening there.
   
   >> Kevin, do you want to apply it?  You had mentioned wanting some
   >> practice with back-patches.
    
   > I'm getting on a plane to Istanbul in less than 48 hours for the
   > VLDB conference, and scrambling to tie up loose ends.
   
   OK, I've committed it.
   
   			regards, tom lane