Interesting glitch in autovacuum
I observed a curious bug in autovac just now. Since plain vacuum avoids
calling GetTransactionSnapshot, an autovac worker that happens not to
analyze any tables will never call GetTransactionSnapshot at all.
This means it will arrive at vac_update_datfrozenxid with
RecentGlobalXmin never having been changed from its boot value of
FirstNormalTransactionId, which means that it will fail to update the
database's datfrozenxid ... or, if the current value of datfrozenxid
is past 2 billion, that it will improperly advance datfrozenxid to
sometime in the future.
Once you get into this state in a reasonably idle database such as
template1, autovac is completely dead in the water: if it thinks
template1 needs to be vacuumed for wraparound, then every subsequent
worker will be launched at template1, every one will fail to advance
its datfrozenxid, rinse and repeat. Even before that happens, the
DB's datfrozenxid will prevent clog truncation, which might explain
some of the recent complaints.
I've only directly tested this in HEAD, but I suspect the problem goes
back a ways.
On reflection I'm not even sure that this is strictly an autovacuum
bug. It can be cast more generically as "RecentGlobalXmin getting
used without ever having been set", and it sure looks to me like the
HOT patch may have introduced a few risks of that sort.
I'm thinking that maybe an appropriate fix is to insert a
GetTransactionSnapshot call at the beginning of InitPostgres'
transaction, thus ensuring that every backend has some vaguely sane
value for RecentGlobalXmin before it tries to do any database access.
Another thought is that even with that, an autovac worker is likely
to reach vac_update_datfrozenxid with a RecentGlobalXmin value that
was computed at the start of its run, and is thus rather old.
I wonder why vac_update_datfrozenxid is using the variable at all
rather than doing GetOldestXmin? It's not like that function is
so performance-critical that it needs to avoid calling GetOldestXmin.
Lastly, now that we have the PROC_IN_VACUUM test in GetSnapshotData,
is it actually necessary for lazy vacuum to avoid setting a snapshot?
It seems like it might be a good idea for it to do so in order to
keep its RecentGlobalXmin reasonably current.
I've only looked at this in HEAD, but I am thinking that we have
a real problem here in both HEAD and 8.3. I'm less sure how bad
things are in the older branches.
Comments?
regards, tom lane
Tom Lane wrote:
I observed a curious bug in autovac just now. Since plain vacuum avoids
calling GetTransactionSnapshot, an autovac worker that happens not to
analyze any tables will never call GetTransactionSnapshot at all.
This means it will arrive at vac_update_datfrozenxid with
RecentGlobalXmin never having been changed from its boot value of
FirstNormalTransactionId, which means that it will fail to update the
database's datfrozenxid ... or, if the current value of datfrozenxid
is past 2 billion, that it will improperly advance datfrozenxid to
sometime in the future.
Ouch :-(
I've only directly tested this in HEAD, but I suspect the problem goes
back a ways.
Well, this logic was introduced in 8.2; I'm not sure if there's a
problem in 8.1, but I don't think so.
On reflection I'm not even sure that this is strictly an autovacuum
bug. It can be cast more generically as "RecentGlobalXmin getting
used without ever having been set", and it sure looks to me like the
HOT patch may have introduced a few risks of that sort.
Agreed.
Maybe we should boot RecentGlobalXmin with InvalidOid, and ensure where
it's going to be used that it's not that.
I'm thinking that maybe an appropriate fix is to insert a
GetTransactionSnapshot call at the beginning of InitPostgres'
transaction, thus ensuring that every backend has some vaguely sane
value for RecentGlobalXmin before it tries to do any database access.
AFAIR there's an "initial transaction" in InitPostgres or something like
that. Since it goes away quickly, it'd be a good place to ensure the
snapshot does not last much longer.
Another thought is that even with that, an autovac worker is likely
to reach vac_update_datfrozenxid with a RecentGlobalXmin value that
was computed at the start of its run, and is thus rather old.
I wonder why vac_update_datfrozenxid is using the variable at all
rather than doing GetOldestXmin? It's not like that function is
so performance-critical that it needs to avoid calling GetOldestXmin.
The function is called only once per autovacuum iteration, and once in
manually-invoked vacuum, so certainly it's not performance-critical.
Lastly, now that we have the PROC_IN_VACUUM test in GetSnapshotData,
is it actually necessary for lazy vacuum to avoid setting a snapshot?
It seems like it might be a good idea for it to do so in order to
keep its RecentGlobalXmin reasonably current.
Hmm, I think I'd rather be inclined to get a snapshot just when it's
going to finish. That way, RecentGlobalXmin will be up to date even if
the
I've only looked at this in HEAD, but I am thinking that we have
a real problem here in both HEAD and 8.3. I'm less sure how bad
things are in the older branches.
8.2 does contain the vac_update_datfrozenxid problem at the very least.
Older versions do not have that logic, so they are probably safe.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
I observed a curious bug in autovac just now. ...
Maybe we should boot RecentGlobalXmin with InvalidOid, and ensure where
it's going to be used that it's not that.
Good idea --- an Assert at the use sites should be sufficient.
Lastly, now that we have the PROC_IN_VACUUM test in GetSnapshotData,
is it actually necessary for lazy vacuum to avoid setting a snapshot?
It seems like it might be a good idea for it to do so in order to
keep its RecentGlobalXmin reasonably current.
Hmm, I think I'd rather be inclined to get a snapshot just when it's
going to finish.
I'm worried about keeping RecentGlobalXmin up to date during the
vacuums, not only at the end, because it will be used for HOT page
pruning during the vacuums.
regards, tom lane
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
Lastly, now that we have the PROC_IN_VACUUM test in GetSnapshotData,
is it actually necessary for lazy vacuum to avoid setting a snapshot?
It seems like it might be a good idea for it to do so in order to
keep its RecentGlobalXmin reasonably current.Hmm, I think I'd rather be inclined to get a snapshot just when it's
going to finish.I'm worried about keeping RecentGlobalXmin up to date during the
vacuums, not only at the end, because it will be used for HOT page
pruning during the vacuums.
Oh, I see. I didn't know we were doing HOT pruning during vacuum; does
it make sense?
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
I'm worried about keeping RecentGlobalXmin up to date during the
vacuums, not only at the end, because it will be used for HOT page
pruning during the vacuums.
Oh, I see. I didn't know we were doing HOT pruning during vacuum; does
it make sense?
Sorry, I got a bit confused there. The vacuum's intentional pruning
will use its own OldestXmin variable, which is known current at the
start of the vacuum (and I think there were even proposals to advance
it more frequently than that). However, a vacuum could require some
incidental system catalog fetches, which I think could result in
prune operations based on RecentGlobalXmin on the catalog pages
(cf index_getnext).
So it's probably not too terribly important ... as long as an autovac
worker doesn't live long enough that its RecentGlobalXmin threatens
to wrap around ... but I'm still interested in it as a code cleanup
measure. Skipping the transaction snapshot fetch was a performance
kluge, and if we don't need it any more I'd like to get rid of that
distinction between full and lazy vacuum behavior.
Anyway I think we are on the same page about the rest of the issues.
Did you want to work on fixing them, or shall I?
regards, tom lane
Alvaro Herrera wrote:
I didn't know we were doing HOT pruning during vacuum; does
it make sense?
Removing HOT-updated, dead tuples is a bit complicated. It needs to be
done one HOT-chain at a time, and the root line pointer needs to be
updated to the next live tuple in the chain. lazy_scan_heap() calls the
regular pruning function heap_page_prune() to deal with those before
doing the normal scan of line pointers.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Wed, 2008-09-10 at 11:52 -0400, Tom Lane wrote:
I'm thinking that maybe an appropriate fix is to insert a
GetTransactionSnapshot call at the beginning of InitPostgres'
transaction, thus ensuring that every backend has some vaguely sane
value for RecentGlobalXmin before it tries to do any database access.
Can't we just set RecentGlobalXmin without getting a Snapshot? There's
no need for a snapshot at that point is there? Just lock ProcArrayLock,
read GlobalXmin, drop lock.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
On Wed, 2008-09-10 at 11:52 -0400, Tom Lane wrote:
I'm thinking that maybe an appropriate fix is to insert a
GetTransactionSnapshot call at the beginning of InitPostgres'
transaction, thus ensuring that every backend has some vaguely sane
value for RecentGlobalXmin before it tries to do any database access.
Can't we just set RecentGlobalXmin without getting a Snapshot?
Well, certainly building an MVCC snapshot is more than we (appear to)
need, but I'm of the opinion that what we have got here is an unexpected
way in which these specialized transactions are unlike all others.
I think the safest fix is to make them more like normal transactions,
rather than invent still other ways to do things. If there were a
serious performance argument against that, then yeah, but I don't see
one. Backend startup isn't cheap in any case, and neither is a VACUUM,
so the incremental cost involved here seems negligible.
There's
no need for a snapshot at that point is there? Just lock ProcArrayLock,
read GlobalXmin, drop lock.
There's no "GlobalXmin" variable. We'd have to use GetOldestXmin();
which is cheaper than GetSnapshotData, but not hugely so.
regards, tom lane
Tom Lane wrote:
Sorry, I got a bit confused there. The vacuum's intentional pruning
will use its own OldestXmin variable, which is known current at the
start of the vacuum (and I think there were even proposals to advance
it more frequently than that). However, a vacuum could require some
incidental system catalog fetches, which I think could result in
prune operations based on RecentGlobalXmin on the catalog pages
(cf index_getnext).
Hmm, right, and what Heikki said too.
Anyway I think we are on the same page about the rest of the issues.
Did you want to work on fixing them, or shall I?
Is this more or less what you had in mind?
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachments:
ensure-recentglobal.patchtext/x-diff; charset=us-asciiDownload
Index: src/backend/access/heap/heapam.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/heap/heapam.c,v
retrieving revision 1.262
diff -c -p -r1.262 heapam.c
*** src/backend/access/heap/heapam.c 11 Aug 2008 11:05:10 -0000 1.262
--- src/backend/access/heap/heapam.c 10 Sep 2008 19:50:36 -0000
*************** heapgetpage(HeapScanDesc scan, BlockNumb
*** 219,224 ****
--- 219,225 ----
/*
* Prune and repair fragmentation for the whole page, if possible.
*/
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
heap_page_prune_opt(scan->rs_rd, buffer, RecentGlobalXmin);
/*
*************** heap_hot_search_buffer(ItemPointer tid,
*** 1469,1474 ****
--- 1470,1477 ----
if (all_dead)
*all_dead = true;
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
+
Assert(ItemPointerGetBlockNumber(tid) == BufferGetBlockNumber(buffer));
offnum = ItemPointerGetOffsetNumber(tid);
at_chain_start = true;
Index: src/backend/access/index/indexam.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/index/indexam.c,v
retrieving revision 1.109
diff -c -p -r1.109 indexam.c
*** src/backend/access/index/indexam.c 19 Jun 2008 00:46:03 -0000 1.109
--- src/backend/access/index/indexam.c 10 Sep 2008 19:51:38 -0000
*************** index_getnext(IndexScanDesc scan, ScanDi
*** 419,424 ****
--- 419,426 ----
SCAN_CHECKS;
GET_SCAN_PROCEDURE(amgettuple);
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
+
/*
* We always reset xs_hot_dead; if we are here then either we are just
* starting the scan, or we previously returned a visible tuple, and in
Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.376
diff -c -p -r1.376 vacuum.c
*** src/backend/commands/vacuum.c 13 Aug 2008 00:07:50 -0000 1.376
--- src/backend/commands/vacuum.c 10 Sep 2008 20:17:32 -0000
*************** vac_update_datfrozenxid(void)
*** 790,803 ****
bool dirty = false;
/*
! * Initialize the "min" calculation with RecentGlobalXmin. Any
! * not-yet-committed pg_class entries for new tables must have
! * relfrozenxid at least this high, because any other open xact must have
! * RecentXmin >= its PGPROC.xmin >= our RecentGlobalXmin; see
! * AddNewRelationTuple(). So we cannot produce a wrong minimum by
! * starting with this.
*/
! newFrozenXid = RecentGlobalXmin;
/*
* We must seqscan pg_class to find the minimum Xid, because there is no
--- 790,801 ----
bool dirty = false;
/*
! * Initialize the "min" calculation with GetOldestXmin, which is a
! * reasonable approximation to the minimum relfrozenxid for not-yet-
! * committed pg_class entries for new tables; see AddNewRelationTuple().
! * Se we cannot produce a wrong minimum by starting with this.
*/
! newFrozenXid = GetOldestXmin(true, true);
/*
* We must seqscan pg_class to find the minimum Xid, because there is no
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 990,1007 ****
/* Begin a transaction for vacuuming this relation */
StartTransactionCommand();
! if (vacstmt->full)
! {
! /* functions in indexes may want a snapshot set */
! PushActiveSnapshot(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 PROC_IN_VACUUM flag, which lets other
* concurrent VACUUMs know that they can ignore this one while
* determining their OldestXmin. (The reason we don't set it during a
* full VACUUM is exactly that we may have to run user- defined
--- 988,1003 ----
/* Begin a transaction for vacuuming this relation */
StartTransactionCommand();
! /*
! * Functions in indexes may want a snapshot set. Also, setting
! * a snapshot ensures that RecentGlobalXmin is kept truly recent.
! */
! PushActiveSnapshot(GetTransactionSnapshot());
!
! if (!vacstmt->full)
{
/*
! * In lazy vacuum, we can set the PROC_IN_VACUUM flag, which lets other
* concurrent VACUUMs know that they can ignore this one while
* determining their OldestXmin. (The reason we don't set it during a
* full VACUUM is exactly that we may have to run user- defined
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 1050,1057 ****
if (!onerel)
{
! if (vacstmt->full)
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
--- 1046,1052 ----
if (!onerel)
{
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 1082,1089 ****
(errmsg("skipping \"%s\" --- only table or database owner can vacuum it",
RelationGetRelationName(onerel))));
relation_close(onerel, lmode);
! if (vacstmt->full)
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
--- 1077,1083 ----
(errmsg("skipping \"%s\" --- only table or database owner can vacuum it",
RelationGetRelationName(onerel))));
relation_close(onerel, lmode);
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 1099,1106 ****
(errmsg("skipping \"%s\" --- cannot vacuum indexes, views, or special system tables",
RelationGetRelationName(onerel))));
relation_close(onerel, lmode);
! if (vacstmt->full)
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
--- 1093,1099 ----
(errmsg("skipping \"%s\" --- cannot vacuum indexes, views, or special system tables",
RelationGetRelationName(onerel))));
relation_close(onerel, lmode);
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 1115,1122 ****
if (isOtherTempNamespace(RelationGetNamespace(onerel)))
{
relation_close(onerel, lmode);
! if (vacstmt->full)
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
--- 1108,1114 ----
if (isOtherTempNamespace(RelationGetNamespace(onerel)))
{
relation_close(onerel, lmode);
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 1168,1175 ****
/*
* Complete the transaction and free all temporary memory used.
*/
! if (vacstmt->full)
! PopActiveSnapshot();
CommitTransactionCommand();
/*
--- 1160,1166 ----
/*
* Complete the transaction and free all temporary memory used.
*/
! PopActiveSnapshot();
CommitTransactionCommand();
/*
Index: src/backend/executor/nodeBitmapHeapscan.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/executor/nodeBitmapHeapscan.c,v
retrieving revision 1.29
diff -c -p -r1.29 nodeBitmapHeapscan.c
*** src/backend/executor/nodeBitmapHeapscan.c 19 Jun 2008 00:46:04 -0000 1.29
--- src/backend/executor/nodeBitmapHeapscan.c 10 Sep 2008 20:18:52 -0000
***************
*** 37,42 ****
--- 37,43 ----
#include "access/heapam.h"
#include "access/relscan.h"
+ #include "access/transam.h"
#include "executor/execdebug.h"
#include "executor/nodeBitmapHeapscan.h"
#include "pgstat.h"
*************** bitgetpage(HeapScanDesc scan, TBMIterate
*** 262,267 ****
--- 263,269 ----
/*
* Prune and repair fragmentation for the whole page, if possible.
*/
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
heap_page_prune_opt(scan->rs_rd, buffer, RecentGlobalXmin);
/*
Index: src/backend/utils/init/postinit.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/init/postinit.c,v
retrieving revision 1.184
diff -c -p -r1.184 postinit.c
*** src/backend/utils/init/postinit.c 12 May 2008 00:00:52 -0000 1.184
--- src/backend/utils/init/postinit.c 10 Sep 2008 20:19:56 -0000
***************
*** 47,52 ****
--- 47,53 ----
#include "utils/plancache.h"
#include "utils/portal.h"
#include "utils/relcache.h"
+ #include "utils/snapmgr.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
*************** InitPostgres(const char *in_dbname, Oid
*** 461,470 ****
on_shmem_exit(ShutdownPostgres, 0);
/*
! * Start a new transaction here before first access to db
*/
if (!bootstrap)
StartTransactionCommand();
/*
* Now that we have a transaction, we can take locks. Take a writer's
--- 462,476 ----
on_shmem_exit(ShutdownPostgres, 0);
/*
! * Start a new transaction here before first access to db, and get a
! * snapshot. We don't have a use for the snapshot itself, but we're
! * interested in the secondary effect that it sets RecentGlobalXmin.
*/
if (!bootstrap)
+ {
StartTransactionCommand();
+ (void) GetTransactionSnapshot();
+ }
/*
* Now that we have a transaction, we can take locks. Take a writer's
Index: src/backend/utils/time/snapmgr.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/time/snapmgr.c,v
retrieving revision 1.4
diff -c -p -r1.4 snapmgr.c
*** src/backend/utils/time/snapmgr.c 11 Jul 2008 02:10:14 -0000 1.4
--- src/backend/utils/time/snapmgr.c 10 Sep 2008 19:49:09 -0000
*************** static Snapshot SecondarySnapshot = NULL
*** 59,68 ****
* These are updated by GetSnapshotData. We initialize them this way
* for the convenience of TransactionIdIsInProgress: even in bootstrap
* mode, we don't want it to say that BootstrapTransactionId is in progress.
*/
TransactionId TransactionXmin = FirstNormalTransactionId;
TransactionId RecentXmin = FirstNormalTransactionId;
! TransactionId RecentGlobalXmin = FirstNormalTransactionId;
/*
* Elements of the list of registered snapshots.
--- 59,72 ----
* These are updated by GetSnapshotData. We initialize them this way
* for the convenience of TransactionIdIsInProgress: even in bootstrap
* mode, we don't want it to say that BootstrapTransactionId is in progress.
+ *
+ * RecentGlobalXmin is initialized to InvalidTransactionId, to ensure that no
+ * one tries to use a stale value. Readers should ensure that it has been set
+ * to something else before using it.
*/
TransactionId TransactionXmin = FirstNormalTransactionId;
TransactionId RecentXmin = FirstNormalTransactionId;
! TransactionId RecentGlobalXmin = InvalidTransactionId;
/*
* Elements of the list of registered snapshots.
On Wed, 2008-09-10 at 16:11 -0400, Tom Lane wrote:
If there were a serious performance argument against that, then yeah,
but I don't see one.
Maybe! Just finishing other patch then I'll be starting Hot Standby
discussions, so we'll see.
--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
Anyway I think we are on the same page about the rest of the issues.
Did you want to work on fixing them, or shall I?
Is this more or less what you had in mind?
Yeah, this looks like exactly what I had in mind for HEAD. I'm not too
sure about the back branches though. I think we could apply all of it
to 8.3, but further back is going to require a separate investigation
for each branch. Will you take that on?
regards, tom lane
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Tom Lane wrote:
Anyway I think we are on the same page about the rest of the issues.
Did you want to work on fixing them, or shall I?Is this more or less what you had in mind?
Yeah, this looks like exactly what I had in mind for HEAD. I'm not too
sure about the back branches though. I think we could apply all of it
to 8.3, but further back is going to require a separate investigation
for each branch. Will you take that on?
Sure.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
I wrote:
Yeah, this looks like exactly what I had in mind for HEAD. I'm not too
sure about the back branches though. I think we could apply all of it
to 8.3, but further back is going to require a separate investigation
for each branch. Will you take that on?
BTW, I did a quick look at all the uses of RecentGlobalXmin in the back
branches, and I think we might be all right before 8.2. The older
branches do in fact init RecentGlobalXmin to InvalidTransactionId,
and the only thing they use it for is "is this tuple dead to everyone"
tests. Since InvalidTransactionId compares older than anything else,
the only consequence of not having set it is overly-conservative
decisions not to mark tuples killed. So unless you see a problem I
missed, I think we only have an issue-worth-fixing in 8.2 and 8.3.
regards, tom lane
Tom Lane wrote:
BTW, I did a quick look at all the uses of RecentGlobalXmin in the back
branches, and I think we might be all right before 8.2. The older
branches do in fact init RecentGlobalXmin to InvalidTransactionId,
and the only thing they use it for is "is this tuple dead to everyone"
tests. Since InvalidTransactionId compares older than anything else,
the only consequence of not having set it is overly-conservative
decisions not to mark tuples killed. So unless you see a problem I
missed, I think we only have an issue-worth-fixing in 8.2 and 8.3.
Actually, 8.2 initializes it to InvalidTransactionId too, so it doesn't
seem like there's a problem there either. Since the problem stems from
using it as initializer for the Min() calculation, there's no actual bug
on 8.2 either. The bug only appeared on 8.3 when the initializer was
changed. And given that there's no HOT in 8.2, then there's no danger
of misusing it in page pruning either.
Still, I produced a patch to 8.2, but given that I'm not sure if it's
worth applying.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Attachments:
ensure-recentglobal-82.patchtext/x-diff; charset=us-asciiDownload
Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.342.2.5
diff -c -p -r1.342.2.5 vacuum.c
*** src/backend/commands/vacuum.c 11 Feb 2008 19:14:38 -0000 1.342.2.5
--- src/backend/commands/vacuum.c 11 Sep 2008 01:08:10 -0000
*************** vac_update_datfrozenxid(void)
*** 764,777 ****
bool dirty = false;
/*
! * Initialize the "min" calculation with RecentGlobalXmin. Any
! * not-yet-committed pg_class entries for new tables must have
! * relfrozenxid at least this high, because any other open xact must have
! * RecentXmin >= its PGPROC.xmin >= our RecentGlobalXmin; see
! * AddNewRelationTuple(). So we cannot produce a wrong minimum by
! * starting with this.
*/
! newFrozenXid = RecentGlobalXmin;
/*
* We must seqscan pg_class to find the minimum Xid, because there is no
--- 764,775 ----
bool dirty = false;
/*
! * Initialize the "min" calculation with GetOldestXmin, which is a
! * reasonable approximation to the minimum relfrozenxid for not-yet-
! * committed pg_class entries for new tables; see AddNewRelationTuple().
! * Se we cannot produce a wrong minimum by starting with this.
*/
! newFrozenXid = GetOldestXmin(true, true);
/*
* We must seqscan pg_class to find the minimum Xid, because there is no
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 965,982 ****
/* 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-
--- 963,978 ----
/* Begin a transaction for vacuuming this relation */
StartTransactionCommand();
! /*
! * Functions in indexes may want a snapshot set. Also, setting
! * a snapshot ensures that RecentGlobalXmin is kept truly recent.
! */
! ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
!
! if (!vacstmt->full)
{
/*
! * During a lazy VACUUM we can 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-
Index: src/backend/utils/init/postinit.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/init/postinit.c,v
retrieving revision 1.172
diff -c -p -r1.172 postinit.c
*** src/backend/utils/init/postinit.c 5 Nov 2006 22:42:09 -0000 1.172
--- src/backend/utils/init/postinit.c 10 Sep 2008 23:43:45 -0000
***************
*** 42,47 ****
--- 42,48 ----
#include "utils/guc.h"
#include "utils/portal.h"
#include "utils/relcache.h"
+ #include "utils/tqual.h"
#include "utils/syscache.h"
#include "pgstat.h"
*************** InitPostgres(const char *dbname, const c
*** 380,389 ****
on_shmem_exit(ShutdownPostgres, 0);
/*
! * Start a new transaction here before first access to db
*/
if (!bootstrap)
StartTransactionCommand();
/*
* Now that we have a transaction, we can take locks. Take a writer's
--- 381,395 ----
on_shmem_exit(ShutdownPostgres, 0);
/*
! * Start a new transaction here before first access to db, and get a
! * snapshot. We don't have a use for the snapshot itself, but we're
! * interested in the secondary effect that it sets RecentGlobalXmin.
*/
if (!bootstrap)
+ {
StartTransactionCommand();
+ (void) GetTransactionSnapshot();
+ }
/*
* Now that we have a transaction, we can take locks. Take a writer's
ensure-recentglobal-83.patchtext/x-diff; charset=us-asciiDownload
Index: src/backend/access/heap/heapam.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/heap/heapam.c,v
retrieving revision 1.249.2.2
diff -c -p -r1.249.2.2 heapam.c
*** src/backend/access/heap/heapam.c 8 Mar 2008 21:58:07 -0000 1.249.2.2
--- src/backend/access/heap/heapam.c 10 Sep 2008 23:18:00 -0000
*************** heapgetpage(HeapScanDesc scan, BlockNumb
*** 212,217 ****
--- 212,218 ----
/*
* Prune and repair fragmentation for the whole page, if possible.
*/
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
heap_page_prune_opt(scan->rs_rd, buffer, RecentGlobalXmin);
/*
*************** heap_hot_search_buffer(ItemPointer tid,
*** 1488,1493 ****
--- 1489,1496 ----
if (all_dead)
*all_dead = true;
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
+
Assert(ItemPointerGetBlockNumber(tid) == BufferGetBlockNumber(buffer));
offnum = ItemPointerGetOffsetNumber(tid);
at_chain_start = true;
Index: src/backend/access/index/indexam.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/index/indexam.c,v
retrieving revision 1.101
diff -c -p -r1.101 indexam.c
*** src/backend/access/index/indexam.c 1 Jan 2008 19:45:46 -0000 1.101
--- src/backend/access/index/indexam.c 10 Sep 2008 23:18:00 -0000
*************** index_getnext(IndexScanDesc scan, ScanDi
*** 419,424 ****
--- 419,426 ----
SCAN_CHECKS;
GET_SCAN_PROCEDURE(amgettuple);
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
+
/*
* We always reset xs_hot_dead; if we are here then either we are just
* starting the scan, or we previously returned a visible tuple, and in
Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.364.2.1
diff -c -p -r1.364.2.1 vacuum.c
*** src/backend/commands/vacuum.c 14 Mar 2008 17:26:00 -0000 1.364.2.1
--- src/backend/commands/vacuum.c 10 Sep 2008 23:21:34 -0000
*************** vac_update_datfrozenxid(void)
*** 781,794 ****
bool dirty = false;
/*
! * Initialize the "min" calculation with RecentGlobalXmin. Any
! * not-yet-committed pg_class entries for new tables must have
! * relfrozenxid at least this high, because any other open xact must have
! * RecentXmin >= its PGPROC.xmin >= our RecentGlobalXmin; see
! * AddNewRelationTuple(). So we cannot produce a wrong minimum by
! * starting with this.
*/
! newFrozenXid = RecentGlobalXmin;
/*
* We must seqscan pg_class to find the minimum Xid, because there is no
--- 781,792 ----
bool dirty = false;
/*
! * Initialize the "min" calculation with GetOldestXmin, which is a
! * reasonable approximation to the minimum relfrozenxid for not-yet-
! * committed pg_class entries for new tables; see AddNewRelationTuple().
! * Se we cannot produce a wrong minimum by starting with this.
*/
! newFrozenXid = GetOldestXmin(true, true);
/*
* We must seqscan pg_class to find the minimum Xid, because there is no
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 982,999 ****
/* 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 PROC_IN_VACUUM flag, which lets other
* concurrent VACUUMs know that they can ignore this one while
* determining their OldestXmin. (The reason we don't set it during a
* full VACUUM is exactly that we may have to run user- defined
--- 980,995 ----
/* Begin a transaction for vacuuming this relation */
StartTransactionCommand();
! /*
! * Functions in indexes may want a snapshot set. Also, setting
! * a snapshot ensures that RecentGlobalXmin is kept truly recent.
! */
! ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
!
! if (!vacstmt->full)
{
/*
! * During a lazy VACUUM we can set the PROC_IN_VACUUM flag, which lets other
* concurrent VACUUMs know that they can ignore this one while
* determining their OldestXmin. (The reason we don't set it during a
* full VACUUM is exactly that we may have to run user- defined
Index: src/backend/executor/nodeBitmapHeapscan.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/executor/nodeBitmapHeapscan.c,v
retrieving revision 1.22
diff -c -p -r1.22 nodeBitmapHeapscan.c
*** src/backend/executor/nodeBitmapHeapscan.c 1 Jan 2008 19:45:49 -0000 1.22
--- src/backend/executor/nodeBitmapHeapscan.c 10 Sep 2008 23:22:26 -0000
***************
*** 36,41 ****
--- 36,42 ----
#include "postgres.h"
#include "access/heapam.h"
+ #include "access/transam.h"
#include "executor/execdebug.h"
#include "executor/nodeBitmapHeapscan.h"
#include "pgstat.h"
*************** bitgetpage(HeapScanDesc scan, TBMIterate
*** 258,263 ****
--- 259,265 ----
/*
* Prune and repair fragmentation for the whole page, if possible.
*/
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
heap_page_prune_opt(scan->rs_rd, buffer, RecentGlobalXmin);
/*
Index: src/backend/utils/init/postinit.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/init/postinit.c,v
retrieving revision 1.180
diff -c -p -r1.180 postinit.c
*** src/backend/utils/init/postinit.c 1 Jan 2008 19:45:53 -0000 1.180
--- src/backend/utils/init/postinit.c 10 Sep 2008 23:25:26 -0000
***************
*** 44,49 ****
--- 44,50 ----
#include "utils/plancache.h"
#include "utils/portal.h"
#include "utils/relcache.h"
+ #include "utils/tqual.h"
#include "utils/syscache.h"
*************** InitPostgres(const char *in_dbname, Oid
*** 457,466 ****
on_shmem_exit(ShutdownPostgres, 0);
/*
! * Start a new transaction here before first access to db
*/
if (!bootstrap)
StartTransactionCommand();
/*
* Now that we have a transaction, we can take locks. Take a writer's
--- 458,472 ----
on_shmem_exit(ShutdownPostgres, 0);
/*
! * Start a new transaction here before first access to db, and get a
! * snapshot. We don't have a use for the snapshot itself, but we're
! * interested in the secondary effect that it sets RecentGlobalXmin.
*/
if (!bootstrap)
+ {
StartTransactionCommand();
+ (void) GetTransactionSnapshot();
+ }
/*
* Now that we have a transaction, we can take locks. Take a writer's
Index: src/backend/utils/time/tqual.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/time/tqual.c,v
retrieving revision 1.109
diff -c -p -r1.109 tqual.c
*** src/backend/utils/time/tqual.c 1 Jan 2008 19:45:55 -0000 1.109
--- src/backend/utils/time/tqual.c 10 Sep 2008 23:22:59 -0000
*************** Snapshot ActiveSnapshot = NULL;
*** 75,84 ****
* These are updated by GetSnapshotData. We initialize them this way
* for the convenience of TransactionIdIsInProgress: even in bootstrap
* mode, we don't want it to say that BootstrapTransactionId is in progress.
*/
TransactionId TransactionXmin = FirstNormalTransactionId;
TransactionId RecentXmin = FirstNormalTransactionId;
! TransactionId RecentGlobalXmin = FirstNormalTransactionId;
/* local functions */
static bool XidInMVCCSnapshot(TransactionId xid, Snapshot snapshot);
--- 75,88 ----
* These are updated by GetSnapshotData. We initialize them this way
* for the convenience of TransactionIdIsInProgress: even in bootstrap
* mode, we don't want it to say that BootstrapTransactionId is in progress.
+ *
+ * RecentGlobalXmin is initialized to InvalidTransactionId, to ensure that no
+ * one tries to use a stale value. Readers should ensure that it has been set
+ * to something else before using it.
*/
TransactionId TransactionXmin = FirstNormalTransactionId;
TransactionId RecentXmin = FirstNormalTransactionId;
! TransactionId RecentGlobalXmin = InvalidTransactionId;
/* local functions */
static bool XidInMVCCSnapshot(TransactionId xid, Snapshot snapshot);
ensure-recentglobal-head.patchtext/x-diff; charset=us-asciiDownload
Index: src/backend/access/heap/heapam.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/heap/heapam.c,v
retrieving revision 1.262
diff -c -p -r1.262 heapam.c
*** src/backend/access/heap/heapam.c 11 Aug 2008 11:05:10 -0000 1.262
--- src/backend/access/heap/heapam.c 10 Sep 2008 19:50:36 -0000
*************** heapgetpage(HeapScanDesc scan, BlockNumb
*** 219,224 ****
--- 219,225 ----
/*
* Prune and repair fragmentation for the whole page, if possible.
*/
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
heap_page_prune_opt(scan->rs_rd, buffer, RecentGlobalXmin);
/*
*************** heap_hot_search_buffer(ItemPointer tid,
*** 1469,1474 ****
--- 1470,1477 ----
if (all_dead)
*all_dead = true;
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
+
Assert(ItemPointerGetBlockNumber(tid) == BufferGetBlockNumber(buffer));
offnum = ItemPointerGetOffsetNumber(tid);
at_chain_start = true;
Index: src/backend/access/index/indexam.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/index/indexam.c,v
retrieving revision 1.109
diff -c -p -r1.109 indexam.c
*** src/backend/access/index/indexam.c 19 Jun 2008 00:46:03 -0000 1.109
--- src/backend/access/index/indexam.c 10 Sep 2008 19:51:38 -0000
*************** index_getnext(IndexScanDesc scan, ScanDi
*** 419,424 ****
--- 419,426 ----
SCAN_CHECKS;
GET_SCAN_PROCEDURE(amgettuple);
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
+
/*
* We always reset xs_hot_dead; if we are here then either we are just
* starting the scan, or we previously returned a visible tuple, and in
Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.376
diff -c -p -r1.376 vacuum.c
*** src/backend/commands/vacuum.c 13 Aug 2008 00:07:50 -0000 1.376
--- src/backend/commands/vacuum.c 10 Sep 2008 20:17:32 -0000
*************** vac_update_datfrozenxid(void)
*** 790,803 ****
bool dirty = false;
/*
! * Initialize the "min" calculation with RecentGlobalXmin. Any
! * not-yet-committed pg_class entries for new tables must have
! * relfrozenxid at least this high, because any other open xact must have
! * RecentXmin >= its PGPROC.xmin >= our RecentGlobalXmin; see
! * AddNewRelationTuple(). So we cannot produce a wrong minimum by
! * starting with this.
*/
! newFrozenXid = RecentGlobalXmin;
/*
* We must seqscan pg_class to find the minimum Xid, because there is no
--- 790,801 ----
bool dirty = false;
/*
! * Initialize the "min" calculation with GetOldestXmin, which is a
! * reasonable approximation to the minimum relfrozenxid for not-yet-
! * committed pg_class entries for new tables; see AddNewRelationTuple().
! * Se we cannot produce a wrong minimum by starting with this.
*/
! newFrozenXid = GetOldestXmin(true, true);
/*
* We must seqscan pg_class to find the minimum Xid, because there is no
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 990,1007 ****
/* Begin a transaction for vacuuming this relation */
StartTransactionCommand();
! if (vacstmt->full)
! {
! /* functions in indexes may want a snapshot set */
! PushActiveSnapshot(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 PROC_IN_VACUUM flag, which lets other
* concurrent VACUUMs know that they can ignore this one while
* determining their OldestXmin. (The reason we don't set it during a
* full VACUUM is exactly that we may have to run user- defined
--- 988,1003 ----
/* Begin a transaction for vacuuming this relation */
StartTransactionCommand();
! /*
! * Functions in indexes may want a snapshot set. Also, setting
! * a snapshot ensures that RecentGlobalXmin is kept truly recent.
! */
! PushActiveSnapshot(GetTransactionSnapshot());
!
! if (!vacstmt->full)
{
/*
! * In lazy vacuum, we can set the PROC_IN_VACUUM flag, which lets other
* concurrent VACUUMs know that they can ignore this one while
* determining their OldestXmin. (The reason we don't set it during a
* full VACUUM is exactly that we may have to run user- defined
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 1050,1057 ****
if (!onerel)
{
! if (vacstmt->full)
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
--- 1046,1052 ----
if (!onerel)
{
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 1082,1089 ****
(errmsg("skipping \"%s\" --- only table or database owner can vacuum it",
RelationGetRelationName(onerel))));
relation_close(onerel, lmode);
! if (vacstmt->full)
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
--- 1077,1083 ----
(errmsg("skipping \"%s\" --- only table or database owner can vacuum it",
RelationGetRelationName(onerel))));
relation_close(onerel, lmode);
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 1099,1106 ****
(errmsg("skipping \"%s\" --- cannot vacuum indexes, views, or special system tables",
RelationGetRelationName(onerel))));
relation_close(onerel, lmode);
! if (vacstmt->full)
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
--- 1093,1099 ----
(errmsg("skipping \"%s\" --- cannot vacuum indexes, views, or special system tables",
RelationGetRelationName(onerel))));
relation_close(onerel, lmode);
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 1115,1122 ****
if (isOtherTempNamespace(RelationGetNamespace(onerel)))
{
relation_close(onerel, lmode);
! if (vacstmt->full)
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
--- 1108,1114 ----
if (isOtherTempNamespace(RelationGetNamespace(onerel)))
{
relation_close(onerel, lmode);
! PopActiveSnapshot();
CommitTransactionCommand();
return;
}
*************** vacuum_rel(Oid relid, VacuumStmt *vacstm
*** 1168,1175 ****
/*
* Complete the transaction and free all temporary memory used.
*/
! if (vacstmt->full)
! PopActiveSnapshot();
CommitTransactionCommand();
/*
--- 1160,1166 ----
/*
* Complete the transaction and free all temporary memory used.
*/
! PopActiveSnapshot();
CommitTransactionCommand();
/*
Index: src/backend/executor/nodeBitmapHeapscan.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/executor/nodeBitmapHeapscan.c,v
retrieving revision 1.29
diff -c -p -r1.29 nodeBitmapHeapscan.c
*** src/backend/executor/nodeBitmapHeapscan.c 19 Jun 2008 00:46:04 -0000 1.29
--- src/backend/executor/nodeBitmapHeapscan.c 10 Sep 2008 20:18:52 -0000
***************
*** 37,42 ****
--- 37,43 ----
#include "access/heapam.h"
#include "access/relscan.h"
+ #include "access/transam.h"
#include "executor/execdebug.h"
#include "executor/nodeBitmapHeapscan.h"
#include "pgstat.h"
*************** bitgetpage(HeapScanDesc scan, TBMIterate
*** 262,267 ****
--- 263,269 ----
/*
* Prune and repair fragmentation for the whole page, if possible.
*/
+ Assert(TransactionIdIsValid(RecentGlobalXmin));
heap_page_prune_opt(scan->rs_rd, buffer, RecentGlobalXmin);
/*
Index: src/backend/utils/init/postinit.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/init/postinit.c,v
retrieving revision 1.184
diff -c -p -r1.184 postinit.c
*** src/backend/utils/init/postinit.c 12 May 2008 00:00:52 -0000 1.184
--- src/backend/utils/init/postinit.c 10 Sep 2008 20:19:56 -0000
***************
*** 47,52 ****
--- 47,53 ----
#include "utils/plancache.h"
#include "utils/portal.h"
#include "utils/relcache.h"
+ #include "utils/snapmgr.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
*************** InitPostgres(const char *in_dbname, Oid
*** 461,470 ****
on_shmem_exit(ShutdownPostgres, 0);
/*
! * Start a new transaction here before first access to db
*/
if (!bootstrap)
StartTransactionCommand();
/*
* Now that we have a transaction, we can take locks. Take a writer's
--- 462,476 ----
on_shmem_exit(ShutdownPostgres, 0);
/*
! * Start a new transaction here before first access to db, and get a
! * snapshot. We don't have a use for the snapshot itself, but we're
! * interested in the secondary effect that it sets RecentGlobalXmin.
*/
if (!bootstrap)
+ {
StartTransactionCommand();
+ (void) GetTransactionSnapshot();
+ }
/*
* Now that we have a transaction, we can take locks. Take a writer's
Index: src/backend/utils/time/snapmgr.c
===================================================================
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/time/snapmgr.c,v
retrieving revision 1.4
diff -c -p -r1.4 snapmgr.c
*** src/backend/utils/time/snapmgr.c 11 Jul 2008 02:10:14 -0000 1.4
--- src/backend/utils/time/snapmgr.c 10 Sep 2008 19:49:09 -0000
*************** static Snapshot SecondarySnapshot = NULL
*** 59,68 ****
* These are updated by GetSnapshotData. We initialize them this way
* for the convenience of TransactionIdIsInProgress: even in bootstrap
* mode, we don't want it to say that BootstrapTransactionId is in progress.
*/
TransactionId TransactionXmin = FirstNormalTransactionId;
TransactionId RecentXmin = FirstNormalTransactionId;
! TransactionId RecentGlobalXmin = FirstNormalTransactionId;
/*
* Elements of the list of registered snapshots.
--- 59,72 ----
* These are updated by GetSnapshotData. We initialize them this way
* for the convenience of TransactionIdIsInProgress: even in bootstrap
* mode, we don't want it to say that BootstrapTransactionId is in progress.
+ *
+ * RecentGlobalXmin is initialized to InvalidTransactionId, to ensure that no
+ * one tries to use a stale value. Readers should ensure that it has been set
+ * to something else before using it.
*/
TransactionId TransactionXmin = FirstNormalTransactionId;
TransactionId RecentXmin = FirstNormalTransactionId;
! TransactionId RecentGlobalXmin = InvalidTransactionId;
/*
* Elements of the list of registered snapshots.
On Wed, Sep 10, 2008 at 9:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
On reflection I'm not even sure that this is strictly an autovacuum
bug. It can be cast more generically as "RecentGlobalXmin getting
used without ever having been set", and it sure looks to me like the
HOT patch may have introduced a few risks of that sort.
ISTM that HOT may be safe here because even if RecentGlobalXmin is not
set and has the boot time value of FirstNormalTransactionId, the
heap_page_prune_opt() would just return without doing any real work.
This is OK except in VACUUM where we anyways use OldestXmin. So I
don't see a real problem here. But its good to fix the larger problem
as per the suggestion.
Thanks,
Pavan
--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com
"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
On Wed, Sep 10, 2008 at 9:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
On reflection I'm not even sure that this is strictly an autovacuum
bug. It can be cast more generically as "RecentGlobalXmin getting
used without ever having been set", and it sure looks to me like the
HOT patch may have introduced a few risks of that sort.
ISTM that HOT may be safe here because even if RecentGlobalXmin is not
set and has the boot time value of FirstNormalTransactionId, the
heap_page_prune_opt() would just return without doing any real work.
Until the XID counter advances past the 2billion mark. Then the
uninitialized RecentGlobalXmin would be "in the future" and would
result in mistaken decisions *to* prune, not to not prune.
In short this is a risk-of-data-loss bug. Once the patch is in
I think we ought to start thinking about a set of update releases...
regards, tom lane
Alvaro Herrera <alvherre@commandprompt.com> writes:
Actually, 8.2 initializes it to InvalidTransactionId too, so it doesn't
seem like there's a problem there either. Since the problem stems from
using it as initializer for the Min() calculation, there's no actual bug
on 8.2 either. The bug only appeared on 8.3 when the initializer was
changed. And given that there's no HOT in 8.2, then there's no danger
of misusing it in page pruning either.
I concur that 8.2 has no problem except in vac_update_datfrozenxid,
but I think it is an actual bug there. If newFrozenXid starts out as
InvalidTransactionId, it'll stay that way because InvalidTransactionId
sorts as older than anything else. The result will be that the routine
fails to advance datfrozenxid, which leads to exactly the type of
autovacuum misbehavior that I saw in HEAD. (Actually there's another
problem in an assert-enabled build: it'll fail at the
Assert(TransactionIdIsNormal(newFrozenXid)) ...)
regards, tom lane