Thread

1. Hot Standby, release candidate?

   Simon Riggs <simon@2ndquadrant.com> — 2009-12-13T19:20:27Z

   Enclose latest version of Hot Standby. This is the "basic" patch, with
   no must-fix issues and no known bugs. Further additions will follow
   after commit, primarily around usability, which will include additional
   control functions for use in testing. Various thoughts discussed here
   http://wiki.postgresql.org/wiki/Hot_Standby_TODO
   
   Patch now includes a set of regression tests that can be run against a
   standby server using "make standbycheck" (requires setup, see
   src/test/regress/standby_schedule). This helps make explicit in code
   which commands and variants are allowed and disallowed in hot standby.
   
   Complete with full docs and comments.
   
   Barring resolving a few points and subject to even more testing, this is
   the version I expect to commit to CVS on Wednesday. I would appreciate
   further objective testing before commit, if possible.
   
   Last remaining points
   
   * NonTransactionalInvalidation logging has been removed following
   review, but AFAICS that means VACUUM FULL doesn't work correctly on
   catalog tables, which regrettably will be the only ones still standing
   even after we apply VFI patch. Did I misunderstand the original intent?
   Was it just buggy somehow? Or is this hoping VF goes completely, which
   seems unlikely in this release. Just noticed this, decided better to get
   stuff out there now.
   
   * Are we OK with using hash indexes in standby plans, even when we know
   the indexes are stale and could return incorrect results?
   
   Software also available via git on the repo used by Heikki and myself.
     ssh://git@git.postgresql.org/users/heikki/postgres.git
   branch = hs-riggs
   
   Patch prepared using from my private repo using
     git diff pgsql/master..hs-simon | filterdiff --format=context
   so please look out for any weirdness that might introduce. 
   
   -- 
    Simon Riggs           www.2ndQuadrant.com
   

2. Re: Hot Standby, release candidate?

   Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-13T20:45:54Z

   Simon Riggs <simon@2ndQuadrant.com> writes:
   > * NonTransactionalInvalidation logging has been removed following
   > review, but AFAICS that means VACUUM FULL doesn't work correctly on
   > catalog tables, which regrettably will be the only ones still standing
   > even after we apply VFI patch. Did I misunderstand the original intent?
   > Was it just buggy somehow? Or is this hoping VF goes completely, which
   > seems unlikely in this release.
   
   For my money, the only reason VF is still around is there hasn't been
   an urgent reason to get rid of it.  If it doesn't play with HS, I think
   we'd be better served to put work into getting rid of it than to put
   work into fixing it.
   
   			regards, tom lane
   
   

3. Re: Hot Standby, release candidate?

   Simon Riggs <simon@2ndquadrant.com> — 2009-12-13T22:22:06Z

   On Sun, 2009-12-13 at 15:45 -0500, Tom Lane wrote:
   > Simon Riggs <simon@2ndQuadrant.com> writes:
   > > * NonTransactionalInvalidation logging has been removed following
   > > review, but AFAICS that means VACUUM FULL doesn't work correctly on
   > > catalog tables, which regrettably will be the only ones still standing
   > > even after we apply VFI patch. Did I misunderstand the original intent?
   > > Was it just buggy somehow? Or is this hoping VF goes completely, which
   > > seems unlikely in this release.
   > 
   > For my money, the only reason VF is still around is there hasn't been
   > an urgent reason to get rid of it.  If it doesn't play with HS, I think
   > we'd be better served to put work into getting rid of it than to put
   > work into fixing it.
   
   I see the logic, though it has many implications. I'll step up, if I can
   get some help from you and Itagaki on the VF side.
   
   You have a rough design here
   http://archives.postgresql.org/message-id/19750.1252094460@sss.pgh.pa.us
   
   Some thoughts and some further work on a detailed design
   
   * Which exact tables are we talking about: just pg_class and the shared
   catalogs? Everything else is in pg_class, so if we can find it we're OK?
   formrdesc() tells me the list of nailed relations is: pg_database,
   pg_class, pg_attribute, pg_proc, and pg_type. Are the nailed relations
   the ones we care about, or are they just a subset? 
   
   * Restrict set of operations to *only* VACUUM FULL. Is there a need for
   anything else to do this, at least in this release?
   
   * Each backend needs to access two map files: shared and local
   
   * Get relcache to read map files at startup in formrdesc(). Rather than
   use RelationInitPhysicalAddr() set relation->rd_node.relNode directly
   
   * Get VF to write a new type of invalidation message that means re-read
   the two map files to overwrite the relation->rd_node.relNode in the
   nailed relations
   
   * Map files would have a very structured format, so each table listed
   has its exact place. Sounds like best place for shared catalogs is
   pg_control. We only need a few additional bytes for that and everything
   else to manipulate it already exists.
   
   * Map files for specific databases would be called pg_database_control,
   with roughly same concepts as pg_control. It's then an obvious place to
   add any further db specific things in future, if we need them.
   
   * Protect all map files reading/writing using ControlFileLock. Sequence
   of update is acquire lock, send invalidation, rewrite file, release lock
   all inside a critical section. Readers would take shared, writers
   exclusive.
   
   * Work would be in two tranches: add new way of working then later
   remove code we don't need; I would actually rather do the second part at
   start of next dev cycle.
   
   -- 
    Simon Riggs           www.2ndQuadrant.com
   
   
   

4. Re: Hot Standby, release candidate?

   Peter Eisentraut <peter_e@gmx.net> — 2009-12-14T08:11:54Z

   On sön, 2009-12-13 at 19:20 +0000, Simon Riggs wrote:
   > Barring resolving a few points and subject to even more testing, this
   > is the version I expect to commit to CVS on Wednesday.
   
   To clarify: Did you pick Wednesday so that it gets included before the
   end of the commit fest (and thus into alpha3) or so that it gets into
   CVS as early as possible after the commit fest (and thus not into
   alpha3)?
   
   
   

5. Re: Hot Standby, release candidate?

   Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T08:54:23Z

   On Mon, 2009-12-14 at 10:11 +0200, Peter Eisentraut wrote:
   > On sön, 2009-12-13 at 19:20 +0000, Simon Riggs wrote:
   > > Barring resolving a few points and subject to even more testing, this
   > > is the version I expect to commit to CVS on Wednesday.
   > 
   > To clarify: Did you pick Wednesday so that it gets included before the
   > end of the commit fest (and thus into alpha3) or so that it gets into
   > CVS as early as possible after the commit fest (and thus not into
   > alpha3)?
   
   Wednesday because that seemed a good delay to allow review. Josh and I
   had discussed the value of getting patch into Alpha3, so that was my
   wish and aim.
   
   I'm not aware of any particular date for end of commitfest, though
   possibly you are about to update me on that?
   
   (Perhaps we really need a single Project Management page that lists all
   the dates that have been agreed, so that everybody can go there and be
   clear. Commitfest start and end dates, beta dates, de-support dates etc.
   BTW, it is absolutely brilliant that we have these now.)
   
   -- 
    Simon Riggs           www.2ndQuadrant.com
   
   
   

6. Re: Hot Standby, release candidate?

   Peter Eisentraut <peter_e@gmx.net> — 2009-12-14T09:44:07Z

   On mån, 2009-12-14 at 08:54 +0000, Simon Riggs wrote:
   > Wednesday because that seemed a good delay to allow review. Josh and I
   > had discussed the value of getting patch into Alpha3, so that was my
   > wish and aim.
   > 
   > I'm not aware of any particular date for end of commitfest, though
   > possibly you are about to update me on that?
   
   Commit fests for 8.5 have usually run from the 15th to the 15th of next
   month, but the CF manager may choose to vary that.
   
   FWIW, the alpha release manager may also vary the release timeline of
   alpha3. ;-)
   
   > (Perhaps we really need a single Project Management page that lists all
   > the dates that have been agreed, so that everybody can go there and be
   > clear. Commitfest start and end dates, beta dates, de-support dates etc.
   > BTW, it is absolutely brilliant that we have these now.)
   
   We had
   <http://wiki.postgresql.org/wiki/PostgreSQL_8.4_Development_Plan>, but I
   don't think we ever actually agreed on a schedule for 8.5.
   
   
   
   

7. Re: Hot Standby, release candidate?

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-12-14T09:54:38Z

   Simon Riggs wrote:
   > Enclose latest version of Hot Standby. This is the "basic" patch, with
   > no must-fix issues and no known bugs. Further additions will follow
   > after commit, primarily around usability, which will include additional
   > control functions for use in testing. Various thoughts discussed here
   > http://wiki.postgresql.org/wiki/Hot_Standby_TODO
   
   I still consider it highly important to be able to start standby from a
   shutdown checkpoint. If you remove it, at the very least put it back on
   the TODO.
   
   But as it is, StandbyRecoverPreparedTransactions() is dead code, and the
   changes to PrescanPreparedTransactions() are not needed either.
   
   > Patch now includes a set of regression tests that can be run against a
   > standby server using "make standbycheck"
   
   Nice!
   
   > (requires setup, see src/test/regress/standby_schedule). 
   
   Any chance of automating that?
   
   > Complete with full docs and comments.
   > 
   > Barring resolving a few points and subject to even more testing, this is
   > the version I expect to commit to CVS on Wednesday. I would appreciate
   > further objective testing before commit, if possible.
   
   * Please remove any spurious whitespace.  "git diff --color" makes them
   stand out like a sore thumb, in red. (pgindent will fix them but always
   better to fix them before committing, IMO).
   
   * vacuum_defer_cleanup_age is very hard to tune. You'll need an estimate
   on your transaction rate to begin with. Do we really want this setting
   in its current form? Does it make sense as PGC_USERSET, as if one
   backend uses a lower setting than others, that's the one that really
   determines when transactions are killed in the standby? I think this
   should be discussed and implemented as a separate patch.
   
   * Are you planning to remove the recovery_connections setting before
   release? The documentation makes it sound like it's a temporary hack
   that we're not really sure is needed at all. That's not very comforting.
   
   * You removed this comment from KnownAssignedXidsInit:
   
   -   /*
   -    * XXX: We should check that we don't exceed maxKnownAssignedXids.
   -    * Even though the hash table might hold a few more entries than that,
   -    * we use fixed-size arrays of that size elsewhere and expected all
   -    * entries in the hash table to fit.
   -    */
   
   but AFAICS you didn't address the issue. It's referring to the 'xids'
   array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
   in without checking that it fits.
   
   * LockAcquireExtended needs a function comment. Or at least something
   explaining what report_memory_error does. And perhaps the argument
   should be named "reportMemoryError" to be in line with the other arguments.
   
   * We tend to not add remarks about authors in code (comments in standby.c).
   
   * This optimization in GetConflictingVirtualXIDs():
   
   > +   /*
   > +    * If we don't know the TransactionId that created the conflict, set
   > +    * it to latestCompletedXid which is the latest possible value.
   > +    */
   > +   if (!TransactionIdIsValid(limitXmin))
   > +       limitXmin = ShmemVariableCache->latestCompletedXid;
   > +
   
   needs a lot more explanation. It took me a very long time to figure out
   why using latest completed xid is safe.
   
   * Are you going to leave the trace_recovery GUC in?
   
   * Can you merge with CVS HEAD, please? There's some merge conflicts.
   
   > Last remaining points
   > 
   > * NonTransactionalInvalidation logging has been removed following
   > review, but AFAICS that means VACUUM FULL doesn't work correctly on
   > catalog tables, which regrettably will be the only ones still standing
   > even after we apply VFI patch. Did I misunderstand the original intent?
   > Was it just buggy somehow? Or is this hoping VF goes completely, which
   > seems unlikely in this release. Just noticed this, decided better to get
   > stuff out there now.
   
   I removed it in the hope that VF is gone before beta.
   
   > * Are we OK with using hash indexes in standby plans, even when we know
   > the indexes are stale and could return incorrect results?
   
   It doesn't seem any more wrong than using hash indexes right after
   recovery, crash recovery or otherwise. It's certainly broken, but I
   don't see much value in a partial fix. The bottom line is that hash
   indexes should be WAL-logged.
   
   -- 
     Heikki Linnakangas
     EnterpriseDB   http://www.enterprisedb.com
   
   

8. Re: Hot Standby, release candidate?

   Magnus Hagander <magnus@hagander.net> — 2009-12-14T10:09:49Z

   On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
   <heikki.linnakangas@enterprisedb.com> wrote:
   > * Please remove any spurious whitespace.  "git diff --color" makes them
   > stand out like a sore thumb, in red. (pgindent will fix them but always
   > better to fix them before committing, IMO).
   
   +1 in general, not particularly for this patch (haven't checked that
   in this patch).
   
   Actually, how about we add that to the page at
   http://wiki.postgresql.org/wiki/Submitting_a_Patch?
   
   
   -- 
    Magnus Hagander
    Me: http://www.hagander.net/
    Work: http://www.redpill-linpro.com/
   
   

9. Re: Hot Standby, release candidate?

   Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T11:07:32Z

   Thanks for the further review, much appreciated.
   
   On Mon, 2009-12-14 at 11:54 +0200, Heikki Linnakangas wrote:
   > Simon Riggs wrote:
   > > Enclose latest version of Hot Standby. This is the "basic" patch, with
   > > no must-fix issues and no known bugs. Further additions will follow
   > > after commit, primarily around usability, which will include additional
   > > control functions for use in testing. Various thoughts discussed here
   > > http://wiki.postgresql.org/wiki/Hot_Standby_TODO
   > 
   > I still consider it highly important to be able to start standby from a
   > shutdown checkpoint. If you remove it, at the very least put it back on
   > the TODO.
   
   Happy to put it back on TODO, but I'm not likely to do this without a
   good reason. IMHO your arguments as to why that is useful were not
   convincing, especially when it introduces further complications and
   requirements for tests.
   
   > But as it is, StandbyRecoverPreparedTransactions() is dead code, and the
   > changes to PrescanPreparedTransactions() are not needed either.
   > 
   > > Patch now includes a set of regression tests that can be run against a
   > > standby server using "make standbycheck"
   > 
   > Nice!
   > 
   > > (requires setup, see src/test/regress/standby_schedule). 
   > 
   > Any chance of automating that?
   
   Future, yes. 
   
   I view standbycheck as similar to installcheck - it requires some setup
   before it can run, so allows you to test an existing server. I see the
   same need here.
   
   > > Complete with full docs and comments.
   > > 
   > > Barring resolving a few points and subject to even more testing, this is
   > > the version I expect to commit to CVS on Wednesday. I would appreciate
   > > further objective testing before commit, if possible.
   > 
   > * Please remove any spurious whitespace.  "git diff --color" makes them
   > stand out like a sore thumb, in red. (pgindent will fix them but always
   > better to fix them before committing, IMO).
   
   What is your definition of spurious whitespace? I removed all
   additions/deletions of individual lines. git diff colours many things
   red, and it seems like a waste of time to spend hours fiddling with
   spaces manually if there is a utility that does this anyway.
   
   > * vacuum_defer_cleanup_age is very hard to tune. You'll need an estimate
   > on your transaction rate to begin with. Do we really want this setting
   > in its current form? Does it make sense as PGC_USERSET, as if one
   > backend uses a lower setting than others, that's the one that really
   > determines when transactions are killed in the standby? I think this
   > should be discussed and implemented as a separate patch.
   
   All the vacuum_*_age parameters have this characteristic, yet they
   exist.
   
   I would like to provide simple features for conflict avoidance in the
   first alpha release. If we find that nobody found it useful then it can
   be removed easily enough.
   
   It's a USERSET now, but it could also be other things. This patch isn't
   the end of discussion, for many people it will be the start.
   
   > * Are you planning to remove the recovery_connections setting before
   > release? The documentation makes it sound like it's a temporary hack
   > that we're not really sure is needed at all. That's not very comforting.
   
   It has been requested and I agree, so its there. Saying it might be
   removed in future is no more than we do elsewhere and AFAIK we all hope
   it will be. Not sure why that is or isn't comforting.
   
   > * You removed this comment from KnownAssignedXidsInit:
   > 
   > -   /*
   > -    * XXX: We should check that we don't exceed maxKnownAssignedXids.
   > -    * Even though the hash table might hold a few more entries than that,
   > -    * we use fixed-size arrays of that size elsewhere and expected all
   > -    * entries in the hash table to fit.
   > -    */
   > 
   > but AFAICS you didn't address the issue. It's referring to the 'xids'
   > array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
   > in without checking that it fits.
   
   I have ensured that they are always the same size, by definition, so no
   need to check.
   
   > * LockAcquireExtended needs a function comment. Or at least something
   > explaining what report_memory_error does. And perhaps the argument
   > should be named "reportMemoryError" to be in line with the other arguments.
   
   OK
   
   > * We tend to not add remarks about authors in code (comments in standby.c).
   
   OK
   
   > * This optimization in GetConflictingVirtualXIDs():
   > 
   > > +   /*
   > > +    * If we don't know the TransactionId that created the conflict, set
   > > +    * it to latestCompletedXid which is the latest possible value.
   > > +    */
   > > +   if (!TransactionIdIsValid(limitXmin))
   > > +       limitXmin = ShmemVariableCache->latestCompletedXid;
   > > +
   > 
   > needs a lot more explanation. It took me a very long time to figure out
   > why using latest completed xid is safe.
   
   OK. Took me a long time as well.
   
   > * Are you going to leave the trace_recovery GUC in?
   
   For now, at least. I have a later proposal around that to follow.
   
   > * Can you merge with CVS HEAD, please? There's some merge conflicts.
   
   Huh? I did. And tested patch against a CVS checkout before submitting.
   Can you explain further?
   
   > > Last remaining points
   > > 
   > > * NonTransactionalInvalidation logging has been removed following
   > > review, but AFAICS that means VACUUM FULL doesn't work correctly on
   > > catalog tables, which regrettably will be the only ones still standing
   > > even after we apply VFI patch. Did I misunderstand the original intent?
   > > Was it just buggy somehow? Or is this hoping VF goes completely, which
   > > seems unlikely in this release. Just noticed this, decided better to get
   > > stuff out there now.
   > 
   > I removed it in the hope that VF is gone before beta.
   
   OK
   
   > > * Are we OK with using hash indexes in standby plans, even when we know
   > > the indexes are stale and could return incorrect results?
   > 
   > It doesn't seem any more wrong than using hash indexes right after
   > recovery, crash recovery or otherwise. It's certainly broken, but I
   > don't see much value in a partial fix. The bottom line is that hash
   > indexes should be WAL-logged.
   
   I know that's your thought; I'm just checking its everyone else's as
   well. We go to great lengths elsewhere in the patch to avoid queries
   returning incorrect results and there is a loss of capability as a
   result. I don't want Hash index users to view this as a feature. I don't
   feel too strongly, but it can be argued both ways, at least.
   
   -- 
    Simon Riggs           www.2ndQuadrant.com
   
   
   

10. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T11:11:46Z

    On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
    > On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
    > <heikki.linnakangas@enterprisedb.com> wrote:
    > > * Please remove any spurious whitespace.  "git diff --color" makes them
    > > stand out like a sore thumb, in red. (pgindent will fix them but always
    > > better to fix them before committing, IMO).
    > 
    > +1 in general, not particularly for this patch (haven't checked that
    > in this patch).
    > 
    > Actually, how about we add that to the page at
    > http://wiki.postgresql.org/wiki/Submitting_a_Patch?
    
    If we can define "spurious whitespace" it would help decide whether
    there is any action to take, and when.
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

11. Re: Hot Standby, release candidate?

    Fujii Masao <masao.fujii@gmail.com> — 2009-12-14T11:15:47Z

    On Mon, Dec 14, 2009 at 8:07 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
    >> * Please remove any spurious whitespace.  "git diff --color" makes them
    >> stand out like a sore thumb, in red. (pgindent will fix them but always
    >> better to fix them before committing, IMO).
    >
    > What is your definition of spurious whitespace? I removed all
    > additions/deletions of individual lines. git diff colours many things
    > red, and it seems like a waste of time to spend hours fiddling with
    > spaces manually if there is a utility that does this anyway.
    
    I guess it's a trailing whitespace. How about using "git diff --check"
    instead of "--color"?
    
    Regards,
    
    -- 
    Fujii Masao
    NIPPON TELEGRAPH AND TELEPHONE CORPORATION
    NTT Open Source Software Center
    
    

12. Re: Hot Standby, release candidate?

    Robert Haas <robertmhaas@gmail.com> — 2009-12-14T11:21:44Z

    On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
    >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
    >> <heikki.linnakangas@enterprisedb.com> wrote:
    >> > * Please remove any spurious whitespace.  "git diff --color" makes them
    >> > stand out like a sore thumb, in red. (pgindent will fix them but always
    >> > better to fix them before committing, IMO).
    >>
    >> +1 in general, not particularly for this patch (haven't checked that
    >> in this patch).
    >>
    >> Actually, how about we add that to the page at
    >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
    >
    > If we can define "spurious whitespace" it would help decide whether
    > there is any action to take, and when.
    
    git defines it as either (1) extra whitespace at the end of a line or
    (2) an initial indent that uses spaces followed by tabs (typically
    something like space-tab, where tab alone would have produced the same
    result).  git diff --check master tends to be useful here.
    
    ...Robert
    
    

13. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T11:35:25Z

    On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
    > On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    > > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
    > >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
    > >> <heikki.linnakangas@enterprisedb.com> wrote:
    > >> > * Please remove any spurious whitespace.  "git diff --color" makes them
    > >> > stand out like a sore thumb, in red. (pgindent will fix them but always
    > >> > better to fix them before committing, IMO).
    > >>
    > >> +1 in general, not particularly for this patch (haven't checked that
    > >> in this patch).
    > >>
    > >> Actually, how about we add that to the page at
    > >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
    > >
    > > If we can define "spurious whitespace" it would help decide whether
    > > there is any action to take, and when.
    > 
    > git defines it as either (1) extra whitespace at the end of a line or
    > (2) an initial indent that uses spaces followed by tabs (typically
    > something like space-tab, where tab alone would have produced the same
    > result).  git diff --check master tends to be useful here.
    
    (2) is a problem that has been discussed before on hackers, anything
    like that should be changed.
    
    Why is (1) important, and if it is important, why is it being mentioned
    only now? Are we saying that all previous reviewers of my work (and
    others') removed these without ever mentioning they had done so?
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

14. Re: Hot Standby, release candidate?

    Robert Haas <robertmhaas@gmail.com> — 2009-12-14T11:51:43Z

    On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    > On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
    >> On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    >> > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
    >> >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
    >> >> <heikki.linnakangas@enterprisedb.com> wrote:
    >> >> > * Please remove any spurious whitespace.  "git diff --color" makes them
    >> >> > stand out like a sore thumb, in red. (pgindent will fix them but always
    >> >> > better to fix them before committing, IMO).
    >> >>
    >> >> +1 in general, not particularly for this patch (haven't checked that
    >> >> in this patch).
    >> >>
    >> >> Actually, how about we add that to the page at
    >> >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
    >> >
    >> > If we can define "spurious whitespace" it would help decide whether
    >> > there is any action to take, and when.
    >>
    >> git defines it as either (1) extra whitespace at the end of a line or
    >> (2) an initial indent that uses spaces followed by tabs (typically
    >> something like space-tab, where tab alone would have produced the same
    >> result).  git diff --check master tends to be useful here.
    >
    > (2) is a problem that has been discussed before on hackers, anything
    > like that should be changed.
    >
    > Why is (1) important, and if it is important, why is it being mentioned
    > only now? Are we saying that all previous reviewers of my work (and
    > others') removed these without ever mentioning they had done so?
    
    pgident will remove such white spaces and create merge conflicts for
    everyone working on those areas of the code.  I certainly mention this
    in any review I do where it's applicable, and have been doing so for
    some time.  I also will certainly fix it for any code I commit.  I
    also mentioned it in the review that I did of Hot Standby.
    
    ...Robert
    
    

15. Re: Hot Standby, release candidate?

    Magnus Hagander <magnus@hagander.net> — 2009-12-14T12:08:34Z

    On Mon, Dec 14, 2009 at 12:51, Robert Haas <robertmhaas@gmail.com> wrote:
    > On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    >> On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
    >>> On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    >>> > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
    >>> >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
    >>> >> <heikki.linnakangas@enterprisedb.com> wrote:
    >>> >> > * Please remove any spurious whitespace.  "git diff --color" makes them
    >>> >> > stand out like a sore thumb, in red. (pgindent will fix them but always
    >>> >> > better to fix them before committing, IMO).
    >>> >>
    >>> >> +1 in general, not particularly for this patch (haven't checked that
    >>> >> in this patch).
    >>> >>
    >>> >> Actually, how about we add that to the page at
    >>> >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
    >>> >
    >>> > If we can define "spurious whitespace" it would help decide whether
    >>> > there is any action to take, and when.
    >>>
    >>> git defines it as either (1) extra whitespace at the end of a line or
    >>> (2) an initial indent that uses spaces followed by tabs (typically
    >>> something like space-tab, where tab alone would have produced the same
    >>> result).  git diff --check master tends to be useful here.
    >>
    >> (2) is a problem that has been discussed before on hackers, anything
    >> like that should be changed.
    >>
    >> Why is (1) important, and if it is important, why is it being mentioned
    >> only now? Are we saying that all previous reviewers of my work (and
    >> others') removed these without ever mentioning they had done so?
    >
    > pgident will remove such white spaces and create merge conflicts for
    > everyone working on those areas of the code.  I certainly mention this
    > in any review I do where it's applicable, and have been doing so for
    > some time.  I also will certainly fix it for any code I commit.  I
    > also mentioned it in the review that I did of Hot Standby.
    
    I also always do this when committing other peoples patches (which I
    don't do as often as I should, but when I *do* do it..)
    
    
    -- 
     Magnus Hagander
     Me: http://www.hagander.net/
     Work: http://www.redpill-linpro.com/
    
    

16. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T12:22:57Z

    On Mon, 2009-12-14 at 11:07 +0000, Simon Riggs wrote:
    > Thanks for the further review, much appreciated.
    > 
    > On Mon, 2009-12-14 at 11:54 +0200, Heikki Linnakangas wrote:
    > > Simon Riggs wrote:
    > > > Enclose latest version of Hot Standby. 
    >  
    > > * LockAcquireExtended needs a function comment. Or at least something
    > > explaining what report_memory_error does. And perhaps the argument
    > > should be named "reportMemoryError" to be in line with the other arguments.
    > 
    > OK
    
    Done
    
    > > * We tend to not add remarks about authors in code (comments in standby.c).
    > 
    > OK
    
    Done
    
    > > * This optimization in GetConflictingVirtualXIDs():
    > > 
    > > > +   /*
    > > > +    * If we don't know the TransactionId that created the conflict, set
    > > > +    * it to latestCompletedXid which is the latest possible value.
    > > > +    */
    > > > +   if (!TransactionIdIsValid(limitXmin))
    > > > +       limitXmin = ShmemVariableCache->latestCompletedXid;
    > > > +
    > > 
    > > needs a lot more explanation. It took me a very long time to figure out
    > > why using latest completed xid is safe.
    > 
    > OK. Took me a long time as well.
    
    Done
    
    > > * Can you merge with CVS HEAD, please? There's some merge conflicts.
    > 
    > Huh? I did. And tested patch against a CVS checkout before submitting.
    > Can you explain further?
    
    Still not sure what conflicts you see or where they might come from...
    
    I am now unable to push these changes to the shared repo. What is
    happening?
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

17. Re: Hot Standby, release candidate?

    Robert Haas <robertmhaas@gmail.com> — 2009-12-14T12:33:28Z

    On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    > I am now unable to push these changes to the shared repo. What is
    > happening?
    
    Perhaps you need to run cvs update on your local copy?
    
    (I find this flavor the most useful: "cvs -q update -d"  YMMV.)
    
    If that's not it, error message?
    
    ...Robert
    
    

18. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T12:42:49Z

    On Mon, 2009-12-14 at 06:51 -0500, Robert Haas wrote:
    > On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    > > On Mon, 2009-12-14 at 06:21 -0500, Robert Haas wrote:
    > >> On Mon, Dec 14, 2009 at 6:11 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    > >> > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
    > >> >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
    > >> >> <heikki.linnakangas@enterprisedb.com> wrote:
    > >> >> > * Please remove any spurious whitespace.  "git diff --color" makes them
    > >> >> > stand out like a sore thumb, in red. (pgindent will fix them but always
    > >> >> > better to fix them before committing, IMO).
    > >> >>
    > >> >> +1 in general, not particularly for this patch (haven't checked that
    > >> >> in this patch).
    > >> >>
    > >> >> Actually, how about we add that to the page at
    > >> >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
    > >> >
    > >> > If we can define "spurious whitespace" it would help decide whether
    > >> > there is any action to take, and when.
    > >>
    > >> git defines it as either (1) extra whitespace at the end of a line or
    > >> (2) an initial indent that uses spaces followed by tabs (typically
    > >> something like space-tab, where tab alone would have produced the same
    > >> result).  git diff --check master tends to be useful here.
    > >
    > > (2) is a problem that has been discussed before on hackers, anything
    > > like that should be changed.
    > >
    > > Why is (1) important, and if it is important, why is it being mentioned
    > > only now? Are we saying that all previous reviewers of my work (and
    > > others') removed these without ever mentioning they had done so?
    > 
    > pgident will remove such white spaces and create merge conflicts for
    > everyone working on those areas of the code.  I certainly mention this
    > in any review I do where it's applicable, and have been doing so for
    > some time.  I also will certainly fix it for any code I commit.  I
    > also mentioned it in the review that I did of Hot Standby.
    
    I don't recall you mentioning that.
    
    There are no changes in this patch that are purely whitespace changes.
    Those were removed prior to patch submission. This is all about code I
    am adding or changing. My additions may disrupt their patches, but not
    because of the whitespace.
    
    If we are going to run pgindent anyway, what is the point? Seems like we
    need a tool to fix patches, not a tool to fix the code.
    
    I've made some changes to the larger files.
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

19. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T12:47:51Z

    On Mon, 2009-12-14 at 07:33 -0500, Robert Haas wrote:
    > On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    > > I am now unable to push these changes to the shared repo. What is
    > > happening?
    > 
    > Perhaps you need to run cvs update on your local copy?
    > 
    > (I find this flavor the most useful: "cvs -q update -d"  YMMV.)
    > 
    > If that's not it, error message?
    
    It was a question for Heikki only, not a CVS issue.
    
    git push ssh://git@git.postgresql.org/users/heikki/postgres.git
    hs-simon:hs-riggs
    To ssh://git@git.postgresql.org/users/heikki/postgres.git
     ! [rejected]        hs-simon -> hs-riggs (non-fast forward)
    error: failed to push some refs to
    'ssh://git@git.postgresql.org/users/heikki/postgres.git'
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

20. Re: Hot Standby, release candidate?

    Magnus Hagander <magnus@hagander.net> — 2009-12-14T12:56:27Z

    On Mon, Dec 14, 2009 at 13:47, Simon Riggs <simon@2ndquadrant.com> wrote:
    > On Mon, 2009-12-14 at 07:33 -0500, Robert Haas wrote:
    >> On Mon, Dec 14, 2009 at 7:22 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    >> > I am now unable to push these changes to the shared repo. What is
    >> > happening?
    >>
    >> Perhaps you need to run cvs update on your local copy?
    >>
    >> (I find this flavor the most useful: "cvs -q update -d"  YMMV.)
    >>
    >> If that's not it, error message?
    >
    > It was a question for Heikki only, not a CVS issue.
    >
    > git push ssh://git@git.postgresql.org/users/heikki/postgres.git
    > hs-simon:hs-riggs
    > To ssh://git@git.postgresql.org/users/heikki/postgres.git
    >  ! [rejected]        hs-simon -> hs-riggs (non-fast forward)
    > error: failed to push some refs to
    > 'ssh://git@git.postgresql.org/users/heikki/postgres.git'
    
    Same issue can be it in git - did you do a "git pull" before? You may
    need merging with what's on there, and for that to work you must pull
    before you can push.
    
    -- 
     Magnus Hagander
     Me: http://www.hagander.net/
     Work: http://www.redpill-linpro.com/
    
    

21. Re: Hot Standby, release candidate?

    Robert Haas <robertmhaas@gmail.com> — 2009-12-14T14:32:25Z

    On Mon, Dec 14, 2009 at 7:42 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    > There are no changes in this patch that are purely whitespace changes.
    > Those were removed prior to patch submission. This is all about code I
    > am adding or changing. My additions may disrupt their patches, but not
    > because of the whitespace.
    >
    > If we are going to run pgindent anyway, what is the point? Seems like we
    > need a tool to fix patches, not a tool to fix the code.
    >
    > I've made some changes to the larger files.
    
    I'll try to explain this again because it is a little bit subtle.
    
    Whenever someone commits ANY patch, there is a danger of disrupting
    other outstanding patches that touch the same code.  That's basically
    unavoidable, although certainly it's a reason to avoid superfluous
    changes like whitespace adjustments or useless identifier renaming.
    
    However, there's a second, completely independent issue, which is that
    eventually we will run pgindent for 8.5, and when we do that, it's
    going to reformat stuff, and that reformatting is going to break
    things.  The amount of reformatting that it does (and therefore the
    amount of breakage that it causes) is directly related to the extent
    to which people have committed patches throughout the release cycle
    that don't conform to the layout that pgident is going to want.  If
    every patch perfectly matched the pgident style, then the pgindent run
    would change nothing and we would all be VERY happy.  The more
    deviations there are, the more stuff breaks.
    
    So I agree with you: we need a tool to fix patches.  However, since we
    haven't got one, we owe it to other contributors not to make the
    problem any worse than necessary by adhering to the project's
    formatting conventions as best we're able when committing things,
    especially with regards to trivial things like trailing white-space
    that git diff --check can easily find.  Actually, git apply has an
    option to fix these simple types of problems, so it's possible to fix
    it diffing the patch set against the master branch, applying it to a
    separate copy of the master branch using git apply --whitespace=fix,
    then diffing that against the original batch and applying the changes.
     Although that's usually overkill unless it's really bad.
    
    There's some interesting discussion on whitespace more generally from
    Tom Lane here:
    
    http://archives.postgresql.org/pgsql-hackers/2009-08/msg01001.php
    
    ...Robert
    
    

22. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T15:08:49Z

    On Mon, 2009-12-14 at 09:32 -0500, Robert Haas wrote:
    
    > If every patch perfectly matched the pgident style, then the pgindent
    > run would change nothing and we would all be VERY happy.
    
    I've made all whitespace changes to the patch.
    
    I understand the reason for acting as you suggest, but we either police
    it, or we don't. If we don't police in all cases then I'm not personally
    happy to be policed.
    
    About 90% of the whitespace in the patch were in docs, README or test
    output files. A great many of that type of file have numerous line
    ending whitespace, not introduced by me, so it seems to me that this has
    never been adequately policed in the way you say. If we really do care
    about this issue then I expect people to look a little further than just
    my patch.
    
    Portability of patches across releases isn't a huge issue for me, nor
    for the project, I think, but I am willing to commit to cleaning future
    patches in this way.
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

23. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T15:12:13Z

    On Mon, 2009-12-14 at 13:56 +0100, Magnus Hagander wrote:
    
    > Same issue can be it in git - did you do a "git pull" before? You may
    > need merging with what's on there, and for that to work you must pull
    > before you can push.
    
    Found some merge conflicts and resolved them. I did fetch and merge at
    different times, so that seems to be the source.
    
    I've resolved the other git issues, so latest version on git now.
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

24. Re: Hot Standby, release candidate?

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2009-12-14T15:14:23Z

    Simon Riggs <simon@2ndQuadrant.com> wrote:
     
    > If we are going to run pgindent anyway, what is the point?
     
    Perhaps it would take less time to run this than to argue the point?:
     
    sed -e 's/[ \t][ \t]*$//' -e 's/  *\t/\t/g' *
     
    -Kevin
    
    

25. Re: Hot Standby, release candidate?

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-14T15:23:26Z

    Robert Haas <robertmhaas@gmail.com> writes:
    > On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    >> Why is (1) important, and if it is important, why is it being mentioned
    >> only now? Are we saying that all previous reviewers of my work (and
    >> others') removed these without ever mentioning they had done so?
    
    > pgident will remove such white spaces and create merge conflicts for
    > everyone working on those areas of the code.
    
    What I try really hard to remove from committed patches is spurious
    whitespace changes to pre-existing code.  Whether new code blocks
    exactly match pgindent's rules is less of a concern, but changing
    code you don't have to in a way that pgindent will undo later anyway
    is just useless creation of potential conflicts.
    
    The whole thing would be a lot easier if someone would put together an
    easily-installable version of pgindent.  Bruce has posted the patches he
    uses but I don't know what version of indent they're against...
    
    			regards, tom lane
    
    

26. Re: Hot Standby, release candidate?

    Robert Haas <robertmhaas@gmail.com> — 2009-12-14T15:24:31Z

    On Mon, Dec 14, 2009 at 10:08 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    > On Mon, 2009-12-14 at 09:32 -0500, Robert Haas wrote:
    >> If every patch perfectly matched the pgident style, then the pgindent
    >> run would change nothing and we would all be VERY happy.
    >
    > I've made all whitespace changes to the patch.
    >
    > I understand the reason for acting as you suggest, but we either police
    > it, or we don't. If we don't police in all cases then I'm not personally
    > happy to be policed.
    
    I am doing my best to police it, and certainly will police it for
    anything that I review or commit.  Heikki was the one who originally
    pointed it on this thread, Magnus gave a +1, and I am pretty sure Tom
    tries to keep an eye out for it, too, so generally I would say it is
    project practice.  Obviously I am not able to control the actions of
    all the other committers, and it does appear that some crap has crept
    in since the last pgindent run.  :-(
    
    At any rate I don't think you're being singled out for special treatment.
    
    > About 90% of the whitespace in the patch were in docs, README or test
    > output files. A great many of that type of file have numerous line
    > ending whitespace, not introduced by me, so it seems to me that this has
    > never been adequately policed in the way you say. If we really do care
    > about this issue then I expect people to look a little further than just
    > my patch.
    
    pgindent won't reindent the docs or the README, but git diff --check
    picks up on trailing whitespace, so it may be that Tom (who doesn't
    use git but does commit a lot of patches) is less finicky about
    trailing whitespace in those places.  If we move to git across the
    board, I expect this to get more standardized handling, but I think we
    have a ways to go on that yet.
    
    > Portability of patches across releases isn't a huge issue for me, nor
    > for the project, I think, but I am willing to commit to cleaning future
    > patches in this way.
    
    It's a pretty significant issue for me personally, and anyone who is
    maintaining a fork of the main source base that has to be merged when
    the pgindent run hits.  It would be nice if someone wanted to build a
    better mousetrap here, but so far no volunteers.
    
    ...Robert
    
    

27. Re: Hot Standby, release candidate?

    Alvaro Herrera <alvherre@commandprompt.com> — 2009-12-14T15:25:20Z

    Tom Lane escribió:
    
    > The whole thing would be a lot easier if someone would put together an
    > easily-installable version of pgindent.  Bruce has posted the patches he
    > uses but I don't know what version of indent they're against...
    
    And we're still unclear on the typedef list that's going to be used.
    
    -- 
    Alvaro Herrera                                http://www.CommandPrompt.com/
    The PostgreSQL Company - Command Prompt, Inc.
    
    

28. Re: Hot Standby, release candidate?

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-12-14T15:27:37Z

    Simon Riggs wrote:
    > On Mon, 2009-12-14 at 11:09 +0100, Magnus Hagander wrote:
    >> On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
    >> <heikki.linnakangas@enterprisedb.com> wrote:
    >>> * Please remove any spurious whitespace.  "git diff --color" makes them
    >>> stand out like a sore thumb, in red. (pgindent will fix them but always
    >>> better to fix them before committing, IMO).
    >> +1 in general, not particularly for this patch (haven't checked that
    >> in this patch).
    >>
    >> Actually, how about we add that to the page at
    >> http://wiki.postgresql.org/wiki/Submitting_a_Patch?
    > 
    > If we can define "spurious whitespace" it would help decide whether
    > there is any action to take, and when.
    
    There's two definitions that are useful:
    
    - Anything that "git diff --color" shows up as glaring red. Important to
    remove simply because the red whitespace is distracting when I review a
    patch.
    
    - Anything that pgindent would/will eventually fix. Because you might as
    well fix them before committing and save the extra churn and potential
    (although trivial) merge conflicts in people's outstanding patches when
    pgindent is run. I don't run pgindent myself, so I wouldn't notice most
    stuff, but I tend to fix things that I do notice.
    
    > Why is (1) important, and if it is important, why is it being mentioned
    > only now? Are we saying that all previous reviewers of my work (and
    > others') removed these without ever mentioning they had done so?
    
    I did it in one pass, Oct 15th according to git log.
    
    I tend to silently fix whitespace issues like that in all patches I
    commit. It's generally trivial enough to fix that it's easier to just
    fix it myself than complain, explain, and look like a real nitpick.
    
    I note that if it was easy to run pgindent yourself on a patch before
    committing/submitting, we wouldn't need to have this discussion. I don't
    know hard it is to get it working right, but I recall I tried once and
    gave up. Any volunteers?
    
    -- 
      Heikki Linnakangas
      EnterpriseDB   http://www.enterprisedb.com
    
    

29. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T15:49:46Z

    On Mon, 2009-12-14 at 09:14 -0600, Kevin Grittner wrote:
    > Simon Riggs <simon@2ndQuadrant.com> wrote:
    >  
    > > If we are going to run pgindent anyway, what is the point?
    >  
    > Perhaps it would take less time to run this than to argue the point?:
    >  
    > sed -e 's/[ \t][ \t]*$//' -e 's/  *\t/\t/g' *
    
    Not certain that is exactly correct, plus it doesn't only work against a
    current patch since there are already many examples of whitespace in CVS
    already. I do appreciate your attempts at resolution and an easy
    tool-based approach for the future, though I'll let someone else run
    with it.
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

30. Re: Hot Standby, release candidate?

    David Fetter <david@fetter.org> — 2009-12-14T15:52:19Z

    On Mon, Dec 14, 2009 at 11:09:49AM +0100, Magnus Hagander wrote:
    > On Mon, Dec 14, 2009 at 10:54, Heikki Linnakangas
    > <heikki.linnakangas@enterprisedb.com> wrote:
    > > * Please remove any spurious whitespace.  "git diff --color" makes
    > > them stand out like a sore thumb, in red. (pgindent will fix them
    > > but always better to fix them before committing, IMO).
    > 
    > +1 in general, not particularly for this patch (haven't checked that
    > in this patch).
    > 
    > Actually, how about we add that to the page at
    > http://wiki.postgresql.org/wiki/Submitting_a_Patch?
    
    Done.
    
    Cheers,
    David.
    -- 
    David Fetter <david@fetter.org> http://fetter.org/
    Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
    Skype: davidfetter      XMPP: david.fetter@gmail.com
    iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
    
    Remember to vote!
    Consider donating to Postgres: http://www.postgresql.org/about/donate
    
    

31. Re: Hot Standby, release candidate?

    Robert Haas <robertmhaas@gmail.com> — 2009-12-14T15:53:04Z

    On Mon, Dec 14, 2009 at 10:49 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    > On Mon, 2009-12-14 at 09:14 -0600, Kevin Grittner wrote:
    >> Simon Riggs <simon@2ndQuadrant.com> wrote:
    >>
    >> > If we are going to run pgindent anyway, what is the point?
    >>
    >> Perhaps it would take less time to run this than to argue the point?:
    >>
    >> sed -e 's/[ \t][ \t]*$//' -e 's/  *\t/\t/g' *
    >
    > Not certain that is exactly correct, plus it doesn't only work against a
    > current patch since there are already many examples of whitespace in CVS
    > already. I do appreciate your attempts at resolution and an easy
    > tool-based approach for the future, though I'll let someone else run
    > with it.
    
    Yeah, that would actually be a disaster, because it would actually add
    deltas to the patch in the short term.
    
    Seems to me that we would be better off figuring out which exact ident
    Bruce is running and checking the typedef list into CVS.
    
    ...Robert
    
    

32. Re: Hot Standby, release candidate?

    Bruce Momjian <bruce@momjian.us> — 2009-12-14T16:24:01Z

    Tom Lane wrote:
    > Robert Haas <robertmhaas@gmail.com> writes:
    > > On Mon, Dec 14, 2009 at 6:35 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    > >> Why is (1) important, and if it is important, why is it being mentioned
    > >> only now? Are we saying that all previous reviewers of my work (and
    > >> others') removed these without ever mentioning they had done so?
    > 
    > > pgident will remove such white spaces and create merge conflicts for
    > > everyone working on those areas of the code.
    > 
    > What I try really hard to remove from committed patches is spurious
    > whitespace changes to pre-existing code.  Whether new code blocks
    > exactly match pgindent's rules is less of a concern, but changing
    > code you don't have to in a way that pgindent will undo later anyway
    > is just useless creation of potential conflicts.
    > 
    > The whole thing would be a lot easier if someone would put together an
    > easily-installable version of pgindent.  Bruce has posted the patches he
    > uses but I don't know what version of indent they're against...
    
    The entire indent tarball with patches is on our ftp site.
    
    -- 
      Bruce Momjian  <bruce@momjian.us>        http://momjian.us
      EnterpriseDB                             http://enterprisedb.com
    
      + If your life is a hard drive, Christ can be your backup. +
    
    

33. Re: Hot Standby, release candidate?

    Greg Smith <greg@2ndquadrant.com> — 2009-12-14T17:06:57Z

    Heikki Linnakangas wrote:
    > I note that if it was easy to run pgindent yourself on a patch before
    > committing/submitting, we wouldn't need to have this discussion. I don't
    > know hard it is to get it working right, but I recall I tried once and
    > gave up.
    >   
    What sort of problems did you run into?
    
    -- 
    Greg Smith    2ndQuadrant   Baltimore, MD
    PostgreSQL Training, Services and Support
    greg@2ndQuadrant.com  www.2ndQuadrant.com
    
    
    

34. Re: Hot Standby, release candidate?

    Greg Stark <gsstark@mit.edu> — 2009-12-14T18:02:56Z

    On Mon, Dec 14, 2009 at 11:07 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
    >> * vacuum_defer_cleanup_age is very hard to tune. You'll need an estimate
    >> on your transaction rate to begin with. Do we really want this setting
    >> in its current form? Does it make sense as PGC_USERSET, as if one
    >> backend uses a lower setting than others, that's the one that really
    >> determines when transactions are killed in the standby? I think this
    >> should be discussed and implemented as a separate patch.
    >
    > All the vacuum_*_age parameters have this characteristic, yet they
    > exist.
    
    I think it makes sense to have it be userset because someone could run
    vacuum on one table with a different defer_cleanup_age for some
    reason. I admit I'm having trouble coming up with a good use case.
    Personally I'm fine with having parameters like this in alphas that
    end up being renamed, or changed to different semantics, or even
    removed by the time it's launched.
    
    
    >> It doesn't seem any more wrong than using hash indexes right after
    >> recovery, crash recovery or otherwise. It's certainly broken, but I
    >> don't see much value in a partial fix. The bottom line is that hash
    >> indexes should be WAL-logged.
    >
    > I know that's your thought; I'm just checking its everyone else's as
    > well. We go to great lengths elsewhere in the patch to avoid queries
    > returning incorrect results and there is a loss of capability as a
    > result. I don't want Hash index users to view this as a feature. I don't
    > feel too strongly, but it can be argued both ways, at least.
    
    This goes back to your pluggable rmgr point. Someone could add a new
    index method and get bogus results on their standby. And unlike hash
    indexes where there's some hope of addressing the problem there's
    nothing they can do to fix this.
    
    It does seem like having a flag in the catalog to mark nonrecoverable
    indexes and make them unavailable to query plans on the standby would
    be worth its weight in code.
    
    -- 
    greg
    
    

35. Re: Hot Standby, release candidate?

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-12-14T18:13:25Z

    Greg Smith wrote:
    > Heikki Linnakangas wrote:
    >> I note that if it was easy to run pgindent yourself on a patch before
    >> committing/submitting, we wouldn't need to have this discussion. I don't
    >> know hard it is to get it working right, but I recall I tried once and
    >> gave up.
    >>   
    > What sort of problems did you run into?
    
    I don't remember, unfortunately.
    
    -- 
      Heikki Linnakangas
      EnterpriseDB   http://www.enterprisedb.com
    
    

36. Re: Hot Standby, release candidate?

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-12-14T18:32:17Z

    Simon Riggs wrote:
    > On Mon, 2009-12-14 at 11:54 +0200, Heikki Linnakangas wrote:
    >> Simon Riggs wrote:
    >> * Are you planning to remove the recovery_connections setting before
    >> release? The documentation makes it sound like it's a temporary hack
    >> that we're not really sure is needed at all. That's not very comforting.
    > 
    > It has been requested and I agree, so its there. Saying it might be
    > removed in future is no more than we do elsewhere and AFAIK we all hope
    > it will be. Not sure why that is or isn't comforting.
    
    Now that recovery_connections has a double-role, and does in the master
    what the wal_standby_info used to do, the documentation probably should
    be clarified that the whole parameter is not going to go away, just the
    role in the master.
    
    >> * You removed this comment from KnownAssignedXidsInit:
    >>
    >> -   /*
    >> -    * XXX: We should check that we don't exceed maxKnownAssignedXids.
    >> -    * Even though the hash table might hold a few more entries than that,
    >> -    * we use fixed-size arrays of that size elsewhere and expected all
    >> -    * entries in the hash table to fit.
    >> -    */
    >>
    >> but AFAICS you didn't address the issue. It's referring to the 'xids'
    >> array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
    >> in without checking that it fits.
    > 
    > I have ensured that they are always the same size, by definition, so no
    > need to check.
    
    How did you ensure that? The hash table has no hard size limit.
    
    -- 
      Heikki Linnakangas
      EnterpriseDB   http://www.enterprisedb.com
    
    

37. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T18:34:34Z

    On Mon, 2009-12-14 at 18:02 +0000, Greg Stark wrote:
    > >> It doesn't seem any more wrong than using hash indexes right after
    > >> recovery, crash recovery or otherwise. It's certainly broken, but I
    > >> don't see much value in a partial fix. The bottom line is that hash
    > >> indexes should be WAL-logged.
    > >
    > > I know that's your thought; I'm just checking its everyone else's as
    > > well. We go to great lengths elsewhere in the patch to avoid queries
    > > returning incorrect results and there is a loss of capability as a
    > > result. I don't want Hash index users to view this as a feature. I
    > don't
    > > feel too strongly, but it can be argued both ways, at least.
    > 
    > This goes back to your pluggable rmgr point. Someone could add a new
    > index method and get bogus results on their standby. And unlike hash
    > indexes where there's some hope of addressing the problem there's
    > nothing they can do to fix this.
    > 
    > It does seem like having a flag in the catalog to mark nonrecoverable
    > indexes and make them unavailable to query plans on the standby would
    > be worth its weight in code.
    
    pg_am.amalmostworks or perhaps pg_am.amhalffinished... :-)
    
    I wouldn't wish to literally persist that situation, especially since if
    we had it we couldn't update it during recovery. We need to allow
    pluggable indexes in full, not just partially. I think we should extend
    pg_am so that rmgr routines can be defined for them also, with dynamic
    assignment of rmgrids, recorded in file so we can use them during
    recovery.
    
    What we also need is a mechanism to identify and mark indexes as corrupt
    while they are being rebuilt, so recovery can complete without them and
    then rebuild automatically when recovery finishes. And so they can be
    skipped during hot standby.
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

38. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T18:40:34Z

    On Sun, 2009-12-13 at 22:25 +0000, Simon Riggs wrote:
    
    > * Which exact tables are we talking about: just pg_class and the shared
    > catalogs? Everything else is in pg_class, so if we can find it we're OK?
    > formrdesc() tells me the list of nailed relations is: pg_database,
    > pg_class, pg_attribute, pg_proc, and pg_type. Are the nailed relations
    > the ones we care about, or are they just a subset? 
    
    Comments in cluster.c's check_index_is_clusterable() suggest that the
    list of tables to which this applies is nailed relations *and* shared
    relations, plus their indexes.
    
    /*
     * Disallow clustering system relations.  This will definitely NOT work
     * for shared relations (we have no way to update pg_class rows in other
     * databases), nor for nailed-in-cache relations (the relfilenode values
     * for those are hardwired, see relcache.c).  It might work for other
     * system relations, but I ain't gonna risk it.
     */
    
    So that means we need to handle 3 cases: nailed-local, nailed-shared and
    non-nailed-shared.
    
    I would presume we would not want to relax the restriction on CLUSTER
    working on these tables, even if new VACUUM FULL does.
    
    Anyway, not going to be done for Alpha3, but seems fairly doable now.
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

39. Re: Hot Standby, release candidate?

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-14T18:48:53Z

    Simon Riggs <simon@2ndQuadrant.com> writes:
    >  * Disallow clustering system relations.  This will definitely NOT work
    >  * for shared relations (we have no way to update pg_class rows in other
    >  * databases), nor for nailed-in-cache relations (the relfilenode values
    >  * for those are hardwired, see relcache.c).  It might work for other
    >  * system relations, but I ain't gonna risk it.
    
    > I would presume we would not want to relax the restriction on CLUSTER
    > working on these tables, even if new VACUUM FULL does.
    
    Why not?  If we solve the problem of allowing these relations to change
    relfilenodes, then CLUSTER would work just fine on them.  Whether it's
    particularly useful is not ours to decide I think.
    
    			regards, tom lane
    
    

40. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T18:58:35Z

    On Mon, 2009-12-14 at 20:32 +0200, Heikki Linnakangas wrote:
    
    > >> * You removed this comment from KnownAssignedXidsInit:
    > >>
    > >> -   /*
    > >> -    * XXX: We should check that we don't exceed maxKnownAssignedXids.
    > >> -    * Even though the hash table might hold a few more entries than that,
    > >> -    * we use fixed-size arrays of that size elsewhere and expected all
    > >> -    * entries in the hash table to fit.
    > >> -    */
    > >>
    > >> but AFAICS you didn't address the issue. It's referring to the 'xids'
    > >> array in TransactionIdIsInProgress(), which KnownAssignedXidsGet() fills
    > >> in without checking that it fits.
    > > 
    > > I have ensured that they are always the same size, by definition, so no
    > > need to check.
    > 
    > How did you ensure that? The hash table has no hard size limit.
    
    The hash table is in shared memory and the entry size is fixed. My
    understanding was that this meant the hash table was fixed in size and
    could not grow beyond the allocation. If that assumption was wrong, then
    yes we could get an error. Is it? Do you know from experience, or from
    code comments?
    
    Incidentally just picked up two much easier issues in that stretch of
    code. Thanks for making me look again!
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

41. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T19:07:18Z

    On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote:
    > Simon Riggs <simon@2ndQuadrant.com> writes:
    > >  * Disallow clustering system relations.  This will definitely NOT work
    > >  * for shared relations (we have no way to update pg_class rows in other
    > >  * databases), nor for nailed-in-cache relations (the relfilenode values
    > >  * for those are hardwired, see relcache.c).  It might work for other
    > >  * system relations, but I ain't gonna risk it.
    > 
    > > I would presume we would not want to relax the restriction on CLUSTER
    > > working on these tables, even if new VACUUM FULL does.
    > 
    > Why not?  If we solve the problem of allowing these relations to change
    > relfilenodes, then CLUSTER would work just fine on them.  Whether it's
    > particularly useful is not ours to decide I think.
    
    I think you are probably right, but my wish to prove Schrodinger's Bug
    does not exist is not high enough for me personally to open that box
    this side of 8.6, especially when the previous code author saw it as a
    risk worth documenting. 
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

42. Re: Hot Standby, release candidate?

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-14T19:14:06Z

    Simon Riggs <simon@2ndQuadrant.com> writes:
    > On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote:
    >> Simon Riggs <simon@2ndQuadrant.com> writes:
    >>> * Disallow clustering system relations.  This will definitely NOT work
    >>> * for shared relations (we have no way to update pg_class rows in other
    >>> * databases), nor for nailed-in-cache relations (the relfilenode values
    >>> * for those are hardwired, see relcache.c).  It might work for other
    >>> * system relations, but I ain't gonna risk it.
    >> 
    >>> I would presume we would not want to relax the restriction on CLUSTER
    >>> working on these tables, even if new VACUUM FULL does.
    >> 
    >> Why not?  If we solve the problem of allowing these relations to change
    >> relfilenodes, then CLUSTER would work just fine on them.  Whether it's
    >> particularly useful is not ours to decide I think.
    
    > I think you are probably right, but my wish to prove Schrodinger's Bug
    > does not exist is not high enough for me personally to open that box
    > this side of 8.6, especially when the previous code author saw it as a
    > risk worth documenting. 
    
    You're talking to the "previous code author" ... or at least I'm pretty
    sure that comment is mine.
    
    			regards, tom lane
    
    

43. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T19:21:49Z

    On Mon, 2009-12-14 at 14:14 -0500, Tom Lane wrote:
    > Simon Riggs <simon@2ndQuadrant.com> writes:
    > > On Mon, 2009-12-14 at 13:48 -0500, Tom Lane wrote:
    > >> Simon Riggs <simon@2ndQuadrant.com> writes:
    > >>> * Disallow clustering system relations.  This will definitely NOT work
    > >>> * for shared relations (we have no way to update pg_class rows in other
    > >>> * databases), nor for nailed-in-cache relations (the relfilenode values
    > >>> * for those are hardwired, see relcache.c).  It might work for other
    > >>> * system relations, but I ain't gonna risk it.
    > >> 
    > >>> I would presume we would not want to relax the restriction on CLUSTER
    > >>> working on these tables, even if new VACUUM FULL does.
    > >> 
    > >> Why not?  If we solve the problem of allowing these relations to change
    > >> relfilenodes, then CLUSTER would work just fine on them.  Whether it's
    > >> particularly useful is not ours to decide I think.
    > 
    > > I think you are probably right, but my wish to prove Schrodinger's Bug
    > > does not exist is not high enough for me personally to open that box
    > > this side of 8.6, especially when the previous code author saw it as a
    > > risk worth documenting. 
    > 
    > You're talking to the "previous code author" ... or at least I'm pretty
    > sure that comment is mine.
    
    Yeh, I figured, but I'm just as scared now as you were back then. 
    
    This might allow CLUSTER to work, but it is definitely not something
    that I will enabling, testing and committing to fix *when* it breaks
    because my time is already allocated on other stuff.
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

44. Re: Hot Standby, release candidate?

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-14T20:24:21Z

    Simon Riggs <simon@2ndQuadrant.com> writes:
    > On Mon, 2009-12-14 at 20:32 +0200, Heikki Linnakangas wrote:
    >>> I have ensured that they are always the same size, by definition, so no
    >>> need to check.
    >> 
    >> How did you ensure that? The hash table has no hard size limit.
    
    > The hash table is in shared memory and the entry size is fixed. My
    > understanding was that this meant the hash table was fixed in size and
    > could not grow beyond the allocation. If that assumption was wrong, then
    > yes we could get an error. Is it?
    
    Entirely.  The only thing the hash table size enters into is the sizing
    of overall shared memory --- different hash tables then consume space
    from the common pool, which includes not only the computed space
    requirements but a pretty hefty slop overhead.  You can go beyond the
    original requested space if there is any slop left.
    
    For a number of shared hashtables that actually have a fixed set of
    entries, we avoid the risk of unexpected out-of-memory by forcing all
    the entries to come into existence during startup.  If your table
    doesn't work that way then you cannot be sure of the exact point where
    it will get an out-of-memory failure.
    
    			regards, tom lane
    
    

45. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T21:20:51Z

    On Mon, 2009-12-14 at 15:24 -0500, Tom Lane wrote:
    > Simon Riggs <simon@2ndQuadrant.com> writes:
    > > On Mon, 2009-12-14 at 20:32 +0200, Heikki Linnakangas wrote:
    > >>> I have ensured that they are always the same size, by definition, so no
    > >>> need to check.
    > >> 
    > >> How did you ensure that? The hash table has no hard size limit.
    > 
    > > The hash table is in shared memory and the entry size is fixed. My
    > > understanding was that this meant the hash table was fixed in size and
    > > could not grow beyond the allocation. If that assumption was wrong, then
    > > yes we could get an error. Is it?
    > 
    > Entirely.  The only thing the hash table size enters into is the sizing
    > of overall shared memory --- different hash tables then consume space
    > from the common pool, which includes not only the computed space
    > requirements but a pretty hefty slop overhead.  You can go beyond the
    > original requested space if there is any slop left.
    
    OK, thanks.
    
    > For a number of shared hashtables that actually have a fixed set of
    > entries, we avoid the risk of unexpected out-of-memory by forcing all
    > the entries to come into existence during startup.  If your table
    > doesn't work that way then you cannot be sure of the exact point where
    > it will get an out-of-memory failure.
    
    The data structure was originally a list of fixed size, though is now a
    shared hash table.
    
    What is the best way of restricting the hash table to a maximum size? 
    
    Your last para makes me think there is a way, but I can't see it
    directly. If there isn't a facility to do this and I need to add code,
    should I add optional code into the dynahash.c to track size, or should
    I add that in the data structure code that uses the hash functions (so,
    internally or externally).
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

46. Re: Hot Standby, release candidate?

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-14T21:39:51Z

    Simon Riggs <simon@2ndQuadrant.com> writes:
    > What is the best way of restricting the hash table to a maximum size? 
    
    There is nothing in dynahash that will enforce a maximum size against
    calling code that's not cooperating; and I'll resist any attempt to
    add such a thing, because it would create a serialization point across
    the whole hashtable.
    
    If you know that you need at most N entries in the hash table, you can
    preallocate that many at startup (note the second arg to ShmemInitHash)
    and be safe.  If your calling code might go past that, you'll need to
    fix the calling code.
    
    			regards, tom lane
    
    

47. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-14T23:23:36Z

    On Mon, 2009-12-14 at 16:39 -0500, Tom Lane wrote:
    > Simon Riggs <simon@2ndQuadrant.com> writes:
    > > What is the best way of restricting the hash table to a maximum size? 
    > 
    > There is nothing in dynahash that will enforce a maximum size against
    > calling code that's not cooperating; and I'll resist any attempt to
    > add such a thing, because it would create a serialization point across
    > the whole hashtable.
    
    No problem, just checking with you where you'd like stuff put.
    
    > If you know that you need at most N entries in the hash table, you can
    > preallocate that many at startup (note the second arg to ShmemInitHash)
    > and be safe.  If your calling code might go past that, you'll need to
    > fix the calling code.
    
    It's easy enough to count em on the way in and count em on the way out.
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

48. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-15T00:03:30Z

    On Mon, 2009-12-14 at 11:44 +0200, Peter Eisentraut wrote:
    > On mån, 2009-12-14 at 08:54 +0000, Simon Riggs wrote:
    > > Wednesday because that seemed a good delay to allow review. Josh and I
    > > had discussed the value of getting patch into Alpha3, so that was my
    > > wish and aim.
    > > 
    > > I'm not aware of any particular date for end of commitfest, though
    > > possibly you are about to update me on that?
    > 
    > Commit fests for 8.5 have usually run from the 15th to the 15th of next
    > month, but the CF manager may choose to vary that.
    > 
    > FWIW, the alpha release manager may also vary the release timeline of
    > alpha3. ;-)
    
    I'm hoping that the alpha release manager can wait until Wednesday,
    please. 
    
    -- 
     Simon Riggs           www.2ndQuadrant.com
    
    
    

49. Re: Hot Standby, release candidate?

    Greg Smith <greg@2ndquadrant.com> — 2009-12-15T06:27:38Z

    Simon Riggs wrote:
    > On Mon, 2009-12-14 at 11:44 +0200, Peter Eisentraut wrote:
    >   
    >> On mån, 2009-12-14 at 08:54 +0000, Simon Riggs wrote:
    >>     
    >>> Wednesday because that seemed a good delay to allow review. Josh and I
    >>> had discussed the value of getting patch into Alpha3, so that was my
    >>> wish and aim.
    >>>
    >>> I'm not aware of any particular date for end of commitfest, though
    >>> possibly you are about to update me on that?
    >>>       
    >> Commit fests for 8.5 have usually run from the 15th to the 15th of next
    >> month, but the CF manager may choose to vary that.
    >>
    >> FWIW, the alpha release manager may also vary the release timeline of
    >> alpha3. ;-)
    >>     
    >
    > I'm hoping that the alpha release manager can wait until Wednesday,
    > please. 
    >   
    At this point we've got 5 small to medium sized patches in the "Ready 
    for Committer" queue.  Maybe that gets all done on Tuesday, maybe it 
    slips to Wednesday or later.  It's not like a bell goes off tomorrow and 
    we're done; there's probably going to be just a little slip here.
    
    In any case, it's certainly not the case that this is all done right now 
    such that the alpha gets packed on Tuesday just because it's the 15th.  
    It sounds like the worst case is that alpha would have to wait a day for 
    Hot Standby to be finally committed, which seems well worth doing if it 
    means HS gets that much more testing on it.  It would be a help to 
    eliminate the merge conflict issues for the Streaming Replication team 
    by giving them only one code base to worry about merges against.  And on 
    the PR side, announcing HS as hitting core and available in the alpha is 
    huge.
    
    -- 
    Greg Smith    2ndQuadrant   Baltimore, MD
    PostgreSQL Training, Services and Support
    greg@2ndQuadrant.com  www.2ndQuadrant.com
    
    

50. Re: Hot Standby, release candidate?

    Peter Eisentraut <peter_e@gmx.net> — 2009-12-17T10:01:17Z

    On sön, 2009-12-13 at 19:20 +0000, Simon Riggs wrote:
    > Barring resolving a few points and subject to even more testing, this
    > is the version I expect to commit to CVS on Wednesday.
    
    So it's Thursday now.  Please keep us updated on the schedule, as we
    need to decide when to wrap alpha3 and whether to reopen the floodgates
    for post-CF commits.
    
    
    

51. Re: Hot Standby, release candidate?

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-17T10:08:36Z

    On Thu, 2009-12-17 at 12:01 +0200, Peter Eisentraut wrote:
    > On sön, 2009-12-13 at 19:20 +0000, Simon Riggs wrote:
    > > Barring resolving a few points and subject to even more testing, this
    > > is the version I expect to commit to CVS on Wednesday.
    > 
    > So it's Thursday now.  Please keep us updated on the schedule, as we
    > need to decide when to wrap alpha3 and whether to reopen the floodgates
    > for post-CF commits.
    
    I've been looking at a semaphore deadlock problem reported by Hiroyuki
    Yamada. It's a serious issue, though luckily somewhat restricted in
    scope.
    
    I don't think its wise to rush in with a solution, since that involves
    some work with semaphores and I could easily make that area less stable
    by acting too quickly.
    
    What I will do now is put in a restriction on lock waits in Hot Standby,
    which will only happen for Alpha3. That avoids the deadlock issue in an
    easy and safe way, though it is heavy handed and I aim to replace it
    fairly soon, following discussion and testing.
    
    -- 
     Simon Riggs           www.2ndQuadrant.com