Thread: 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. (Thislimit 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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Thu, Mar 31, 2011 at 12:17 PM, Bruce Momjian <bruce@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
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 <bruce@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 <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian <bruce@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
Robert Haas wrote: > On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian <bruce@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Thu, Mar 31, 2011 at 4:16 PM, Bruce Momjian <bruce@momjian.us> wrote: > Robert Haas wrote: >> On Thu, Mar 31, 2011 at 2:59 PM, Bruce Momjian <bruce@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
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-2max_xid-1 max_xid-1max_xid max_xid0 max_xid-3 <- backward here1 max_xid-22 max_xid-13 3 With the -= change to =, we get: xidForceLimit Result---------------------------max_xid-2 max_xid-2max_xid-1 max_xid-1max_xid max_xid0 31 32 33 3 -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Thu, Mar 31, 2011 at 4:35 PM, Bruce Momjian <bruce@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
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 -> 5104 -> 4103 -> 3102 -> max_xid101 -> max_xid - 1100 -> max_xid - 2 99 -> max_id 98 -> max_id -1 Wouldn't you rather: 105 -> 5104 -> 4103 -> 3102 -> 3101 -> 3100 -> 3 99 -> max_id 98 -> max_id -1 -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Thu, Mar 31, 2011 at 10:59 PM, Bruce Momjian <bruce@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
Greg Stark wrote: > On Thu, Mar 31, 2011 at 10:59 PM, Bruce Momjian <bruce@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Thu, Mar 31, 2011 at 5:59 PM, Bruce Momjian <bruce@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
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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Fri, Apr 1, 2011 at 11:18 AM, Bruce Momjian <bruce@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
Robert Haas wrote: > On Fri, Apr 1, 2011 at 11:18 AM, Bruce Momjian <bruce@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
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 <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
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 <bruce@momjian.us> http://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; /*
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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
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 wraparoundand the special values. Surely we have a similar problem in the code that increments XIDs, and possibly other placesas well. -- Jim C. Nasby, Database Architect jim@nasby.net 512.569.9461 (cell) http://jim.nasby.net
On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian <bruce@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
Robert Haas wrote: > On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian <bruce@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Robert Haas wrote: > On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian <bruce@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
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 * beingsignificant 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 - 1000000; if (xidStopLimit< FirstNormalTransactionId) xidStopLimit -= FirstNormalTransactionId; Now I am not sure where to add a C comment. :-( -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Robert Haas <robertmhaas@gmail.com> writes: > On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian <bruce@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
Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Fri, Apr 1, 2011 at 5:48 PM, Bruce Momjian <bruce@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 <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +