Re: [HACKERS] Bug in autovacuum.c?
Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian br...@momjian.us wrote: Agreed it is not worth it but I think we should at least C comment something. ? I think at a minimum we should set it to FirstNormalTransactionId. I think you should leave it well enough alone. Yes. The point of the existing coding is to ensure that we don't overestimate the table age at which vacuums should be forced. Bruce's proposed change would move the inaccuracy in the wrong direction, and thus cause some cases to not force autovac though an exact calculation would have done so. It's not worth trying to be exactly correct here, but I don't think that we want to err in that direction. If we had a symbol for the max normal XID, we could instead code like this: if (xidForceLimit FirstNormalTransactionId) xidForceLimit = LastNormalTransactionId; But AFAIR we don't, and I don't especially want to introduce one, because people might be misled by it. As you mentioned earlier, the XID space is circular so there isn't really a last XID. Sorry for the late reply but it seems HighestNormalTransactionId might be an apporopriate name. However, it is not code I normally deal with so unless someone who works in this area wants to make this cleanup, I will ignore the issue. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Robert Haas robertmh...@gmail.com writes: On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian br...@momjian.us wrote: Agreed it is not worth it but I think we should at least C comment something. I think at a minimum we should set it to FirstNormalTransactionId. I think you should leave it well enough alone. Yes. The point of the existing coding is to ensure that we don't overestimate the table age at which vacuums should be forced. Bruce's proposed change would move the inaccuracy in the wrong direction, and thus cause some cases to not force autovac though an exact calculation would have done so. It's not worth trying to be exactly correct here, but I don't think that we want to err in that direction. If we had a symbol for the max normal XID, we could instead code like this: if (xidForceLimit FirstNormalTransactionId) xidForceLimit = LastNormalTransactionId; But AFAIR we don't, and I don't especially want to introduce one, because people might be misled by it. As you mentioned earlier, the XID space is circular so there isn't really a last XID. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Greg Stark wrote: On Thu, Mar 31, 2011 at 10:59 PM, Bruce Momjian br...@momjian.us wrote: OK, just keep going below 100: ? ? ? ?105 - 5 ? ? ? ?104 - 4 ? ? ? ?103 - 3 ? ? ? ?102 - max_xid ? ? ? ?101 - max_xid - 1 ? ? ? ?100 - max_xid - 2 ? ? ? ? 99 - max_id ? ? ? ? 98 - max_id -1 Yeah, I think this is what the code is doing. Wouldn't you rather: ? ? ? ?105 - 5 ? ? ? ?104 - 4 ? ? ? ?103 - 3 ? ? ? ?102 - 3 ? ? ? ?101 - 3 ? ? ? ?100 - 3 ? ? ? ? 99 - max_id ? ? ? ? 98 - max_id -1 I think I would expect ? ? ? ?105 - 5 ? ? ? ?104 - 4 ? ? ? ?103 - 3 ? ? ? ?102 - max_id ? ? ? ?101 - max_id-1 ? ? ? ?100 - max_id-2 ? ? ? ? 99 - max_id-3 But it doesn't really matter either way, does it? We don't even allow setting vacuum_max_freeze_age to 2^31-1 or any value that would be close to triggering a problem here. It doesn't need to be that high because it is subtracted from the current xid counter, so if vacuum_max_freeze_age is 100, and the xid counter is 101, we see the problem. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
On Thu, Mar 31, 2011 at 5:59 PM, Bruce Momjian br...@momjian.us wrote: OK, just keep going below 100: 105 - 5 104 - 4 103 - 3 102 - max_xid 101 - max_xid - 1 100 - max_xid - 2 99 - max_id 98 - max_id -1 Wouldn't you rather: 105 - 5 104 - 4 103 - 3 102 - 3 101 - 3 100 - 3 99 - max_id 98 - max_id -1 Oh, quite right. Sorry I missed that. I suppose if we wanted to fix this for real, we'd want to get: 105-5 104-4 103-3 102-max_xid 101-max_xid-1 100-max_xid-2 99-max_xid-3 98-max_xid-4 But it doesn't seem worth getting excited about. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Robert Haas wrote: Oh, quite right. Sorry I missed that. I suppose if we wanted to fix this for real, we'd want to get: 105-5 104-4 103-3 102-max_xid 101-max_xid-1 100-max_xid-2 99-max_xid-3 98-max_xid-4 But it doesn't seem worth getting excited about. I think (?) the problem with that is the every time you wrap around you get more out of sync. :-O Thinking more, the problem is that when the xid counter wraps around from max_xid to 3, we jump the freeze horizon by three, e.g 5000 to 5003. So when, the freeze horizon wraps, we can either have that jump by three, e.g set it to FirstNormalTransactionId, or delay by three, e.g. set it to MaxTransactionId. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
On Fri, Apr 1, 2011 at 11:18 AM, Bruce Momjian br...@momjian.us wrote: Robert Haas wrote: Oh, quite right. Sorry I missed that. I suppose if we wanted to fix this for real, we'd want to get: 105-5 104-4 103-3 102-max_xid 101-max_xid-1 100-max_xid-2 99-max_xid-3 98-max_xid-4 But it doesn't seem worth getting excited about. I think (?) the problem with that is the every time you wrap around you get more out of sync. :-O It's not clear to me that it matters a bit, though. Thinking more, the problem is that when the xid counter wraps around from max_xid to 3, we jump the freeze horizon by three, e.g 5000 to 5003. So when, the freeze horizon wraps, we can either have that jump by three, e.g set it to FirstNormalTransactionId, or delay by three, e.g. set it to MaxTransactionId. So what? :-) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Robert Haas wrote: On Fri, Apr 1, 2011 at 11:18 AM, Bruce Momjian br...@momjian.us wrote: Robert Haas wrote: Oh, quite right. ?Sorry I missed that. ?I suppose if we wanted to fix this for real, we'd want to get: 105-5 104-4 103-3 102-max_xid 101-max_xid-1 100-max_xid-2 99-max_xid-3 98-max_xid-4 But it doesn't seem worth getting excited about. I think (?) the problem with that is the every time you wrap around you get more out of sync. ?:-O It's not clear to me that it matters a bit, though. To do the right thing every computation that passes over the xid wraparound bounary should subtract FirstNormalTransactionId, not just those that fall in the boundry. That would prevent the value from going backward and still allow the mapping you liked; it isn't worth it, but that is the right answer. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Excerpts from Bruce Momjian's message of vie abr 01 16:50:29 -0300 2011: To do the right thing every computation that passes over the xid wraparound bounary should subtract FirstNormalTransactionId, not just those that fall in the boundry. That would prevent the value from going backward and still allow the mapping you liked; it isn't worth it, but that is the right answer. This code is only concerned calculating an immediate the wrap horizon for the autovacuuming run that's about to take place. If it's wrong in one or three counts doesn't mean much. Consider what would happen if load was high and it would have taken 100 extra milliseconds to get to that bit: ReadNewTransactionId would have returned a value 3 transactions later. Furthermore, before this value is even used at all for vacuuming, there has to be a whole lot of inter-process signalling, a fork, and a new backend startup. I think this should be left alone. As you said, it isn't worth it. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Alvaro Herrera wrote: Excerpts from Bruce Momjian's message of vie abr 01 16:50:29 -0300 2011: To do the right thing every computation that passes over the xid wraparound bounary should subtract FirstNormalTransactionId, not just those that fall in the boundry. That would prevent the value from going backward and still allow the mapping you liked; it isn't worth it, but that is the right answer. This code is only concerned calculating an immediate the wrap horizon for the autovacuuming run that's about to take place. If it's wrong in one or three counts doesn't mean much. Consider what would happen if load was high and it would have taken 100 extra milliseconds to get to that bit: ReadNewTransactionId would have returned a value 3 transactions later. Furthermore, before this value is even used at all for vacuuming, there has to be a whole lot of inter-process signalling, a fork, and a new backend startup. I think this should be left alone. As you said, it isn't worth it. Agreed it is not worth it but I think we should at least C comment something. I think at a minimum we should set it to FirstNormalTransactionId. I am not so concerned about this case but about other cases where we are computing xid distances across the invalid range. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c new file mode 100644 index efc8e7c..244930f *** a/src/backend/postmaster/autovacuum.c --- b/src/backend/postmaster/autovacuum.c *** do_start_worker(void) *** 1107,1114 */ recentXid = ReadNewTransactionId(); xidForceLimit = recentXid - autovacuum_freeze_max_age; ! /* ensure it's a normal XID, else TransactionIdPrecedes misbehaves */ ! if (xidForceLimit FirstNormalTransactionId) xidForceLimit -= FirstNormalTransactionId; /* --- 1107,1120 */ recentXid = ReadNewTransactionId(); xidForceLimit = recentXid - autovacuum_freeze_max_age; ! /* ! * Adjust for xids in the invalid range, less than FirstNormalTransactionId. ! * We map the xids as though the invalid range did not exist. ! * We cannot simplify the test below because of underflow ! * issues with unsigned values. ! */ ! if (xidForceLimit FirstNormalTransactionId || /* in invalid range? */ ! recentXid autovacuum_freeze_max_age) /* did we underflow? */ xidForceLimit -= FirstNormalTransactionId; /* -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Bruce Momjian wrote: I think this should be left alone. As you said, it isn't worth it. Agreed it is not worth it but I think we should at least C comment something. I think at a minimum we should set it to FirstNormalTransactionId. I am not so concerned about this case but about other cases where we are computing xid distances across the invalid range. Please ignore the patch I had attached. I realize it just moves the problem around. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
On Apr 1, 2011, at 4:48 PM, Bruce Momjian wrote: I am not so concerned about this case but about other cases where we are computing xid distances across the invalid range. Bruce, I think you hit the nail on the head earlier: To do the right thing every computation that passes over the xid wraparound bounary should subtract FirstNormalTransactionId, not just those that fall in the boundry. Put another way: XID calculations should not use just +/-, but an operator (presumably a macro) that understands wraparound and the special values. Surely we have a similar problem in the code that increments XIDs, and possibly other places as well. -- Jim C. Nasby, Database Architect j...@nasby.net 512.569.9461 (cell) http://jim.nasby.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian br...@momjian.us wrote: Alvaro Herrera wrote: Excerpts from Bruce Momjian's message of vie abr 01 16:50:29 -0300 2011: To do the right thing every computation that passes over the xid wraparound bounary should subtract FirstNormalTransactionId, not just those that fall in the boundry. That would prevent the value from going backward and still allow the mapping you liked; it isn't worth it, but that is the right answer. This code is only concerned calculating an immediate the wrap horizon for the autovacuuming run that's about to take place. If it's wrong in one or three counts doesn't mean much. Consider what would happen if load was high and it would have taken 100 extra milliseconds to get to that bit: ReadNewTransactionId would have returned a value 3 transactions later. Furthermore, before this value is even used at all for vacuuming, there has to be a whole lot of inter-process signalling, a fork, and a new backend startup. I think this should be left alone. As you said, it isn't worth it. Agreed it is not worth it but I think we should at least C comment something. I think at a minimum we should set it to FirstNormalTransactionId. I think you should leave it well enough alone. I am not so concerned about this case but about other cases where we are computing xid distances across the invalid range. Such as? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Robert Haas wrote: On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian br...@momjian.us wrote: Alvaro Herrera wrote: Excerpts from Bruce Momjian's message of vie abr 01 16:50:29 -0300 2011: To do the right thing every computation that passes over the xid wraparound bounary should subtract FirstNormalTransactionId, not just those that fall in the boundry. ?That would prevent the value from going backward and still allow the mapping you liked; ?it isn't worth it, but that is the right answer. This code is only concerned calculating an immediate the wrap horizon for the autovacuuming run that's about to take place. ?If it's wrong in one or three counts doesn't mean much. ?Consider what would happen if load was high and it would have taken 100 extra milliseconds to get to that bit: ReadNewTransactionId would have returned a value 3 transactions later. ?Furthermore, before this value is even used at all for vacuuming, there has to be a whole lot of inter-process signalling, a fork, and a new backend startup. I think this should be left alone. ?As you said, it isn't worth it. Agreed it is not worth it but I think we should at least C comment something. ? I think at a minimum we should set it to FirstNormalTransactionId. I think you should leave it well enough alone. I am not so concerned about this case but about other cases where we are computing xid distances across the invalid range. Such as? This causes the freeze horizon to move backward slighly, but that is OK. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Robert Haas wrote: On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian br...@momjian.us wrote: Alvaro Herrera wrote: Excerpts from Bruce Momjian's message of vie abr 01 16:50:29 -0300 2011: To do the right thing every computation that passes over the xid wraparound bounary should subtract FirstNormalTransactionId, not just those that fall in the boundry. ?That would prevent the value from going backward and still allow the mapping you liked; ?it isn't worth it, but that is the right answer. This code is only concerned calculating an immediate the wrap horizon for the autovacuuming run that's about to take place. ?If it's wrong in one or three counts doesn't mean much. ?Consider what would happen if load was high and it would have taken 100 extra milliseconds to get to that bit: ReadNewTransactionId would have returned a value 3 transactions later. ?Furthermore, before this value is even used at all for vacuuming, there has to be a whole lot of inter-process signalling, a fork, and a new backend startup. I think this should be left alone. ?As you said, it isn't worth it. Agreed it is not worth it but I think we should at least C comment something. ? I think at a minimum we should set it to FirstNormalTransactionId. I think you should leave it well enough alone. OK, but we still need a C comment, which I suggested as: This causes the freeze horizon to move backward slighly, but that is OK. I am not so concerned about this case but about other cases where we are computing xid distances across the invalid range. Such as? Not sure. I have not had time to research this, but there might be cases where this backward movement matters --- remember our XIDs are valid only within about a 2 billion range, and we do less/greater comparisons in that range (using a macro). That macro is not going to cover over backward xid movement. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Bruce Momjian wrote:
I am not so concerned about this case but about other cases where we are
computing xid distances across the invalid range.
Such as?
Not sure. I have not had time to research this, but there might be
cases where this backward movement matters --- remember our XIDs are
valid only within about a 2 billion range, and we do less/greater
comparisons in that range (using a macro). That macro is not going to
cover over backward xid movement.
OK, I am done training for the day, and found this macro:
/* advance a transaction ID variable, handling wraparound correctly */
#define TransactionIdAdvance(dest) \
do { \
(dest)++; \
if ((dest) FirstNormalTransactionId) \
(dest) = FirstNormalTransactionId; \
} while(0)
which seems OK, but we the -= all over varsup.c
/*
* We'll refuse to continue assigning XIDs in interactive mode once we get
* within 1M transactions of data loss. This leaves lots of room for the
* DBA to fool around fixing things in a standalone backend, while not
* being significant compared to total XID space. (Note that since
* vacuuming requires one transaction per table cleaned, we had better be
* sure there's lots of XIDs left...)
*/
xidStopLimit = xidWrapLimit - 100;
if (xidStopLimit FirstNormalTransactionId)
xidStopLimit -= FirstNormalTransactionId;
Now I am not sure where to add a C comment. :-(
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Bug in autovacuum.c?
Looking over the autovacuum.c code, I see: /* * Determine the oldest datfrozenxid/relfrozenxid that we will allow to * pass without forcing a vacuum. (This limit can be tightened for * particular tables, but not loosened.) */ recentXid = ReadNewTransactionId(); xidForceLimit = recentXid - autovacuum_freeze_max_age; /* ensure it's a normal XID, else TransactionIdPrecedes misbehaves */ if (xidForceLimit FirstNormalTransactionId) xidForceLimit -= FirstNormalTransactionId; This last line doesn't look right to me; should it be: xidForceLimit = FirstNormalTransactionId; -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
On Thu, Mar 31, 2011 at 12:17 PM, Bruce Momjian br...@momjian.us wrote: Looking over the autovacuum.c code, I see: /* * Determine the oldest datfrozenxid/relfrozenxid that we will allow to * pass without forcing a vacuum. (This limit can be tightened for * particular tables, but not loosened.) */ recentXid = ReadNewTransactionId(); xidForceLimit = recentXid - autovacuum_freeze_max_age; /* ensure it's a normal XID, else TransactionIdPrecedes misbehaves */ if (xidForceLimit FirstNormalTransactionId) xidForceLimit -= FirstNormalTransactionId; This last line doesn't look right to me; should it be: xidForceLimit = FirstNormalTransactionId; That would probably work, but the existing coding actually makes more sense. It's essentially trying to scan backwards by autovacuum_freeze_max_age XIDs through the circular XID space. But the XID space isn't actually circular, because there are 3 special values. So if we land on one of those values we want to skip backward by 3. Here FirstNormalTransactionId doesn't represent itself, but rather the number of special XIDs that exist. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Excerpts from Robert Haas's message of jue mar 31 13:58:41 -0300 2011: On Thu, Mar 31, 2011 at 12:17 PM, Bruce Momjian br...@momjian.us wrote: Looking over the autovacuum.c code, I see: /* * Determine the oldest datfrozenxid/relfrozenxid that we will allow to * pass without forcing a vacuum. (This limit can be tightened for * particular tables, but not loosened.) */ recentXid = ReadNewTransactionId(); xidForceLimit = recentXid - autovacuum_freeze_max_age; /* ensure it's a normal XID, else TransactionIdPrecedes misbehaves */ if (xidForceLimit FirstNormalTransactionId) xidForceLimit -= FirstNormalTransactionId; This last line doesn't look right to me; should it be: xidForceLimit = FirstNormalTransactionId; That would probably work, but the existing coding actually makes more sense. It's essentially trying to scan backwards by autovacuum_freeze_max_age XIDs through the circular XID space. But the XID space isn't actually circular, because there are 3 special values. So if we land on one of those values we want to skip backward by 3. Here FirstNormalTransactionId doesn't represent itself, but rather the number of special XIDs that exist. Yeah, I think this change would have the effect of moving the freeze limit by one (or two?) counts. Given the moving nature of values returned by ReadNewTransactionId this would probably have no practical effect. Still, the code as is seems more natural to me (Tom wrote this bit IIRC, not me). -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Alvaro Herrera wrote: That would probably work, but the existing coding actually makes more sense. It's essentially trying to scan backwards by autovacuum_freeze_max_age XIDs through the circular XID space. But the XID space isn't actually circular, because there are 3 special values. So if we land on one of those values we want to skip backward by 3. Here FirstNormalTransactionId doesn't represent itself, but rather the number of special XIDs that exist. Yeah, I think this change would have the effect of moving the freeze limit by one (or two?) counts. Given the moving nature of values returned by ReadNewTransactionId this would probably have no practical effect. Still, the code as is seems more natural to me (Tom wrote this bit IIRC, not me). I am now thinking the code is correct --- it maps values from 0 to FirstNormalTransactionId into the top of the (unsigned) xid range. Unless someone objects, I will add a C comment about this behavior so future readers are not confused. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Bruce Momjian wrote: Yeah, I think this change would have the effect of moving the freeze limit by one (or two?) counts. Given the moving nature of values returned by ReadNewTransactionId this would probably have no practical effect. Still, the code as is seems more natural to me (Tom wrote this bit IIRC, not me). I am now thinking the code is correct --- it maps values from 0 to FirstNormalTransactionId into the top of the (unsigned) xid range. Unless someone objects, I will add a C comment about this behavior so future readers are not confused. OK, now I think it is wrong. :-) The effect is to map max xid + 1 to max xid - FirstNormalTransactionId(3) + 1, which makes the xid look like it is going backwards, less than max xid --- not good. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian br...@momjian.us wrote: Bruce Momjian wrote: Yeah, I think this change would have the effect of moving the freeze limit by one (or two?) counts. Given the moving nature of values returned by ReadNewTransactionId this would probably have no practical effect. Still, the code as is seems more natural to me (Tom wrote this bit IIRC, not me). I am now thinking the code is correct --- it maps values from 0 to FirstNormalTransactionId into the top of the (unsigned) xid range. Unless someone objects, I will add a C comment about this behavior so future readers are not confused. OK, now I think it is wrong. :-) The effect is to map max xid + 1 to max xid - FirstNormalTransactionId(3) + 1, which makes the xid look like it is going backwards, less than max xid --- not good. The XID space is *circular*. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Robert Haas wrote: On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian br...@momjian.us wrote: Bruce Momjian wrote: Yeah, I think this change would have the effect of moving the freeze limit by one (or two?) counts. ?Given the moving nature of values returned by ReadNewTransactionId this would probably have no practical effect. ?Still, the code as is seems more natural to me (Tom wrote this bit IIRC, not me). I am now thinking the code is correct --- it maps values from 0 to FirstNormalTransactionId into the top of the (unsigned) xid range. Unless someone objects, I will add a C comment about this behavior so future readers are not confused. OK, now I think it is wrong. ? :-) The effect is to map max xid + 1 to max xid - FirstNormalTransactionId(3) + 1, which makes the xid look like it is going backwards, less than max xid --- not good. The XID space is *circular*. Right but you would think that as the xid moves forward, the caculation of how far back to vacuum should move only forward. In this case, incrementing the xid by one would cause the vacuum horizon to move backward by two. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
On Thu, Mar 31, 2011 at 4:16 PM, Bruce Momjian br...@momjian.us wrote: Robert Haas wrote: On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian br...@momjian.us wrote: Bruce Momjian wrote: Yeah, I think this change would have the effect of moving the freeze limit by one (or two?) counts. ?Given the moving nature of values returned by ReadNewTransactionId this would probably have no practical effect. ?Still, the code as is seems more natural to me (Tom wrote this bit IIRC, not me). I am now thinking the code is correct --- it maps values from 0 to FirstNormalTransactionId into the top of the (unsigned) xid range. Unless someone objects, I will add a C comment about this behavior so future readers are not confused. OK, now I think it is wrong. ? :-) The effect is to map max xid + 1 to max xid - FirstNormalTransactionId(3) + 1, which makes the xid look like it is going backwards, less than max xid --- not good. The XID space is *circular*. Right but you would think that as the xid moves forward, the caculation of how far back to vacuum should move only forward. In this case, incrementing the xid by one would cause the vacuum horizon to move backward by two. I don't see how that would happen. The XID immediately preceding FirstNormalTransactionId is 2^32-1, and that's exactly what this calculation produces. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Robert Haas wrote: The effect is to map max xid + 1 to max xid - FirstNormalTransactionId(3) + 1, which makes the xid look like it is going backwards, less than max xid --- not good. The XID space is *circular*. Right but you would think that as the xid moves forward, the caculation of how far back to vacuum should move only forward. ?In this case, incrementing the xid by one would cause the vacuum horizon to move backward by two. I don't see how that would happen. The XID immediately preceding FirstNormalTransactionId is 2^32-1, and that's exactly what this calculation produces. OK, let me see if I understand --- the caculation is below: xidForceLimit = recentXid - autovacuum_freeze_max_age; if (xidForceLimit FirstNormalTransactionId) xidForceLimit -= FirstNormalTransactionId; The values: xidForceLimit Result --- max_xid-2 max_xid-2 max_xid-1 max_xid-1 max_xid max_xid 0 max_xid-3 - backward here 1 max_xid-2 2 max_xid-1 3 3 With the -= change to =, we get: xidForceLimit Result --- max_xid-2 max_xid-2 max_xid-1 max_xid-1 max_xid max_xid 0 3 1 3 2 3 3 3 -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
On Thu, Mar 31, 2011 at 4:35 PM, Bruce Momjian br...@momjian.us wrote: Robert Haas wrote: The effect is to map max xid + 1 to max xid - FirstNormalTransactionId(3) + 1, which makes the xid look like it is going backwards, less than max xid --- not good. The XID space is *circular*. Right but you would think that as the xid moves forward, the caculation of how far back to vacuum should move only forward. ?In this case, incrementing the xid by one would cause the vacuum horizon to move backward by two. I don't see how that would happen. The XID immediately preceding FirstNormalTransactionId is 2^32-1, and that's exactly what this calculation produces. OK, let me see if I understand --- the caculation is below: xidForceLimit = recentXid - autovacuum_freeze_max_age; if (xidForceLimit FirstNormalTransactionId) xidForceLimit -= FirstNormalTransactionId; The values: xidForceLimit Result --- max_xid-2 max_xid-2 max_xid-1 max_xid-1 max_xid max_xid 0 max_xid-3 - backward here 1 max_xid-2 2 max_xid-1 3 3 You have to consider those three lines all of a piece. Suppose autovacuum_freeze_age is 100. Then: 105 - 5 104 - 4 103 - 3 102 - max_xid 101 - max_xid - 1 100 - max_xid - 2 -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
Robert Haas wrote: ? ?xidForceLimit = recentXid - autovacuum_freeze_max_age; ? ?if (xidForceLimit FirstNormalTransactionId) ? ? ? ?xidForceLimit -= FirstNormalTransactionId; The values: ? ? ? ?xidForceLimit ? Result ? ? ? ?--- ? ? ? ?max_xid-2 ? ? ? max_xid-2 ? ? ? ?max_xid-1 ? ? ? max_xid-1 ? ? ? ?max_xid ? ? ? ? max_xid ? ? ? ?0 ? ? ? ? ? ? ? max_xid-3 ? ? ? - backward here ? ? ? ?1 ? ? ? ? ? ? ? max_xid-2 ? ? ? ?2 ? ? ? ? ? ? ? max_xid-1 ? ? ? ?3 ? ? ? ? ? ? ? 3 You have to consider those three lines all of a piece. Suppose autovacuum_freeze_age is 100. Then: 105 - 5 104 - 4 103 - 3 102 - max_xid 101 - max_xid - 1 100 - max_xid - 2 OK, just keep going below 100: 105 - 5 104 - 4 103 - 3 102 - max_xid 101 - max_xid - 1 100 - max_xid - 2 99 - max_id 98 - max_id -1 Wouldn't you rather: 105 - 5 104 - 4 103 - 3 102 - 3 101 - 3 100 - 3 99 - max_id 98 - max_id -1 -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in autovacuum.c?
On Thu, Mar 31, 2011 at 10:59 PM, Bruce Momjian br...@momjian.us wrote: OK, just keep going below 100: 105 - 5 104 - 4 103 - 3 102 - max_xid 101 - max_xid - 1 100 - max_xid - 2 99 - max_id 98 - max_id -1 Yeah, I think this is what the code is doing. Wouldn't you rather: 105 - 5 104 - 4 103 - 3 102 - 3 101 - 3 100 - 3 99 - max_id 98 - max_id -1 I think I would expect 105 - 5 104 - 4 103 - 3 102 - max_id 101 - max_id-1 100 - max_id-2 99 - max_id-3 But it doesn't really matter either way, does it? We don't even allow setting vacuum_max_freeze_age to 2^31-1 or any value that would be close to triggering a problem here. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
