Thread: PATCH to allow concurrent VACUUMs to not lock each other out from cleaning old tuples
PATCH to allow concurrent VACUUMs to not lock each other out from cleaning old tuples
From
Hannu Krosing
Date:
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>
Attachment
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>
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
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
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
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>
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
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>
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
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
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>
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
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
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>
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
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
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
Index: src/backend/access/transam/twophase.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/access/transam/twophase.c,v
retrieving revision 1.9
diff -c -r1.9 twophase.c
*** src/backend/access/transam/twophase.c 31 Jul 2005 17:19:17 -0000 1.9
--- src/backend/access/transam/twophase.c 17 Aug 2005 19:35:48 -0000
***************
*** 273,278 ****
--- 273,279 ----
gxact->proc.pid = 0;
gxact->proc.databaseId = databaseid;
gxact->proc.roleId = owner;
+ gxact->proc.inVacuum = false;
gxact->proc.lwWaiting = false;
gxact->proc.lwExclusive = false;
gxact->proc.lwWaitLink = NULL;
Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.212
diff -c -r1.212 xact.c
*** src/backend/access/transam/xact.c 8 Aug 2005 19:17:22 -0000 1.212
--- src/backend/access/transam/xact.c 17 Aug 2005 19:35:49 -0000
***************
*** 1516,1521 ****
--- 1516,1522 ----
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
MyProc->xid = InvalidTransactionId;
MyProc->xmin = InvalidTransactionId;
+ MyProc->inVacuum = false; /* must be cleared with xid/xmin */
/* Clear the subtransaction-XID cache too while holding the lock */
MyProc->subxids.nxids = 0;
***************
*** 1752,1757 ****
--- 1753,1759 ----
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
MyProc->xid = InvalidTransactionId;
MyProc->xmin = InvalidTransactionId;
+ MyProc->inVacuum = false; /* must be cleared with xid/xmin */
/* Clear the subtransaction-XID cache too while holding the lock */
MyProc->subxids.nxids = 0;
***************
*** 1915,1920 ****
--- 1917,1923 ----
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
MyProc->xid = InvalidTransactionId;
MyProc->xmin = InvalidTransactionId;
+ MyProc->inVacuum = false; /* must be cleared with xid/xmin */
/* Clear the subtransaction-XID cache too while holding the lock */
MyProc->subxids.nxids = 0;
Index: src/backend/access/transam/xlog.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/access/transam/xlog.c,v
retrieving revision 1.215
diff -c -r1.215 xlog.c
*** src/backend/access/transam/xlog.c 11 Aug 2005 21:11:43 -0000 1.215
--- src/backend/access/transam/xlog.c 17 Aug 2005 19:35:51 -0000
***************
*** 5226,5232 ****
* mustn't do this because StartupSUBTRANS hasn't been called yet.
*/
if (!InRecovery)
! TruncateSUBTRANS(GetOldestXmin(true));
if (!shutdown)
ereport(DEBUG2,
--- 5226,5232 ----
* mustn't do this because StartupSUBTRANS hasn't been called yet.
*/
if (!InRecovery)
! TruncateSUBTRANS(GetOldestXmin(true, false));
if (!shutdown)
ereport(DEBUG2,
Index: src/backend/catalog/index.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/catalog/index.c,v
retrieving revision 1.259
diff -c -r1.259 index.c
*** src/backend/catalog/index.c 12 Aug 2005 01:35:56 -0000 1.259
--- src/backend/catalog/index.c 17 Aug 2005 19:35:52 -0000
***************
*** 1433,1439 ****
else
{
snapshot = SnapshotAny;
! OldestXmin = GetOldestXmin(heapRelation->rd_rel->relisshared);
}
scan = heap_beginscan(heapRelation, /* relation */
--- 1433,1440 ----
else
{
snapshot = SnapshotAny;
! /* okay to ignore lazy VACUUMs here */
! OldestXmin = GetOldestXmin(heapRelation->rd_rel->relisshared, true);
}
scan = heap_beginscan(heapRelation, /* relation */
Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.312
diff -c -r1.312 vacuum.c
*** src/backend/commands/vacuum.c 29 Jul 2005 19:30:03 -0000 1.312
--- src/backend/commands/vacuum.c 17 Aug 2005 19:35:52 -0000
***************
*** 36,41 ****
--- 36,42 ----
#include "executor/executor.h"
#include "miscadmin.h"
#include "storage/freespace.h"
+ #include "storage/proc.h"
#include "storage/procarray.h"
#include "storage/smgr.h"
#include "tcop/pquery.h"
***************
*** 342,349 ****
* determining the cutoff with which we vacuum shared relations,
* it is not possible for that database to have a cutoff newer
* than OLDXMIN recorded in pg_database.
*/
! vacuum_set_xid_limits(vacstmt, false,
&initialOldestXmin,
&initialFreezeLimit);
}
--- 343,353 ----
* determining the cutoff with which we vacuum shared relations,
* it is not possible for that database to have a cutoff newer
* than OLDXMIN recorded in pg_database.
+ *
+ * We can't ignore concurrent lazy VACUUMs, because these values will
+ * be used to truncate clog below.
*/
! vacuum_set_xid_limits(vacstmt, false, false,
&initialOldestXmin,
&initialFreezeLimit);
}
***************
*** 583,595 ****
* vacuum_set_xid_limits() -- compute oldest-Xmin and freeze cutoff points
*/
void
! vacuum_set_xid_limits(VacuumStmt *vacstmt, bool sharedRel,
TransactionId *oldestXmin,
TransactionId *freezeLimit)
{
TransactionId limit;
! *oldestXmin = GetOldestXmin(sharedRel);
Assert(TransactionIdIsNormal(*oldestXmin));
--- 587,600 ----
* vacuum_set_xid_limits() -- compute oldest-Xmin and freeze cutoff points
*/
void
! vacuum_set_xid_limits(VacuumStmt *vacstmt,
! bool sharedRel, bool ignoreVacuum,
TransactionId *oldestXmin,
TransactionId *freezeLimit)
{
TransactionId limit;
! *oldestXmin = GetOldestXmin(sharedRel, ignoreVacuum);
Assert(TransactionIdIsNormal(*oldestXmin));
***************
*** 644,649 ****
--- 649,659 ----
* pg_class would've been obsoleted. Of course, this only works for
* fixed-size never-null columns, but these are.
*
+ * Another reason for doing it this way is that when we are in a lazy
+ * VACUUM and have inVacuum set, we mustn't do any updates --- somebody
+ * vacuuming pg_class might think they could delete a tuple marked with
+ * xmin = our xid.
+ *
* This routine is shared by full VACUUM, lazy VACUUM, and stand-alone
* ANALYZE.
*/
***************
*** 924,931 ****
/* Begin a transaction for vacuuming this relation */
StartTransactionCommand();
! /* functions in indexes may want a snapshot set */
! ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
/*
* Tell the cache replacement strategy that vacuum is causing all
--- 934,968 ----
/* Begin a transaction for vacuuming this relation */
StartTransactionCommand();
!
! if (vacstmt->full)
! {
! /* functions in indexes may want a snapshot set */
! ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
! }
! else
! {
! /*
! * During a lazy VACUUM we do not run any user-supplied functions,
! * and so it should be safe to not create a transaction snapshot.
! *
! * We can furthermore set the inVacuum flag, which lets other
! * concurrent VACUUMs know that they can ignore this one while
! * determining their OldestXmin. (The reason we don't set inVacuum
! * during a full VACUUM is exactly that we may have to run user-
! * defined functions for functional indexes, and we want to make
! * sure that if they use the snapshot set above, any tuples it
! * requires can't get removed from other tables. An index function
! * that depends on the contents of other tables is arguably broken,
! * but we won't break it here by violating transaction semantics.)
! *
! * Note: the inVacuum flag remains set until CommitTransaction or
! * AbortTransaction. We don't want to clear it until we reset
! * MyProc->xid/xmin, else OldestXmin might appear to go backwards,
! * which is probably Not Good.
! */
! MyProc->inVacuum = true;
! }
/*
* Tell the cache replacement strategy that vacuum is causing all
***************
*** 1105,1111 ****
i;
VRelStats *vacrelstats;
! vacuum_set_xid_limits(vacstmt, onerel->rd_rel->relisshared,
&OldestXmin, &FreezeLimit);
/*
--- 1142,1148 ----
i;
VRelStats *vacrelstats;
! vacuum_set_xid_limits(vacstmt, onerel->rd_rel->relisshared, true,
&OldestXmin, &FreezeLimit);
/*
Index: src/backend/commands/vacuumlazy.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/vacuumlazy.c,v
retrieving revision 1.56
diff -c -r1.56 vacuumlazy.c
*** src/backend/commands/vacuumlazy.c 29 Jul 2005 19:30:03 -0000 1.56
--- src/backend/commands/vacuumlazy.c 17 Aug 2005 19:35:52 -0000
***************
*** 142,148 ****
else
elevel = DEBUG2;
! vacuum_set_xid_limits(vacstmt, onerel->rd_rel->relisshared,
&OldestXmin, &FreezeLimit);
vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats));
--- 142,148 ----
else
elevel = DEBUG2;
! vacuum_set_xid_limits(vacstmt, onerel->rd_rel->relisshared, true,
&OldestXmin, &FreezeLimit);
vacrelstats = (LVRelStats *) palloc0(sizeof(LVRelStats));
Index: src/backend/storage/ipc/procarray.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/storage/ipc/procarray.c,v
retrieving revision 1.4
diff -c -r1.4 procarray.c
*** src/backend/storage/ipc/procarray.c 31 Jul 2005 17:19:18 -0000 1.4
--- src/backend/storage/ipc/procarray.c 17 Aug 2005 19:35:52 -0000
***************
*** 386,405 ****
* If allDbs is TRUE then all backends are considered; if allDbs is FALSE
* then only backends running in my own database are considered.
*
* This is used by VACUUM to decide which deleted tuples must be preserved
* in a table. allDbs = TRUE is needed for shared relations, but allDbs =
* FALSE is sufficient for non-shared relations, since only backends in my
! * own database could ever see the tuples in them.
*
* This is also used to determine where to truncate pg_subtrans. allDbs
! * must be TRUE for that case.
*
* Note: we include the currently running xids in the set of considered xids.
* This ensures that if a just-started xact has not yet set its snapshot,
* when it does set the snapshot it cannot set xmin less than what we compute.
*/
TransactionId
! GetOldestXmin(bool allDbs)
{
ProcArrayStruct *arrayP = procArray;
TransactionId result;
--- 386,409 ----
* If allDbs is TRUE then all backends are considered; if allDbs is FALSE
* then only backends running in my own database are considered.
*
+ * If ignoreVacuum is TRUE then backends with inVacuum set are ignored.
+ *
* This is used by VACUUM to decide which deleted tuples must be preserved
* in a table. allDbs = TRUE is needed for shared relations, but allDbs =
* FALSE is sufficient for non-shared relations, since only backends in my
! * own database could ever see the tuples in them. Also, we can ignore
! * concurrently running lazy VACUUMs because (a) they must be working on other
! * tables, and (b) they don't need to do snapshot-based lookups.
*
* This is also used to determine where to truncate pg_subtrans. allDbs
! * must be TRUE for that case, and ignoreVacuum FALSE.
*
* Note: we include the currently running xids in the set of considered xids.
* This ensures that if a just-started xact has not yet set its snapshot,
* when it does set the snapshot it cannot set xmin less than what we compute.
*/
TransactionId
! GetOldestXmin(bool allDbs, bool ignoreVacuum)
{
ProcArrayStruct *arrayP = procArray;
TransactionId result;
***************
*** 423,428 ****
--- 427,435 ----
{
PGPROC *proc = arrayP->procs[index];
+ if (ignoreVacuum && proc->inVacuum)
+ continue;
+
if (allDbs || proc->databaseId == MyDatabaseId)
{
/* Fetch xid just once - see GetNewTransactionId */
***************
*** 470,476 ****
* older than this are known not running any more.
* RecentGlobalXmin: the global xmin (oldest TransactionXmin across all
* running transactions). This is the same computation done by
! * GetOldestXmin(TRUE).
*----------
*/
Snapshot
--- 477,483 ----
* older than this are known not running any more.
* RecentGlobalXmin: the global xmin (oldest TransactionXmin across all
* running transactions). This is the same computation done by
! * GetOldestXmin(TRUE, FALSE).
*----------
*/
Snapshot
Index: src/backend/storage/lmgr/proc.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/storage/lmgr/proc.c,v
retrieving revision 1.162
diff -c -r1.162 proc.c
*** src/backend/storage/lmgr/proc.c 8 Aug 2005 03:11:55 -0000 1.162
--- src/backend/storage/lmgr/proc.c 17 Aug 2005 19:35:52 -0000
***************
*** 256,261 ****
--- 256,262 ----
MyProc->databaseId = MyDatabaseId;
/* Will be set properly after the session role id is determined */
MyProc->roleId = InvalidOid;
+ MyProc->inVacuum = false;
MyProc->lwWaiting = false;
MyProc->lwExclusive = false;
MyProc->lwWaitLink = NULL;
***************
*** 335,340 ****
--- 336,342 ----
MyProc->xmin = InvalidTransactionId;
MyProc->databaseId = MyDatabaseId;
MyProc->roleId = InvalidOid;
+ MyProc->inVacuum = false;
MyProc->lwWaiting = false;
MyProc->lwExclusive = false;
MyProc->lwWaitLink = NULL;
Index: src/include/commands/vacuum.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/commands/vacuum.h,v
retrieving revision 1.60
diff -c -r1.60 vacuum.h
*** src/include/commands/vacuum.h 14 Jul 2005 05:13:43 -0000 1.60
--- src/include/commands/vacuum.h 17 Aug 2005 19:35:52 -0000
***************
*** 133,139 ****
BlockNumber num_pages,
double num_tuples,
bool hasindex);
! extern void vacuum_set_xid_limits(VacuumStmt *vacstmt, bool sharedRel,
TransactionId *oldestXmin,
TransactionId *freezeLimit);
extern bool vac_is_partial_index(Relation indrel);
--- 133,140 ----
BlockNumber num_pages,
double num_tuples,
bool hasindex);
! extern void vacuum_set_xid_limits(VacuumStmt *vacstmt,
! bool sharedRel, bool ignoreVacuum,
TransactionId *oldestXmin,
TransactionId *freezeLimit);
extern bool vac_is_partial_index(Relation indrel);
Index: src/include/storage/proc.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/storage/proc.h,v
retrieving revision 1.80
diff -c -r1.80 proc.h
*** src/include/storage/proc.h 31 Jul 2005 17:19:22 -0000 1.80
--- src/include/storage/proc.h 17 Aug 2005 19:35:52 -0000
***************
*** 73,78 ****
--- 73,80 ----
Oid databaseId; /* OID of database this backend is using */
Oid roleId; /* OID of role using this backend */
+ bool inVacuum; /* true if current xact is a VACUUM */
+
/* Info about LWLock the process is currently waiting for, if any. */
bool lwWaiting; /* true if waiting for an LW lock */
bool lwExclusive; /* true if waiting for exclusive access */
Index: src/include/storage/procarray.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/storage/procarray.h,v
retrieving revision 1.3
diff -c -r1.3 procarray.h
*** src/include/storage/procarray.h 31 Jul 2005 17:19:22 -0000 1.3
--- src/include/storage/procarray.h 17 Aug 2005 19:35:52 -0000
***************
*** 24,30 ****
extern bool TransactionIdIsInProgress(TransactionId xid);
extern bool TransactionIdIsActive(TransactionId xid);
! extern TransactionId GetOldestXmin(bool allDbs);
extern PGPROC *BackendPidGetProc(int pid);
extern bool IsBackendPid(int pid);
--- 24,30 ----
extern bool TransactionIdIsInProgress(TransactionId xid);
extern bool TransactionIdIsActive(TransactionId xid);
! extern TransactionId GetOldestXmin(bool allDbs, bool ignoreVacuum);
extern PGPROC *BackendPidGetProc(int pid);
extern bool IsBackendPid(int pid);
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
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
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. +
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. +
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
Ü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
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.
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 ...
regards, tom lane
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
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