Thread

1. PATCH to allow concurrent VACUUMs to not lock each other out from cleaning old tuples

   Hannu Krosing <hannu@tm.ee> — 2005-05-18T08:54:05Z

   The attached patch allows VACUUMS's on small relations to clean up dead
   tuples while VACUUM or ANALYSE is running for a long time on some big
   table.
   
   This is done by adding a "bool inVacuum" to PGPROC and then making use
   of it in GetOldestXmin.
   
   This patch is against current CVS head, but should also apply to 8.0.2
   with minorpach  warnings.
   
   -- 
   Hannu Krosing <hannu@tm.ee>
   

2. Re: PATCH to allow concurrent VACUUMs to not lock each

   Hannu Krosing <hannu@skype.net> — 2005-05-23T09:49:42Z

   On K, 2005-05-18 at 11:54 +0300, Hannu Krosing wrote:
   > The attached patch allows VACUUMS's on small relations to clean up dead
   > tuples while VACUUM or ANALYSE is running for a long time on some big
   > table.
   > 
   > This is done by adding a "bool inVacuum" to PGPROC and then making use
   > of it in GetOldestXmin.
   > 
   > This patch is against current CVS head, but should also apply to 8.0.2
   > with minorpach  warnings.
   
   Could this patch be applied (or rejected if something is badly wrong
   with it) ?
   
   Or should I move the discussion back to pgsql-hackers ad try to make it
   a TODO first ?
   
   This patch implements what I described in
   http://archives.postgresql.org/pgsql-hackers/2005-05/msg00704.php
   plus a small change to make it work for simple ANALYSE too.
   
   -- 
   Hannu Krosing <hannu@skype.net>
   
   
   

3. Re: PATCH to allow concurrent VACUUMs to not lock each other

   Bruce Momjian <pgman@candle.pha.pa.us> — 2005-05-23T13:13:31Z

   Hannu Krosing wrote:
   > On K, 2005-05-18 at 11:54 +0300, Hannu Krosing wrote:
   > > The attached patch allows VACUUMS's on small relations to clean up dead
   > > tuples while VACUUM or ANALYSE is running for a long time on some big
   > > table.
   > > 
   > > This is done by adding a "bool inVacuum" to PGPROC and then making use
   > > of it in GetOldestXmin.
   > > 
   > > This patch is against current CVS head, but should also apply to 8.0.2
   > > with minorpach  warnings.
   > 
   > Could this patch be applied (or rejected if something is badly wrong
   > with it) ?
   > 
   > Or should I move the discussion back to pgsql-hackers ad try to make it
   > a TODO first ?
   > 
   > This patch implements what I described in
   > http://archives.postgresql.org/pgsql-hackers/2005-05/msg00704.php
   > plus a small change to make it work for simple ANALYSE too.
   
   I have it in my mailbox and will get to it.  Thanks.
   
   -- 
     Bruce Momjian                        |  http://candle.pha.pa.us
     pgman@candle.pha.pa.us               |  (610) 359-1001
     +  If your life is a hard drive,     |  13 Roberts Road
     +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
   
   

4. Re: PATCH to allow concurrent VACUUMs to not lock each

   Neil Conway <neilc@samurai.com> — 2005-05-23T13:58:58Z

   Hannu Krosing wrote:
   > *** src/backend/access/transam/xact.c	28 Apr 2005 21:47:10 -0000	1.200
   > --- src/backend/access/transam/xact.c	17 May 2005 22:06:34 -0000
   > ***************
   > *** 1411,1416 ****
   > --- 1411,1424 ----
   >   	AfterTriggerBeginXact();
   >   
   >   	/*
   > + 	 * mark the transaction as not VACUUM  (vacuum_rel will set isVacuum to true 
   > + 	 * directly after calling BeginTransactionCommand() )
   > + 	 */
   > + 	if (MyProc != NULL)  
   > + 	{
   > + 		MyProc->inVacuum = false;
   > + 	}
   
   I'm a little worried about having this set to "true" after a VACUUM is 
   executed, and only reset to false when the next transaction is begun: it 
   shouldn't affect correctness right now, but it seems like asking for 
   trouble. Resetting the flag to "false" after processing a transaction 
   would probably be worth doing.
   
   > *** src/backend/commands/vacuum.c	6 May 2005 17:24:53 -0000	1.308
   > --- src/backend/commands/vacuum.c	17 May 2005 22:06:35 -0000
   > ***************
   > *** 420,425 ****
   > --- 418,428 ----
   >   				if (use_own_xacts)
   >   				{
   >   					StartTransactionCommand();
   > + 					if (MyProc != NULL)  /* is this needed here ? */
   > + 					{
   > + 						/* so other vacuums don't look at our xid/xmin in GetOldestXmin() */
   > + 						MyProc->inVacuum = true;
   > + 					}
   >   					/* functions in indexes may want a snapshot set */
   >   					ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
   >   				}
   
   Is it valid to apply this optimization to ANALYZE? Since ANALYZE updates 
   pg_statistic, ISTM it can affect tuple visibility.
   
   > *** src/backend/storage/ipc/sinval.c	31 Dec 2004 22:00:56 -0000	1.75
   > --- src/backend/storage/ipc/sinval.c	17 May 2005 22:06:36 -0000
   > ***************
   > *** 845,854 ****
   >   			 * them as running anyway.	We also assume that such xacts
   >   			 * can't compute an xmin older than ours, so they needn't be
   >   			 * considered in computing globalxmin.
   >   			 */
   >   			if (proc == MyProc ||
   >   				!TransactionIdIsNormal(xid) ||
   > ! 				TransactionIdFollowsOrEquals(xid, xmax))
   >   				continue;
   >   
   >   			if (TransactionIdPrecedes(xid, xmin))
   > --- 845,858 ----
   >   			 * them as running anyway.	We also assume that such xacts
   >   			 * can't compute an xmin older than ours, so they needn't be
   >   			 * considered in computing globalxmin.
   > + 			 *
   > + 			 * there is also no need to consider transaxtions runnibg the 
   > + 			 * vacuum command as it will not affect tuple visibility
   >   			 */
   
   Typos.
   
   -Neil
   
   

5. Re: PATCH to allow concurrent VACUUMs to not lock each

   Tom Lane <tgl@sss.pgh.pa.us> — 2005-05-23T14:16:06Z

   Neil Conway <neilc@samurai.com> writes:
   > I'm a little worried about having this set to "true" after a VACUUM is 
   > executed, and only reset to false when the next transaction is begun: it 
   > shouldn't affect correctness right now, but it seems like asking for 
   > trouble. Resetting the flag to "false" after processing a transaction 
   > would probably be worth doing.
   
   These days I'd be inclined to use a PG_TRY construct to guarantee the
   flag is cleared, rather than loading another cleanup operation onto
   unrelated code.
   
   The MyProc != NULL tests are a waste of code space.  You can't even
   acquire an LWLock without MyProc being set, let alone access tables.
   
   The real issue here though is whether anyone can blow a hole in the
   xmin assumptions: is there any situation where ignoring a vacuum
   transaction breaks things?  I haven't had time to think about it
   in any detail, but it definitely needs to be thought about.
   
   			regards, tom lane
   
   

6. Re: PATCH to allow concurrent VACUUMs to not lock each

   Hannu Krosing <hannu@skype.net> — 2005-05-23T15:13:47Z

   On E, 2005-05-23 at 10:16 -0400, Tom Lane wrote:
   > Neil Conway <neilc@samurai.com> writes:
   > > I'm a little worried about having this set to "true" after a VACUUM is 
   > > executed, and only reset to false when the next transaction is begun: it 
   > > shouldn't affect correctness right now, but it seems like asking for 
   > > trouble. Resetting the flag to "false" after processing a transaction 
   > > would probably be worth doing.
   > 
   > These days I'd be inclined to use a PG_TRY construct to guarantee the
   > flag is cleared, rather than loading another cleanup operation onto
   > unrelated code.
   
   Ok, will check out PG_TRY. I hoped that there is some way not to set
   inVacuum to false at each transaction start and still be sure that it
   will be reverted after vacuum_rel.
   
   So I'll set it once at the start of connection and then maintain it in
   vacuum_rel() using PG_TRY.
   
   > The MyProc != NULL tests are a waste of code space.  You can't even
   > acquire an LWLock without MyProc being set, let alone access tables.
   
   Thanks, I'll get rid of them.
   
   > The real issue here though is whether anyone can blow a hole in the
   > xmin assumptions: is there any situation where ignoring a vacuum
   > transaction breaks things?  I haven't had time to think about it
   > in any detail, but it definitely needs to be thought about.
   
   There may be need to exclude vacuum/analyse on system relations from
   being ignored by vacuum_rel() as I suspect that the info they both write
   to pg_class, pg_attribute, and possibly other tables may be vulnerable
   to crashes at right moment. 
   
   Also it may be prudent to not exclude other vacuums, when the vacuum_rel
   () itself is run on a system relation.
   
   I'm not sure which way it is, as my head gets all thick each time I try
   to figure it out ;p.
   
   I can't think of any other cases where it could matter, as at least the
   work done inside vacuum_rel() itself seema non-rollbackable.
   
   -- 
   Hannu Krosing <hannu@skype.net>
   
   
   

7. Re: PATCH to allow concurrent VACUUMs to not lock each

   Tom Lane <tgl@sss.pgh.pa.us> — 2005-05-23T15:42:31Z

   Hannu Krosing <hannu@skype.net> writes:
   > I can't think of any other cases where it could matter, as at least the
   > work done inside vacuum_rel() itself seema non-rollbackable.
   
   VACUUM FULL's tuple-moving is definitely roll-back-able, so it might be
   prudent to only do this for lazy VACUUM.  But on the other hand, VACUUM
   FULL holds an exclusive lock on the table so no one else is going to see
   its effects concurrently anyway.
   
   As I said, it needs more thought than I've been able to spare for it yet
   ...
   
   			regards, tom lane
   
   

8. Re: PATCH to allow concurrent VACUUMs to not lock each

   Hannu Krosing <hannu@skype.net> — 2005-05-23T16:20:16Z

   On E, 2005-05-23 at 11:42 -0400, Tom Lane wrote:
   > Hannu Krosing <hannu@skype.net> writes:
   > > I can't think of any other cases where it could matter, as at least the
   > > work done inside vacuum_rel() itself seema non-rollbackable.
   > 
   > VACUUM FULL's tuple-moving is definitely roll-back-able, so it might be
   > prudent to only do this for lazy VACUUM.  But on the other hand, VACUUM
   > FULL holds an exclusive lock on the table so no one else is going to see
   > its effects concurrently anyway.
   
   I'm not interested in VACUUM FULL at all. This is improvement mainly for
   heavy update OLAP databases, where I would not even think of running
   VACUUM FULL.
   
   I'll cheks if there's an easy way to exclude VACUUM FULL.
   
   > As I said, it needs more thought than I've been able to spare for it yet
   > ...
   
   Ok, thanks for comments this far .
   
   -- 
   Hannu Krosing <hannu@skype.net>
   
   
   

9. Re: PATCH to allow concurrent VACUUMs to not lock each

   Hannu Krosing <hannu@skype.net> — 2005-07-03T16:14:38Z

   On E, 2005-05-23 at 11:42 -0400, Tom Lane wrote:
   > Hannu Krosing <hannu@skype.net> writes:
   > > I can't think of any other cases where it could matter, as at least the
   > > work done inside vacuum_rel() itself seema non-rollbackable.
   > 
   > VACUUM FULL's tuple-moving is definitely roll-back-able, so it might be
   > prudent to only do this for lazy VACUUM.  But on the other hand, VACUUM
   > FULL holds an exclusive lock on the table so no one else is going to see
   > its effects concurrently anyway.
   
   Ok, this is a new version of the vacuum patch with the following changes
   following some suggestions in this thread.
   
   * changed the patch to affect only lazy vacuum 
   * moved inVacuum handling to use PG_TRY
   * moved vac_update_relstats() out of lazy_vacuum_rel into a separate
     transaction. The code to do this may not be the prettiest, maybe it
     should use a separate struct.
   
   -- 
   Hannu Krosing <hannu@skype.net>
   

10. Re: PATCH to allow concurrent VACUUMs to not lock each

    Tom Lane <tgl@sss.pgh.pa.us> — 2005-07-03T16:19:49Z

    Hannu Krosing <hannu@skype.net> writes:
    > Ok, this is a new version of the vacuum patch with the following changes
    > following some suggestions in this thread.
    
    The more I look at this, the uglier it looks ... and I still haven't
    seen any convincing demonstration that it *works*, ie doesn't have
    bad side-effects on the transaction-is-in-progress logic.  I'm
    particularly concerned about what happens to the RecentXmin horizon
    for pg_subtrans and pg_multixact operations.
    
    			regards, tom lane
    
    

11. Re: PATCH to allow concurrent VACUUMs to not lock each

    Hannu Krosing <hannu@skype.net> — 2005-07-04T07:24:59Z

    On P, 2005-07-03 at 12:19 -0400, Tom Lane wrote:
    > Hannu Krosing <hannu@skype.net> writes:
    > > Ok, this is a new version of the vacuum patch with the following changes
    > > following some suggestions in this thread.
    > 
    > The more I look at this, the uglier it looks ... and I still haven't
    > seen any convincing demonstration that it *works*, ie doesn't have
    > bad side-effects on the transaction-is-in-progress logic.
    
    The function GetOldestXmin is used *only* when determining oldest xmin
    for transactions.
    
    > I'm particularly concerned about what happens to the RecentXmin horizon
    > for pg_subtrans and pg_multixact operations.
    
    How are they affected by this change ? They should still see the vacuum
    as oldest transaction, unless they 
    
    
    Oh, now I see. I'm pretty sure that at the time of original patch, the
    *only* uses of GetOldestXmin was from VACUUM and catalog/index.c and
    both for the same purpose, but now I see also a call from
    access/transam/xlog.c.
    
    Perhaps I should separate the function used by vacuum into another
    function, say GetOldestDataChangingXmin(),  to keep the possible impact
    as localised as possible.
    
    Do you have any specific concerns related to this patch after that ?
    
    Or should I just back off for now and maybe start a separate project for
    ironing out patches related to running postgresql in real-world 24/7
    OLTP environment (similar to what Bizgres does for OLAP ) ?
    
    -- 
    Hannu Krosing <hannu@skype.net>
    
    
    
    

12. Re: PATCH to allow concurrent VACUUMs to not lock each

    Hannu Krosing <hannu@tm.ee> — 2005-07-04T11:26:54Z

    On E, 2005-07-04 at 10:24 +0300, Hannu Krosing wrote:
    > On P, 2005-07-03 at 12:19 -0400, Tom Lane wrote:
    > > Hannu Krosing <hannu@skype.net> writes:
    > > > Ok, this is a new version of the vacuum patch with the following changes
    > > > following some suggestions in this thread.
    > > 
    > > The more I look at this, the uglier it looks ... and I still haven't
    > > seen any convincing demonstration that it *works*, ie doesn't have
    > > bad side-effects on the transaction-is-in-progress logic.
    
    Ok, I changed GetOldestXmin() to use proc->inVacuum only when
    determining the oldest visible xid for vacuum and index (i.e. which
    tuples are safe to delete and which tuples there is no need to index).
    
    The third use on GetOldestXmin() in xlog.c is changed to use old
    GetOldestXmin() logic.
    
    
    My reasoning for why the patch should work is as follows:
    
    1) the only transaction during which inVacuum is set is the 2nd
    transaction (of originally 3, now 4) of lazy VACUUM, which does simple
    heap scanning and old tuple removal (lazy_vacuum_rel()), and does no
    externally visible changes to the data. It only removes tuples which are
    already invisible to all running transactions. 
    
    2) That transaction never deletes, updates or inserts any tuples on it
    own.
    
    3) As it can't add any tuples or change any existing tuples to have its
    xid as either xmin or xmax, it already does run logically "outside of
    transactions".
    
    4) The only use made of of proc->inVacuum is when determining which
    tuples are safe to remove (in vacuum.c) or not worth indexing (in
    index.c) and thus can't affect anything else.
    
    
    
    I can easily demonstrate that it "works" in the sense that it allows
    several concurrent vacuums to clean out old tuples, and I have thus far
    been unable to construct the counterexample where it does anything bad.
    
    Could you tell me which part of my reasoning is wrong or what else do I
    overlook.
    
    -- 
    Hannu Krosing <hannu@tm.ee>
    

13. Re: PATCH to allow concurrent VACUUMs to not lock each

    Bruce Momjian <pgman@candle.pha.pa.us> — 2005-08-12T19:47:13Z

    This has been saved for the 8.2 release:
    
    	http://momjian.postgresql.org/cgi-bin/pgpatches_hold
    
    ---------------------------------------------------------------------------
    
    Hannu Krosing wrote:
    > On E, 2005-07-04 at 10:24 +0300, Hannu Krosing wrote:
    > > On P, 2005-07-03 at 12:19 -0400, Tom Lane wrote:
    > > > Hannu Krosing <hannu@skype.net> writes:
    > > > > Ok, this is a new version of the vacuum patch with the following changes
    > > > > following some suggestions in this thread.
    > > > 
    > > > The more I look at this, the uglier it looks ... and I still haven't
    > > > seen any convincing demonstration that it *works*, ie doesn't have
    > > > bad side-effects on the transaction-is-in-progress logic.
    > 
    > Ok, I changed GetOldestXmin() to use proc->inVacuum only when
    > determining the oldest visible xid for vacuum and index (i.e. which
    > tuples are safe to delete and which tuples there is no need to index).
    > 
    > The third use on GetOldestXmin() in xlog.c is changed to use old
    > GetOldestXmin() logic.
    > 
    > 
    > My reasoning for why the patch should work is as follows:
    > 
    > 1) the only transaction during which inVacuum is set is the 2nd
    > transaction (of originally 3, now 4) of lazy VACUUM, which does simple
    > heap scanning and old tuple removal (lazy_vacuum_rel()), and does no
    > externally visible changes to the data. It only removes tuples which are
    > already invisible to all running transactions. 
    > 
    > 2) That transaction never deletes, updates or inserts any tuples on it
    > own.
    > 
    > 3) As it can't add any tuples or change any existing tuples to have its
    > xid as either xmin or xmax, it already does run logically "outside of
    > transactions".
    > 
    > 4) The only use made of of proc->inVacuum is when determining which
    > tuples are safe to remove (in vacuum.c) or not worth indexing (in
    > index.c) and thus can't affect anything else.
    > 
    > 
    > 
    > I can easily demonstrate that it "works" in the sense that it allows
    > several concurrent vacuums to clean out old tuples, and I have thus far
    > been unable to construct the counterexample where it does anything bad.
    > 
    > Could you tell me which part of my reasoning is wrong or what else do I
    > overlook.
    > 
    > -- 
    > Hannu Krosing <hannu@tm.ee>
    
    [ Attachment, skipping... ]
    
    > 
    > ---------------------------(end of broadcast)---------------------------
    > TIP 6: Have you searched our list archives?
    > 
    >                http://archives.postgresql.org
    
    -- 
      Bruce Momjian                        |  http://candle.pha.pa.us
      pgman@candle.pha.pa.us               |  (610) 359-1001
      +  If your life is a hard drive,     |  13 Roberts Road
      +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
    
    

14. Re: PATCH to allow concurrent VACUUMs to not lock each

    Hannu Krosing <hannu@tm.ee> — 2005-08-12T22:43:59Z

    On R, 2005-08-12 at 15:47 -0400, Bruce Momjian wrote:
    > This has been saved for the 8.2 release:
    > 
    > 	http://momjian.postgresql.org/cgi-bin/pgpatches_hold
    
    Is there any particular reason for not putting it in 8.1 ?
    
    -- 
    Hannu Krosing <hannu@tm.ee>
    
    

15. Re: PATCH to allow concurrent VACUUMs to not lock each

    Bruce Momjian <pgman@candle.pha.pa.us> — 2005-08-14T03:06:38Z

    Hannu Krosing wrote:
    > On R, 2005-08-12 at 15:47 -0400, Bruce Momjian wrote:
    > > This has been saved for the 8.2 release:
    > > 
    > > 	http://momjian.postgresql.org/cgi-bin/pgpatches_hold
    > 
    > Is there any particular reason for not putting it in 8.1 ?
    
    I thought there was still uncertainty about the patch.  Is there?
    
    -- 
      Bruce Momjian                        |  http://candle.pha.pa.us
      pgman@candle.pha.pa.us               |  (610) 359-1001
      +  If your life is a hard drive,     |  13 Roberts Road
      +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
    
    

16. Re: PATCH to allow concurrent VACUUMs to not lock each

    Tom Lane <tgl@sss.pgh.pa.us> — 2005-08-14T16:56:06Z

    Bruce Momjian <pgman@candle.pha.pa.us> writes:
    >> Is there any particular reason for not putting it in 8.1 ?
    
    > I thought there was still uncertainty about the patch.  Is there?
    
    Considerable uncertainty, in my mind.  What we've got here is some
    pretty fundamental hacking on the transaction visibility logic, and
    neither Hannu nor anyone else has produced a convincing argument
    that it's correct.  "It hasn't failed yet in my usage" isn't enough
    to give me a good feeling about it.  Some specific concerns:
    
    * Given that VACUUM ANALYZE does create new output tuples stamped with
    its xid, I'm unclear on what happens in pg_statistic with this code in
    place.  It seems entirely possible that someone might conclude the
    analyze tuples are from a crashed transaction and mark them invalid
    before the analyze can commit (notice TransactionIdIsInProgress does not
    bother looking in PGPROC when the tuple xmin is less than RecentXmin).
    
    * If the vacuum xact is older than what others think is the global xmin,
    it could have problems with other vacuums removing tuples it should
    still be able to see (presumably only in the system catalogs, so maybe
    this isn't an issue, but I'm unsure).  A related scenario that I don't
    think can be dismissed is someone truncating off part of pg_subtrans or
    pg_multixact that the vacuum still needs.
    
    			regards, tom lane
    
    

17. Re: PATCH to allow concurrent VACUUMs to not lock each

    Tom Lane <tgl@sss.pgh.pa.us> — 2005-08-17T19:40:53Z

    Just for the archives, attached is as far as I'd gotten with cleaning up
    Hannu's patch before I realized that it wasn't doing what it needed to
    do.  This fixes an end-of-transaction race condition (can't unset
    inVacuum before xact end, unless you want OldestXmin going backwards
    from the point of view of other people) and improves the documentation
    of what's going on.  But unless someone can convince me that it's safe
    to mess with GetSnapshotData, it's unlikely this'll ever get applied.
    
    			regards, tom lane
    
    

18. Re: PATCH to allow concurrent VACUUMs to not lock each

    Hannu Krosing <hannu@skype.net> — 2005-08-24T12:10:13Z

    On K, 2005-08-17 at 15:40 -0400, Tom Lane wrote:
    >                            Saatja: 
    > Tom Lane <tgl@sss.pgh.pa.us>
    >                           Kellele: 
    > Bruce Momjian
    > <pgman@candle.pha.pa.us>, Hannu
    > Krosing <hannu@tm.ee>, Neil Conway
    > <neilc@samurai.com>, pgsql-
    > patches@postgresql.org
    >                             Teema: 
    > Re: [PATCHES] PATCH to allow
    > concurrent VACUUMs to not lock each
    >                           Kuupäev: 
    > Wed, 17 Aug 2005 15:40:53 -0400
    > (22:40 EEST)
    > 
    > Just for the archives, attached is as far as I'd gotten with cleaning
    > up
    > Hannu's patch before I realized that it wasn't doing what it needed to
    > do.  This fixes an end-of-transaction race condition (can't unset
    > inVacuum before xact end, unless you want OldestXmin going backwards
    > from the point of view of other people) and improves the documentation
    > of what's going on.  But unless someone can convince me that it's safe
    > to mess with GetSnapshotData, it's unlikely this'll ever get applied.
    > 
    > 
    > 
    
    Attached is a patch, based on you last one, which messes with
    GetSnapshotData in what I think is a safe way.
    
    It introduces another attribute to PROC , proc->nonInVacuumXmin and
    computes this in addition to prox->xmin inside GetSnapshotData.
    
    When (and only when) GetOldestXmin is called with ignoreVacuum=true,
    then proc->nonInVacuumXmin is checked instead of prox->xmin.
    
    I believe that this will make this change invisible to all other places
    where GetSnapshotData or GetOldestXmin is used.
    
    -- 
    Hannu Krosing <hannu@skype.net>
    
    
    
    
    
    

19. Re: PATCH to allow concurrent VACUUMs to not lock each

    Bruce Momjian <pgman@candle.pha.pa.us> — 2005-08-24T14:08:41Z

    This has been saved for the 8.2 release:
    
    	http://momjian.postgresql.org/cgi-bin/pgpatches_hold
    
    ---------------------------------------------------------------------------
    
    Hannu Krosing wrote:
    > On K, 2005-08-17 at 15:40 -0400, Tom Lane wrote:
    > >                            Saatja: 
    > > Tom Lane <tgl@sss.pgh.pa.us>
    > >                           Kellele: 
    > > Bruce Momjian
    > > <pgman@candle.pha.pa.us>, Hannu
    > > Krosing <hannu@tm.ee>, Neil Conway
    > > <neilc@samurai.com>, pgsql-
    > > patches@postgresql.org
    > >                             Teema: 
    > > Re: [PATCHES] PATCH to allow
    > > concurrent VACUUMs to not lock each
    > >                           Kuup?ev: 
    > > Wed, 17 Aug 2005 15:40:53 -0400
    > > (22:40 EEST)
    > > 
    > > Just for the archives, attached is as far as I'd gotten with cleaning
    > > up
    > > Hannu's patch before I realized that it wasn't doing what it needed to
    > > do.  This fixes an end-of-transaction race condition (can't unset
    > > inVacuum before xact end, unless you want OldestXmin going backwards
    > > from the point of view of other people) and improves the documentation
    > > of what's going on.  But unless someone can convince me that it's safe
    > > to mess with GetSnapshotData, it's unlikely this'll ever get applied.
    > > 
    > > 
    > > 
    > 
    > Attached is a patch, based on you last one, which messes with
    > GetSnapshotData in what I think is a safe way.
    > 
    > It introduces another attribute to PROC , proc->nonInVacuumXmin and
    > computes this in addition to prox->xmin inside GetSnapshotData.
    > 
    > When (and only when) GetOldestXmin is called with ignoreVacuum=true,
    > then proc->nonInVacuumXmin is checked instead of prox->xmin.
    > 
    > I believe that this will make this change invisible to all other places
    > where GetSnapshotData or GetOldestXmin is used.
    > 
    > -- 
    > Hannu Krosing <hannu@skype.net>
    > 
    > 
    > 
    > 
    > 
    
    [ Attachment, skipping... ]
    
    -- 
      Bruce Momjian                        |  http://candle.pha.pa.us
      pgman@candle.pha.pa.us               |  (610) 359-1001
      +  If your life is a hard drive,     |  13 Roberts Road
      +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
    
    

20. Re: PATCH to allow concurrent VACUUMs to not lock each

    Bruce Momjian <pgman@candle.pha.pa.us> — 2006-03-21T03:34:33Z

    This is going to need a significant safety review.
    
    Your patch has been added to the PostgreSQL unapplied patches list at:
    
    	http://momjian.postgresql.org/cgi-bin/pgpatches
    
    It will be applied as soon as one of the PostgreSQL committers reviews
    and approves it.
    
    ---------------------------------------------------------------------------
    
    
    Hannu Krosing wrote:
    > On K, 2005-08-17 at 15:40 -0400, Tom Lane wrote:
    > >                            Saatja: 
    > > Tom Lane <tgl@sss.pgh.pa.us>
    > >                           Kellele: 
    > > Bruce Momjian
    > > <pgman@candle.pha.pa.us>, Hannu
    > > Krosing <hannu@tm.ee>, Neil Conway
    > > <neilc@samurai.com>, pgsql-
    > > patches@postgresql.org
    > >                             Teema: 
    > > Re: [PATCHES] PATCH to allow
    > > concurrent VACUUMs to not lock each
    > >                           Kuup?ev: 
    > > Wed, 17 Aug 2005 15:40:53 -0400
    > > (22:40 EEST)
    > > 
    > > Just for the archives, attached is as far as I'd gotten with cleaning
    > > up
    > > Hannu's patch before I realized that it wasn't doing what it needed to
    > > do.  This fixes an end-of-transaction race condition (can't unset
    > > inVacuum before xact end, unless you want OldestXmin going backwards
    > > from the point of view of other people) and improves the documentation
    > > of what's going on.  But unless someone can convince me that it's safe
    > > to mess with GetSnapshotData, it's unlikely this'll ever get applied.
    > > 
    > > 
    > > 
    > 
    > Attached is a patch, based on you last one, which messes with
    > GetSnapshotData in what I think is a safe way.
    > 
    > It introduces another attribute to PROC , proc->nonInVacuumXmin and
    > computes this in addition to prox->xmin inside GetSnapshotData.
    > 
    > When (and only when) GetOldestXmin is called with ignoreVacuum=true,
    > then proc->nonInVacuumXmin is checked instead of prox->xmin.
    > 
    > I believe that this will make this change invisible to all other places
    > where GetSnapshotData or GetOldestXmin is used.
    > 
    > -- 
    > Hannu Krosing <hannu@skype.net>
    > 
    > 
    > 
    > 
    > 
    
    [ Attachment, skipping... ]
    
    > 
    > ---------------------------(end of broadcast)---------------------------
    > TIP 9: In versions below 8.0, the planner will ignore your desire to
    >        choose an index scan if your joining column's datatypes do not
    >        match
    
    -- 
      Bruce Momjian   http://candle.pha.pa.us
      SRA OSS, Inc.   http://www.sraoss.com
    
      + If your life is a hard drive, Christ can be your backup. +
    
    

21. Re: PATCH to allow concurrent VACUUMs to not lock each

    Bruce Momjian <bruce@momjian.us> — 2006-07-30T02:16:27Z

    Alvaro has just applied a modified version of this patch.
    
    ---------------------------------------------------------------------------
    
    Hannu Krosing wrote:
    > On E, 2005-05-23 at 11:42 -0400, Tom Lane wrote:
    > > Hannu Krosing <hannu@skype.net> writes:
    > > > I can't think of any other cases where it could matter, as at least the
    > > > work done inside vacuum_rel() itself seema non-rollbackable.
    > > 
    > > VACUUM FULL's tuple-moving is definitely roll-back-able, so it might be
    > > prudent to only do this for lazy VACUUM.  But on the other hand, VACUUM
    > > FULL holds an exclusive lock on the table so no one else is going to see
    > > its effects concurrently anyway.
    > 
    > Ok, this is a new version of the vacuum patch with the following changes
    > following some suggestions in this thread.
    > 
    > * changed the patch to affect only lazy vacuum 
    > * moved inVacuum handling to use PG_TRY
    > * moved vac_update_relstats() out of lazy_vacuum_rel into a separate
    >   transaction. The code to do this may not be the prettiest, maybe it
    >   should use a separate struct.
    > 
    > -- 
    > Hannu Krosing <hannu@skype.net>
    
    [ Attachment, skipping... ]
    
    > 
    > ---------------------------(end of broadcast)---------------------------
    > TIP 9: In versions below 8.0, the planner will ignore your desire to
    >        choose an index scan if your joining column's datatypes do not
    >        match
    
    -- 
      Bruce Momjian   bruce@momjian.us
      EnterpriseDB    http://www.enterprisedb.com
    
      + If your life is a hard drive, Christ can be your backup. +
    
    

22. Re: PATCH to allow concurrent VACUUMs to not lock each

    Alvaro Herrera <alvherre@commandprompt.com> — 2006-07-30T18:11:42Z

    Bruce Momjian wrote:
    > 
    > Alvaro has just applied a modified version of this patch.
    
    Hannu, I'm curious:
    
    > Hannu Krosing wrote:
    
    > > Ok, this is a new version of the vacuum patch with the following changes
    > > following some suggestions in this thread.
    > > 
    > > * changed the patch to affect only lazy vacuum 
    > > * moved inVacuum handling to use PG_TRY
    > > * moved vac_update_relstats() out of lazy_vacuum_rel into a separate
    > >   transaction. The code to do this may not be the prettiest, maybe it
    > >   should use a separate struct.
    
    What was idea behind moving vac_update_relstats to a separate
    transaction?  I'm wondering if it's still needed, if it further enhances
    the system somehow, or your patch did something differently than what
    was applied.
    
    -- 
    Alvaro Herrera                                http://www.CommandPrompt.com/
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
    
    

23. Re: PATCH to allow concurrent VACUUMs to not lock each

    Hannu Krosing <hannu@skype.net> — 2006-07-30T18:55:05Z

    Ühel kenal päeval, P, 2006-07-30 kell 14:11, kirjutas Alvaro Herrera:
    > Bruce Momjian wrote:
    > > 
    > > Alvaro has just applied a modified version of this patch.
    > 
    > Hannu, I'm curious:
    > 
    > > Hannu Krosing wrote:
    > 
    > > > Ok, this is a new version of the vacuum patch with the following changes
    > > > following some suggestions in this thread.
    > > > 
    > > > * changed the patch to affect only lazy vacuum 
    > > > * moved inVacuum handling to use PG_TRY
    > > > * moved vac_update_relstats() out of lazy_vacuum_rel into a separate
    > > >   transaction. The code to do this may not be the prettiest, maybe it
    > > >   should use a separate struct.
    
    > What was idea behind moving vac_update_relstats to a separate
    > transaction?  I'm wondering if it's still needed, if it further enhances
    > the system somehow, or your patch did something differently than what
    > was applied.
    
    The part of transactions which actually modified the data (iirc it updates
    relpages and reltuples in pg_class) is not safe to ignore by concurrent 
    vacuum, say a vacuum on pg_class .
    
    When the updating is done in the same trx that marks itself inVacuum,
    then these vacuums would be permitted to remove the old versions of
    pg_class and then, in case the inVacuum transaction aborts after that we
    are left with no valid pg_class row.
    
    The odds of this happening seem really small, but it might still be
    possibe.
    
    Or it might have been some other possible scenario. The main thing was,
    that vacuum can only ignore transaxtions which do not modify data and as
    vac_update_relstats does modify data it can't be run in isVacuum
    transaction.
    
    
    -- 
    ----------------
    Hannu Krosing
    Database Architect
    Skype Technologies OÜ
    Akadeemia tee 21 F, Tallinn, 12618, Estonia
    
    Skype me:  callto:hkrosing
    Get Skype for free:  http://www.skype.com
    
    
    

24. Re: PATCH to allow concurrent VACUUMs to not lock each

    Alvaro Herrera <alvherre@commandprompt.com> — 2006-07-30T19:18:34Z

    Hannu Krosing wrote:
    > Ühel kenal päeval, P, 2006-07-30 kell 14:11, kirjutas Alvaro Herrera:
    
    > > What was idea behind moving vac_update_relstats to a separate
    > > transaction?  I'm wondering if it's still needed, if it further enhances
    > > the system somehow, or your patch did something differently than what
    > > was applied.
    > 
    > The part of transactions which actually modified the data (iirc it updates
    > relpages and reltuples in pg_class) is not safe to ignore by concurrent 
    > vacuum, say a vacuum on pg_class .
    > 
    > When the updating is done in the same trx that marks itself inVacuum,
    > then these vacuums would be permitted to remove the old versions of
    > pg_class and then, in case the inVacuum transaction aborts after that we
    > are left with no valid pg_class row.
    
    I understand.  But the pg_class row is updated using in-place update,
    which means that it continues having the same Xmin as before -- to the
    rest of the system, it's exactly the same row as before, and it won't be
    removed.  So this is not a problem.  Thanks for clarifying.
    
    -- 
    Alvaro Herrera                                http://www.CommandPrompt.com/
    The PostgreSQL Company - Command Prompt, Inc.
    
    

25. Re: PATCH to allow concurrent VACUUMs to not lock each

    Tom Lane <tgl@sss.pgh.pa.us> — 2006-07-30T19:19:23Z

    Hannu Krosing <hannu@skype.net> writes:
    > hel kenal peval, P, 2006-07-30 kell 14:11, kirjutas Alvaro Herrera:
    >> What was idea behind moving vac_update_relstats to a separate
    >> transaction?  I'm wondering if it's still needed, if it further enhances
    >> the system somehow, or your patch did something differently than what
    >> was applied.
    
    > The part of transactions which actually modified the data (iirc it updates
    > relpages and reltuples in pg_class) is not safe to ignore by concurrent 
    > vacuum, say a vacuum on pg_class .
    
    But that's done as a nontransactional update, or at least was the last
    time I looked, so there's no need to do it in a separate xact.
    
    Knew I should have taken time to review that patch before it went in ...
    
    			regards, tom lane
    
    

26. Re: PATCH to allow concurrent VACUUMs to not lock each

    Alvaro Herrera <alvherre@commandprompt.com> — 2006-07-30T19:21:39Z

    Tom Lane wrote:
    > Hannu Krosing <hannu@skype.net> writes:
    > > Ühel kenal päeval, P, 2006-07-30 kell 14:11, kirjutas Alvaro Herrera:
    > >> What was idea behind moving vac_update_relstats to a separate
    > >> transaction?  I'm wondering if it's still needed, if it further enhances
    > >> the system somehow, or your patch did something differently than what
    > >> was applied.
    > 
    > > The part of transactions which actually modified the data (iirc it updates
    > > relpages and reltuples in pg_class) is not safe to ignore by concurrent 
    > > vacuum, say a vacuum on pg_class .
    > 
    > But that's done as a nontransactional update, or at least was the last
    > time I looked, so there's no need to do it in a separate xact.
    > 
    > Knew I should have taken time to review that patch before it went in ...
    
    Which one?  The one I applied doesn't have this change.  (You are still
    more than welcome to review it of course.)
    
    -- 
    Alvaro Herrera                                http://www.CommandPrompt.com/
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
    
    

27. Re: PATCH to allow concurrent VACUUMs to not lock each

    Tom Lane <tgl@sss.pgh.pa.us> — 2006-07-30T20:24:09Z

    Alvaro Herrera <alvherre@commandprompt.com> writes:
    > Tom Lane wrote:
    >> Knew I should have taken time to review that patch before it went in ...
    
    > Which one?  The one I applied doesn't have this change.
    
    Never mind --- I misunderstood the context of the discussion and thought
    you had made larger changes in the last version of the patch than I was
    expecting ...
    
    The patch as committed looks fine to me, modulo a couple of comments
    which I've fixed.
    
    One thing that slightly troubles me is that GetOldestXmin will now
    ignore a lazy vacuum's *own* xmin, which is not like the previous
    behavior.  Offhand I can't see a reason why this is not safe, but
    maybe it'd have been better for it to do
    
    + 		if (ignoreVacuum && proc->inVacuum && proc != MyProc)
    + 			continue;
    
    Thoughts?
    
    			regards, tom lane